Attachment 32074 Details for Bug 25642 – see patch

[patch] see patch

0001-Ignore-exit-code-zero-from-activated-services.patch (text/plain), 9.21 KB, created by Colin Walters on 2009-12-14 15:19:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Colin Walters

Created: 2009-12-14 15:19:13 UTC

Size: 9.21 KB

patch

obsolete

>From d8a51a14fea8b2f00e66cc07e692e695a8861c4d Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@verbum.org>
>Date: Mon, 14 Dec 2009 18:12:24 -0500
>Subject: [PATCH] Ignore exit code zero from activated services
>
>A variety of system components have migrated from legacy init into DBus
>service activation.  Many of these system components "daemonize", which
>involves forking.  The DBus activation system treated an exit as an
>activation failure, assuming that the child process which grabbed the
>DBus name didn't run first.
>
>While we're in here, also differentiate in this code path between the
>servicehelper (system) versus direct activation (session) paths.  In
>the session activation path our error message mentioned a helper
>process which was confusing, since none was involved.
>
>Based on a patch and debugging research from Ray Strode <rstrode@redhat.com>
>---
> bus/activation.c                                   |   82 +++++++++++++-------
> configure.in                                       |    1 +
> ...top.DBus.TestSuiteForkingEchoService.service.in |    3 +
> test/name-test/run-test.sh                         |    6 ++
> test/test-service.c                                |   31 +++++++-
> 5 files changed, 91 insertions(+), 32 deletions(-)
> create mode 100644 test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in
> mode change 100755 => 100644 test/name-test/run-test.sh
>
>diff --git a/bus/activation.c b/bus/activation.c
>index 782ffed..9683001 100644
>--- a/bus/activation.c
>+++ b/bus/activation.c
>@@ -1212,8 +1212,8 @@ pending_activation_failed (BusPendingActivation *pending_activation,
>  * Depending on the exit code of the helper, set the error accordingly
>  */
> static void
>-handle_activation_exit_error (int        exit_code,
>-                              DBusError *error)
>+handle_servicehelper_exit_error (int        exit_code,
>+                                 DBusError *error)
> {
>   switch (exit_code)
>     {
>@@ -1268,13 +1268,27 @@ babysitter_watch_callback (DBusWatch     *watch,
>   BusPendingActivation *pending_activation = data;
>   dbus_bool_t retval;
>   DBusBabysitter *babysitter;
>+  dbus_bool_t uses_servicehelper;
> 
>   babysitter = pending_activation->babysitter;
>-  
>+
>   _dbus_babysitter_ref (babysitter);
>-  
>+
>   retval = dbus_watch_handle (watch, condition);
> 
>+  /* There are two major cases here; are we the system bus or the session?  Here this
>+   * is distinguished by whether or not we use a setuid helper launcher.  On the system
>+   * bus, we want to ignore direct child exits (reflected by the DBUS_ERROR_SPAWN_CHILD_EXITED)
>+   * because it may have (misguidedly) "daemonized", as can happen when code is taken from
>+   * an init script and put into a DBus service file.
>+   *
>+   * On the session bus, we're monitoring the binary directly, not monitoring the service helper.
>+   * In that case, just ignore the return value entirely; while it's less likely session code
>+   * will be daemonizing, it's possible.  The tradeoff is here system resilience versus
>+   * developer/sysadmin convenience, and I've decided the former wins.
>+   */
>+  uses_servicehelper = bus_context_get_servicehelper (pending_activation->activation->context) != NULL;
>+
>   /* FIXME this is broken in the same way that
>    * connection watches used to be; there should be
>    * a separate callback for status change, instead
>@@ -1284,43 +1298,53 @@ babysitter_watch_callback (DBusWatch     *watch,
>    * Fixing this lets us move dbus_watch_handle
>    * calls into dbus-mainloop.c
>    */
>-  
>   if (_dbus_babysitter_get_child_exited (babysitter))
>     {
>       DBusError error;
>       DBusHashIter iter;
>-      
>+      dbus_bool_t activation_failed;
>+      int exit_code = 0;
>+
>       dbus_error_init (&error);
>-      _dbus_babysitter_set_child_exit_error (babysitter, &error);
> 
>-      /* refine the error code if we got an exit code */
>-      if (dbus_error_has_name (&error, DBUS_ERROR_SPAWN_CHILD_EXITED))
>-      	{
>-          int exit_code = 0;
>-          if (_dbus_babysitter_get_child_exit_status (babysitter, &exit_code))
>+      if (_dbus_babysitter_get_child_exit_status (babysitter, &exit_code))
>+        {
>+          activation_failed = exit_code != 0;
>+
>+          if (activation_failed)
>             {
>-              dbus_error_free (&error);
>-              handle_activation_exit_error (exit_code, &error);
>+              if (uses_servicehelper)
>+                handle_servicehelper_exit_error (exit_code, &error);
>+              else
>+                _dbus_babysitter_set_child_exit_error (babysitter, &error);
>             }
>-      	}
>-
>-      /* Destroy all pending activations with the same exec */
>-      _dbus_hash_iter_init (pending_activation->activation->pending_activations,
>-                            &iter);
>-      while (_dbus_hash_iter_next (&iter))
>+        }
>+      else
>         {
>-          BusPendingActivation *p = _dbus_hash_iter_get_value (&iter);
>-         
>-          if (p != pending_activation && strcmp (p->exec, pending_activation->exec) == 0)
>-            pending_activation_failed (p, &error);
>+          activation_failed = TRUE;
>+          _dbus_babysitter_set_child_exit_error (babysitter, &error);
>         }
>-      
>-      /* Destroys the pending activation */
>-      pending_activation_failed (pending_activation, &error);
> 
>-      dbus_error_free (&error);
>+      if (activation_failed)
>+        {
>+          /* Destroy all pending activations with the same exec */
>+          _dbus_hash_iter_init (pending_activation->activation->pending_activations,
>+                                &iter);
>+          while (_dbus_hash_iter_next (&iter))
>+            {
>+              BusPendingActivation *p = _dbus_hash_iter_get_value (&iter);
>+
>+              if (p != pending_activation && strcmp (p->exec, pending_activation->exec) == 0)
>+                pending_activation_failed (p, &error);
>+            }
>+
>+          /* Destroys the pending activation */
>+          pending_activation_failed (pending_activation, &error);
>+
>+          dbus_error_free (&error);
>+        }
>     }
>-  
>+
>   _dbus_babysitter_unref (babysitter);
> 
>   return retval;
>diff --git a/configure.in b/configure.in
>index 7ef6632..1f2c896 100644
>--- a/configure.in
>+++ b/configure.in
>@@ -1499,6 +1499,7 @@ test/data/valid-config-files-system/debug-allow-all-pass.conf
> test/data/valid-config-files-system/debug-allow-all-fail.conf
> test/data/valid-service-files/org.freedesktop.DBus.TestSuite.PrivServer.service
> test/data/valid-service-files/org.freedesktop.DBus.TestSuiteEchoService.service
>+test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service
> test/data/valid-service-files/org.freedesktop.DBus.TestSuiteSegfaultService.service
> test/data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceSuccess.service
> test/data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceFail.service
>diff --git a/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in b/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in
>new file mode 100644
>index 0000000..49fcac3
>--- /dev/null
>+++ b/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in
>@@ -0,0 +1,3 @@
>+[D-BUS Service]
>+Name=org.freedesktop.DBus.TestSuiteForkingEchoService
>+Exec=@TEST_SERVICE_BINARY@ org.freedesktop.DBus.TestSuiteForkingEchoService fork
>diff --git a/test/name-test/run-test.sh b/test/name-test/run-test.sh
>old mode 100755
>new mode 100644
>index fba4558..4eb2425
>--- a/test/name-test/run-test.sh
>+++ b/test/name-test/run-test.sh
>@@ -50,3 +50,9 @@ ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/name-
> 
> echo "running test-shutdown"
> ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/name-test/test-shutdown || die "test-shutdown failed"
>+
>+echo "running test activation forking"
>+if ! python $DBUS_TOP_SRCDIR/test/name-test/test-activation-forking.py; then
>+  echo "Failed test-activation-forking"
>+  exit 1
>+fi
>diff --git a/test/test-service.c b/test/test-service.c
>index c9f5839..6088006 100644
>--- a/test/test-service.c
>+++ b/test/test-service.c
>@@ -398,7 +398,32 @@ main (int    argc,
>   DBusError error;
>   int result;
>   DBusConnection *connection;
>-  
>+  const char *name;
>+  dbus_bool_t do_fork;
>+
>+  if (argc != 3)
>+    {
>+      name = "org.freedesktop.DBus.TestSuiteEchoService";
>+      do_fork = FALSE;
>+    }
>+  else
>+    {
>+      name = argv[1];
>+      do_fork = strcmp (argv[2], "fork") == 0;
>+    }
>+
>+  /* The bare minimum for simulating a program "daemonizing"; the intent
>+   * is to test services which move from being legacy init scripts to
>+   * activated services.
>+   * https://bugzilla.redhat.com/show_bug.cgi?id=545267
>+   */
>+  if (do_fork)
>+    {
>+      pid_t pid = fork ();
>+      if (pid != 0)
>+        exit (0);
>+    }
>+
>   dbus_error_init (&error);
>   connection = dbus_bus_get (DBUS_BUS_STARTER, &error);
>   if (connection == NULL)
>@@ -433,8 +458,8 @@ main (int    argc,
>     if (d != (void*) 0xdeadbeef)
>       die ("dbus_connection_get_object_path_data() doesn't seem to work right\n");
>   }
>-  
>-  result = dbus_bus_request_name (connection, "org.freedesktop.DBus.TestSuiteEchoService",
>+
>+  result = dbus_bus_request_name (connection, name,
>                                   0, &error);
>   if (dbus_error_is_set (&error))
>     {
>-- 
>1.6.2.5
>

Actions: View | Splinter Review

Attachments on bug 25642: 32074 | 32075

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.