2003-04-05 Havoc Pennington <hp@pobox.com>

	* bus/loop.c (bus_loop_iterate): fix the timeout code, using
	magic from GLib

	* dbus/dbus-spawn.c (_dbus_babysitter_unref): set sitter_pid
	to -1 once we've reaped the babysitter
	(_dbus_babysitter_handle_watch): do as much work as we can, not
	just one go of it

	* bus/activation.c: add code using DBusBabysitter so that we
	handle it when a service fails to start up properly.
	(bus_activation_service_created): don't remove the activation
	entries as we go, just let them get removed when we free the pending
	activation. Unref reply messages after sending them.

8 files changed, 396 insertions, 70 deletions

diff --git a/ChangeLog b/ChangeLog
index 521d951a..d04731c9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,21 @@
 2003-04-05  Havoc Pennington  <hp@pobox.com>
 
+	* bus/loop.c (bus_loop_iterate): fix the timeout code, using 
+	magic from GLib
+
+	* dbus/dbus-spawn.c (_dbus_babysitter_unref): set sitter_pid 
+	to -1 once we've reaped the babysitter
+	(_dbus_babysitter_handle_watch): do as much work as we can, not
+	just one go of it
+
+	* bus/activation.c: add code using DBusBabysitter so that we
+	handle it when a service fails to start up properly.
+	(bus_activation_service_created): don't remove the activation
+	entries as we go, just let them get removed when we free the pending
+	activation. Unref reply messages after sending them.
+
+2003-04-05  Havoc Pennington  <hp@pobox.com>
+
 	* test/decode-gcov.c (main): print per-directory stats in the report
 
 	* Makefile.am (coverage-report.txt): don't include test/* in gcov stats
diff --git a/bus/activation.c b/bus/activation.c
index 2045baa2..54ddd948 100644
--- a/bus/activation.c
+++ b/bus/activation.c
@@ -29,6 +29,7 @@
 #include <dbus/dbus-hash.h>
 #include <dbus/dbus-list.h>
 #include <dbus/dbus-spawn.h>
+#include <dbus/dbus-timeout.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <errno.h>
@@ -62,8 +63,12 @@ struct BusPendingActivationEntry
 
 typedef struct
 {
+  BusActivation *activation;
   char *service_name;
   DBusList *entries;
+  DBusBabysitter *babysitter;
+  DBusTimeout *timeout;
+  unsigned int timeout_added : 1;
 } BusPendingActivation;
 
 static void
@@ -74,21 +79,53 @@ bus_pending_activation_entry_free (BusPendingActivationEntry *entry)
   
   if (entry->connection)
     dbus_connection_unref (entry->connection);
-
+  
   dbus_free (entry);
 }
 
 static void
-bus_pending_activation_free (BusPendingActivation *activation)
+handle_timeout_callback (DBusTimeout   *timeout,
+                         void          *data)
+{
+  BusPendingActivation *pending_activation = data;
+
+  while (!dbus_timeout_handle (pending_activation->timeout))
+    bus_wait_for_memory ();
+}
+
+static void
+bus_pending_activation_free (BusPendingActivation *pending_activation)
 {
   DBusList *link;
   
-  if (!activation)
+  if (pending_activation == NULL) /* hash table requires this */
     return;
 
-  dbus_free (activation->service_name);
+  if (pending_activation->timeout_added)
+    {
+      bus_loop_remove_timeout (bus_context_get_loop (pending_activation->activation->context),
+                               pending_activation->timeout,
+                               handle_timeout_callback, pending_activation);
+      pending_activation->timeout_added = FALSE;
+    }
+
+  if (pending_activation->timeout)
+    _dbus_timeout_unref (pending_activation->timeout);
+  
+  if (pending_activation->babysitter)
+    {
+      if (!_dbus_babysitter_set_watch_functions (pending_activation->babysitter,
+                                                 NULL, NULL, NULL,
+                                                 pending_activation->babysitter,
+                                                 NULL))
+        _dbus_assert_not_reached ("setting watch functions to NULL failed");
+      
+      _dbus_babysitter_unref (pending_activation->babysitter);
+    }
+  
+  dbus_free (pending_activation->service_name);
 
-  link = _dbus_list_get_first_link (&activation->entries);
+  link = _dbus_list_get_first_link (&pending_activation->entries);
 
   while (link != NULL)
     {
@@ -96,11 +133,11 @@ bus_pending_activation_free (BusPendingActivation *activation)
 
       bus_pending_activation_entry_free (entry);
 
-      link = _dbus_list_get_next_link (&activation->entries, link);
+      link = _dbus_list_get_next_link (&pending_activation->entries, link);
     }
-  _dbus_list_clear (&activation->entries);
+  _dbus_list_clear (&pending_activation->entries);
   
-  dbus_free (activation);
+  dbus_free (pending_activation);
 }
 
 static void
@@ -478,11 +515,10 @@ bus_activation_service_created (BusActivation  *activation,
 	      BUS_SET_OOM (error);
 	      goto error;
 	    }
+
+          dbus_message_unref (message);
 	}
 
-      bus_pending_activation_entry_free (entry);
-      
-      _dbus_list_remove_link (&pending_activation->entries, link);      
       link = next;
     }
   
@@ -495,6 +531,165 @@ bus_activation_service_created (BusActivation  *activation,
   return FALSE;
 }
 
+/**
+ * FIXME @todo the error messages here would ideally be preallocated
+ * so we don't need to allocate memory to send them.
+ * Using the usual tactic, prealloc an OOM message, then
+ * if we can't alloc the real error send the OOM error instead.
+ */
+static dbus_bool_t
+try_send_activation_failure (BusPendingActivation *pending_activation,
+                             const DBusError      *how)
+{
+  BusActivation *activation;
+  DBusMessage *message;
+  DBusList *link;
+  BusTransaction *transaction;
+  
+  activation = pending_activation->activation;
+
+  transaction = bus_transaction_new (activation->context);
+  if (transaction == NULL)
+    return FALSE;
+  
+  link = _dbus_list_get_first_link (&pending_activation->entries);
+  while (link != NULL)
+    {
+      BusPendingActivationEntry *entry = link->data;
+      DBusList *next = _dbus_list_get_next_link (&pending_activation->entries, link);
+      
+      if (dbus_connection_get_is_connected (entry->connection))
+	{
+	  message = dbus_message_new_error_reply (entry->activation_message,
+                                                  how->name,
+                                                  how->message);
+	  if (!message)
+            goto error;
+
+	  if (!dbus_message_set_sender (message, DBUS_SERVICE_DBUS))
+            {
+	      dbus_message_unref (message);
+	      goto error;
+	    }
+          
+	  if (!bus_transaction_send_message (transaction, entry->connection, message))
+	    {
+	      dbus_message_unref (message);
+	      goto error;
+	    }
+
+          dbus_message_unref (message);
+	}
+
+      link = next;
+    }
+
+  bus_transaction_execute_and_free (transaction);
+  
+  return TRUE;
+
+ error:
+  if (transaction)
+    bus_transaction_cancel_and_free (transaction);
+  return FALSE;
+}
+
+/**
+ * Free the pending activation and send an error message to all the
+ * connections that were waiting for it.
+ */
+static void
+pending_activation_failed (BusPendingActivation *pending_activation,
+                           const DBusError      *how)
+{
+  /* FIXME use preallocated OOM messages instead of bus_wait_for_memory() */
+  while (!try_send_activation_failure (pending_activation, how))
+    bus_wait_for_memory ();
+
+  /* Destroy this pending activation */
+  _dbus_hash_table_remove_string (pending_activation->activation->pending_activations,
+                                  pending_activation->service_name);
+}
+
+static dbus_bool_t
+babysitter_watch_callback (DBusWatch     *watch,
+                           unsigned int   condition,
+                           void          *data)
+{
+  BusPendingActivation *pending_activation = data;
+  dbus_bool_t retval;
+  DBusBabysitter *babysitter;
+
+  babysitter = pending_activation->babysitter;
+  
+  _dbus_babysitter_ref (babysitter);
+  
+  retval = _dbus_babysitter_handle_watch (babysitter, watch, condition);
+
+  if (_dbus_babysitter_get_child_exited (babysitter))
+    {
+      DBusError error;
+
+      dbus_error_init (&error);
+      _dbus_babysitter_set_child_exit_error (babysitter, &error);
+
+      /* Destroys the pending activation */
+      pending_activation_failed (pending_activation, &error);
+
+      dbus_error_free (&error);
+    }
+  
+  _dbus_babysitter_unref (babysitter);
+
+  return retval;
+}
+
+static dbus_bool_t
+add_babysitter_watch (DBusWatch      *watch,
+                      void           *data)
+{
+  BusPendingActivation *pending_activation = data;
+
+  return bus_loop_add_watch (bus_context_get_loop (pending_activation->activation->context),
+                             watch, babysitter_watch_callback, pending_activation,
+                             NULL);
+}
+
+static void
+remove_babysitter_watch (DBusWatch      *watch,
+                         void           *data)
+{
+  BusPendingActivation *pending_activation = data;
+  
+  bus_loop_remove_watch (bus_context_get_loop (pending_activation->activation->context),
+                         watch, babysitter_watch_callback, pending_activation);
+}
+
+static dbus_bool_t
+pending_activation_timed_out (void *data)
+{
+  BusPendingActivation *pending_activation = data;
+  DBusError error;
+  
+  /* Kill the spawned process, since it sucks
+   * (not sure this is what we want to do, but
+   * may as well try it for now)
+   */
+  _dbus_babysitter_kill_child (pending_activation->babysitter);
+
+  dbus_error_init (&error);
+
+  dbus_set_error (&error, DBUS_ERROR_TIMED_OUT,
+                  "Activation of %s timed out",
+                  pending_activation->service_name);
+
+  pending_activation_failed (pending_activation, &error);
+
+  dbus_error_free (&error);
+
+  return TRUE;
+}
+
 dbus_bool_t
 bus_activation_activate_service (BusActivation  *activation,
 				 DBusConnection *connection,
@@ -586,6 +781,9 @@ bus_activation_activate_service (BusActivation  *activation,
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
 	  return FALSE;
 	}
+
+      pending_activation->activation = activation;
+      
       pending_activation->service_name = _dbus_strdup (service_name);
       if (!pending_activation->service_name)
 	{
@@ -595,6 +793,33 @@ bus_activation_activate_service (BusActivation  *activation,
 	  return FALSE;
 	}
 
+      pending_activation->timeout =
+        _dbus_timeout_new (bus_context_get_activation_timeout (activation->context),
+                           pending_activation_timed_out,
+                           pending_activation,
+                           NULL);
+      if (!pending_activation->timeout)
+	{
+	  BUS_SET_OOM (error);
+	  bus_pending_activation_free (pending_activation);
+	  bus_pending_activation_entry_free (pending_activation_entry);	  
+	  return FALSE;
+	}
+
+      if (!bus_loop_add_timeout (bus_context_get_loop (activation->context),
+                                 pending_activation->timeout,
+                                 handle_timeout_callback,
+                                 pending_activation,
+                                 NULL))
+	{
+	  BUS_SET_OOM (error);
+	  bus_pending_activation_free (pending_activation);
+	  bus_pending_activation_entry_free (pending_activation_entry);	  
+	  return FALSE;
+	}
+
+      pending_activation->timeout_added = TRUE;
+      
       if (!_dbus_list_append (&pending_activation->entries, pending_activation_entry))
 	{
 	  BUS_SET_OOM (error);
@@ -620,7 +845,7 @@ bus_activation_activate_service (BusActivation  *activation,
   argv[0] = entry->exec;
   argv[1] = NULL;
 
-  if (!_dbus_spawn_async_with_babysitter (NULL, argv,
+  if (!_dbus_spawn_async_with_babysitter (&pending_activation->babysitter, argv,
                                           child_setup, activation, 
                                           error))
     {
@@ -628,6 +853,22 @@ bus_activation_activate_service (BusActivation  *activation,
 				      pending_activation->service_name);
       return FALSE;
     }
+
+  _dbus_assert (pending_activation->babysitter != NULL);
+  
+  if (!_dbus_babysitter_set_watch_functions (pending_activation->babysitter,
+                                             add_babysitter_watch,
+                                             remove_babysitter_watch,
+                                             NULL,
+                                             pending_activation,
+                                             NULL))
+    {
+      BUS_SET_OOM (error);
+      
+      _dbus_hash_table_remove_string (activation->pending_activations,
+				      pending_activation->service_name);
+      return FALSE;
+    }
   
   return TRUE;
 }
diff --git a/bus/bus.c b/bus/bus.c
index 9125fc75..5ae77d62 100644
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -47,6 +47,7 @@ struct BusContext
   DBusList *mandatory_rules;    /**< Mandatory policy rules */
   DBusHashTable *rules_by_uid;  /**< per-UID policy rules */
   DBusHashTable *rules_by_gid;  /**< per-GID policy rules */
+  int activation_timeout;       /**< How long to wait for an activation to time out */
 };
 
 static int server_data_slot = -1;
@@ -333,6 +334,12 @@ bus_context_new (const DBusString *config_file,
   
   context->refcount = 1;
 
+#ifdef DBUS_BUILD_TESTS
+  context->activation_timeout = 6000;   /* 6/10 second */ /* FIXME */
+#else
+  context->activation_timeout = 10000; /* 10 seconds */
+#endif
+  
   context->loop = bus_loop_new ();
   if (context->loop == NULL)
     {
@@ -853,3 +860,10 @@ bus_context_create_connection_policy (BusContext      *context,
   bus_policy_unref (policy);
   return NULL;
 }
+
+int
+bus_context_get_activation_timeout (BusContext *context)
+{
+  
+  return context->activation_timeout;
+}
diff --git a/bus/bus.h b/bus/bus.h
index 8357f7b8..8b26cffc 100644
--- a/bus/bus.h
+++ b/bus/bus.h
@@ -54,5 +54,6 @@ dbus_bool_t     bus_context_allow_user               (BusContext       *context,
                                                       unsigned long     uid);
 BusPolicy*      bus_context_create_connection_policy (BusContext       *context,
                                                       DBusConnection   *connection);
+int             bus_context_get_activation_timeout   (BusContext       *context);
 
 #endif /* BUS_BUS_H */
diff --git a/bus/dispatch.c b/bus/dispatch.c
index 21d57ae1..6e3a61e3 100644
--- a/bus/dispatch.c
+++ b/bus/dispatch.c
@@ -1070,9 +1070,11 @@ check_existent_service_activation (BusContext     *context,
 
   bus_test_run_everything (context);
 
-  /* now wait for the message bus to hear back from the activated service */
-  bus_test_run_bus_loop (context);
-
+  if (dbus_connection_get_dispatch_status (connection) ==
+      DBUS_DISPATCH_COMPLETE)
+    /* now wait for the message bus to hear back from the activated service */
+    bus_test_run_bus_loop (context);
+  
   /* and process everything again */
   bus_test_run_everything (context);
 
diff --git a/bus/loop.c b/bus/loop.c
index a237defa..e2e129eb 100644
--- a/bus/loop.c
+++ b/bus/loop.c
@@ -312,6 +312,90 @@ bus_loop_remove_timeout (BusLoop            *loop,
               timeout, function, data);
 }
 
+/* Convolutions from GLib, there really must be a better way
+ * to do this.
+ */
+static dbus_bool_t
+check_timeout (unsigned long    tv_sec,
+               unsigned long    tv_usec,
+               TimeoutCallback *tcb,
+               int             *timeout)
+{
+  long sec;
+  long msec;
+  unsigned long expiration_tv_sec;
+  unsigned long expiration_tv_usec;
+  long interval_seconds;
+  long interval_milliseconds;
+  int interval;
+
+  interval = dbus_timeout_get_interval (tcb->timeout);
+  
+  interval_seconds = interval / 1000;
+  interval_milliseconds = interval - interval_seconds * 1000;
+  
+  expiration_tv_sec = tcb->last_tv_sec + interval_seconds;
+  expiration_tv_usec = tcb->last_tv_usec + interval_milliseconds * 1000;
+  if (expiration_tv_usec >= 1000000)
+    {
+      expiration_tv_usec -= 1000000;
+      expiration_tv_sec += 1;
+    }
+  
+  sec = expiration_tv_sec - tv_sec;
+  msec = (expiration_tv_usec - tv_usec) / 1000;
+
+#if 0
+  printf ("Interval is %ld seconds %ld msecs\n",
+          interval_seconds,
+          interval_milliseconds);
+  printf ("Now is %lu seconds %lu usecs\n",
+          tv_sec, tv_usec);
+  printf ("Exp is %lu seconds %lu usecs\n",
+          expiration_tv_sec, expiration_tv_usec);
+  printf ("Pre-correction, remaining sec %ld msec %ld\n", sec, msec);
+#endif
+  
+  /* We do the following in a rather convoluted fashion to deal with
+   * the fact that we don't have an integral type big enough to hold
+   * the difference of two timevals in millseconds.
+   */
+  if (sec < 0 || (sec == 0 && msec < 0))
+    msec = 0;
+  else
+    {
+      if (msec < 0)
+	{
+	  msec += 1000;
+	  sec -= 1;
+	}
+      
+      if (sec > interval_seconds ||
+	  (sec == interval_seconds && msec > interval_milliseconds))
+	{
+	  /* The system time has been set backwards, reset the timeout */
+          tcb->last_tv_sec = tv_sec;
+          tcb->last_tv_usec = tv_usec;
+          
+          msec = MIN (_DBUS_INT_MAX, interval);
+
+          _dbus_verbose ("System clock went backward\n");
+	}
+      else
+	{
+	  msec = MIN (_DBUS_INT_MAX, (unsigned int)msec + 1000 * (unsigned int)sec);
+	}
+    }
+
+  *timeout = msec;
+
+#if 0
+  printf ("Timeout expires in %d milliseconds\n", *timeout);
+#endif
+  
+  return msec == 0;
+}
+
 /* Returns TRUE if we have any timeouts or ready file descriptors,
  * which is just used in test code as a debug hack
  */
@@ -447,34 +531,15 @@ bus_loop_iterate (BusLoop     *loop,
               dbus_timeout_get_enabled (TIMEOUT_CALLBACK (cb)->timeout))
             {
               TimeoutCallback *tcb = TIMEOUT_CALLBACK (cb);
-              unsigned long interval;
-              unsigned long elapsed;
+              int msecs_remaining;
 
-              if (tcb->last_tv_sec > tv_sec ||
-                  (tcb->last_tv_sec == tv_sec &&
-                   tcb->last_tv_usec > tv_usec))
-                {
-                  /* Clock went backward, pretend timeout
-                   * was just installed.
-                   */
-                  tcb->last_tv_sec = tv_sec;
-                  tcb->last_tv_usec = tv_usec;
-                  _dbus_verbose ("System clock went backward\n");
-                }
-                  
-              interval = dbus_timeout_get_interval (tcb->timeout);
+              check_timeout (tv_sec, tv_usec, tcb, &msecs_remaining);
 
-              elapsed =
-                (tv_sec - tcb->last_tv_sec) * 1000 +
-                (tv_usec - tcb->last_tv_usec) / 1000;
-
-              if (interval < elapsed)
-                timeout = 0;
-              else if (timeout < 0)
-                timeout = interval - elapsed;
+              if (timeout < 0)
+                timeout = msecs_remaining;
               else
-                timeout = MIN (((unsigned long)timeout), interval - elapsed);
-
+                timeout = MIN (msecs_remaining, timeout);
+              
               _dbus_assert (timeout >= 0);
                   
               if (timeout == 0)
@@ -486,7 +551,12 @@ bus_loop_iterate (BusLoop     *loop,
     }
 
   if (!block)
-    timeout = 0;
+    {
+      timeout = 0;
+#if 0
+      printf ("timeout is 0 as we aren't blocking\n");
+#endif
+    }
 
   /* if a watch is OOM, don't wait longer than the OOM
    * wait to re-enable it
@@ -522,41 +592,17 @@ bus_loop_iterate (BusLoop     *loop,
               dbus_timeout_get_enabled (TIMEOUT_CALLBACK (cb)->timeout))
             {
               TimeoutCallback *tcb = TIMEOUT_CALLBACK (cb);
-              unsigned long interval;
-              unsigned long elapsed;
-                  
-              if (tcb->last_tv_sec > tv_sec ||
-                  (tcb->last_tv_sec == tv_sec &&
-                   tcb->last_tv_usec > tv_usec))
-                {
-                  /* Clock went backward, pretend timeout
-                   * was just installed.
-                   */
-                  tcb->last_tv_sec = tv_sec;
-                  tcb->last_tv_usec = tv_usec;
-                  _dbus_verbose ("System clock went backward\n");
-                  goto next_timeout;
-                }
-                  
-              interval = dbus_timeout_get_interval (tcb->timeout);
-
-              elapsed =
-                (tv_sec - tcb->last_tv_sec) * 1000 +
-                (tv_usec - tcb->last_tv_usec) / 1000;
-
-#if 0
-              _dbus_verbose ("  interval = %lu elapsed = %lu\n",
-                             interval, elapsed);
-#endif
+              int msecs_remaining;
               
-              if (interval <= elapsed)
+              if (check_timeout (tv_sec, tv_usec,
+                                 tcb, &msecs_remaining))
                 {
                   /* Save last callback time and fire this timeout */
                   tcb->last_tv_sec = tv_sec;
                   tcb->last_tv_usec = tv_usec;
 
 #if 0
-                  _dbus_verbose ("  invoking timeout\n");
+                  printf ("  invoking timeout\n");
 #endif
                   
                   (* tcb->function) (tcb->timeout,
@@ -564,7 +610,6 @@ bus_loop_iterate (BusLoop     *loop,
                 }
             }
 
-        next_timeout:
           link = next;
         }
     }
diff --git a/dbus/dbus-errors.h b/dbus/dbus-errors.h
index 59fa1e49..8d8e16ec 100644
--- a/dbus/dbus-errors.h
+++ b/dbus/dbus-errors.h
@@ -73,6 +73,7 @@ struct DBusError
 #define DBUS_ERROR_INVALID_ARGS               "org.freedesktop.DBus.Error.InvalidArgs"
 #define DBUS_ERROR_FILE_NOT_FOUND             "org.freedesktop.DBus.Error.FileNotFound"
 #define DBUS_ERROR_UNKNOWN_MESSAGE            "org.freedesktop.DBus.Error.UnknownMessage"
+#define DBUS_ERROR_TIMED_OUT                  "org.freedesktop.DBus.Error.TimedOut"
 
 void        dbus_error_init      (DBusError       *error);
 void        dbus_error_free      (DBusError       *error);
diff --git a/dbus/dbus-spawn.c b/dbus/dbus-spawn.c
index 5ced84fc..5fa2d5e8 100644
--- a/dbus/dbus-spawn.c
+++ b/dbus/dbus-spawn.c
@@ -293,6 +293,8 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
               else
                 _dbus_verbose ("Babysitter exited abnormally\n");
             }
+
+          sitter->sitter_pid = -1;
         }
       
       if (sitter->error_watch)
@@ -699,6 +701,10 @@ _dbus_babysitter_handle_watch (DBusBabysitter  *sitter,
     handle_error_pipe (sitter, revents);
   else if (fd == sitter->socket_to_babysitter)
     handle_babysitter_socket (sitter, revents);
+
+  while (LIVE_CHILDREN (sitter) &&
+         babysitter_iteration (sitter, FALSE))
+    ;
   
   return TRUE;
 }