DBusLoop: remove a layer of pointless abstraction around timeouts

Instead of supplying 8 tiny wrapper functions around dbus_timeout_handle,
each with a user_data parameter that's a potentially unsafe borrowed
pointer but isn't actually used, we can call dbus_timeout_handle directly
and save a lot of trouble.

One of the wrappers previously called dbus_timeout_handle repeatedly
if it returned FALSE to indicate OOM, but that timeout's handler never
actually returned FALSE, so there was no practical effect. The rest just
ignore the return, which is documented as OK to do.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33342
Reviewed-by: Thiago Macieira <thiago@kde.org>

diff --git a/bus/activation.c b/bus/activation.c
index 3177d02..31dbaf5 100644 (file)
--- a/bus/activation.c
+++ b/bus/activation.c
@@ -143,16 +143,6 @@ bus_pending_activation_entry_free (BusPendingActivationEntry *entry)
   dbus_free (entry);
 }
 
-static void
-handle_timeout_callback (DBusTimeout   *timeout,
-                         void          *data)
-{
-  BusPendingActivation *pending_activation = data;
-
-  while (!dbus_timeout_handle (pending_activation->timeout))
-    _dbus_wait_for_memory ();
-}
-
 static BusPendingActivation *
 bus_pending_activation_ref (BusPendingActivation *pending_activation)
 {
@@ -179,8 +169,7 @@ bus_pending_activation_unref (BusPendingActivation *pending_activation)
   if (pending_activation->timeout_added)
     {
       _dbus_loop_remove_timeout (bus_context_get_loop (pending_activation->activation->context),
-                                 pending_activation->timeout,
-                                 handle_timeout_callback, pending_activation);
+                                 pending_activation->timeout);
       pending_activation->timeout_added = FALSE;
     }
 
@@ -1860,10 +1849,7 @@ bus_activation_activate_service (BusActivation  *activation,
         }
 
       if (!_dbus_loop_add_timeout (bus_context_get_loop (activation->context),
-                                   pending_activation->timeout,
-                                   handle_timeout_callback,
-                                   pending_activation,
-                                   NULL))
+                                   pending_activation->timeout))
         {
           _dbus_verbose ("Failed to add timeout for pending activation\n");
 

diff --git a/bus/bus.c b/bus/bus.c
index 6b0dc08..2008d1d 100644 (file)
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -142,15 +142,6 @@ remove_server_watch (DBusWatch  *watch,
                            watch, server_watch_callback, server);
 }
 
-
-static void
-server_timeout_callback (DBusTimeout   *timeout,
-                         void          *data)
-{
-  /* can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 static dbus_bool_t
 add_server_timeout (DBusTimeout *timeout,
                     void        *data)
@@ -160,8 +151,7 @@ add_server_timeout (DBusTimeout *timeout,
 
   context = server_get_context (server);
 
-  return _dbus_loop_add_timeout (context->loop,
-                                 timeout, server_timeout_callback, server, NULL);
+  return _dbus_loop_add_timeout (context->loop, timeout);
 }
 
 static void
@@ -173,8 +163,7 @@ remove_server_timeout (DBusTimeout *timeout,
 
   context = server_get_context (server);
 
-  _dbus_loop_remove_timeout (context->loop,
-                             timeout, server_timeout_callback, server);
+  _dbus_loop_remove_timeout (context->loop, timeout);
 }
 
 static void

diff --git a/bus/connection.c b/bus/connection.c
index 8e7d222..692948e 100644 (file)
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -331,24 +331,13 @@ remove_connection_watch (DBusWatch      *watch,
                            watch, connection_watch_callback, connection);
 }
 
-static void
-connection_timeout_callback (DBusTimeout   *timeout,
-                             void          *data)
-{
-  /* DBusConnection *connection = data; */
-
-  /* can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 static dbus_bool_t
 add_connection_timeout (DBusTimeout    *timeout,
                         void           *data)
 {
   DBusConnection *connection = data;
   
-  return _dbus_loop_add_timeout (connection_get_loop (connection),
-                                 timeout, connection_timeout_callback, connection, NULL);
+  return _dbus_loop_add_timeout (connection_get_loop (connection), timeout);
 }
 
 static void
@@ -357,8 +346,7 @@ remove_connection_timeout (DBusTimeout    *timeout,
 {
   DBusConnection *connection = data;
   
-  _dbus_loop_remove_timeout (connection_get_loop (connection),
-                             timeout, connection_timeout_callback, connection);
+  _dbus_loop_remove_timeout (connection_get_loop (connection), timeout);
 }
 
 static void
@@ -460,8 +448,7 @@ bus_connections_new (BusContext *context)
     goto failed_4;
   
   if (!_dbus_loop_add_timeout (bus_context_get_loop (context),
-                               connections->expire_timeout,
-                               call_timeout_callback, NULL, NULL))
+                               connections->expire_timeout))
     goto failed_5;
   
   connections->refcount = 1;
@@ -532,8 +519,7 @@ bus_connections_unref (BusConnections *connections)
       bus_expire_list_free (connections->pending_replies);
       
       _dbus_loop_remove_timeout (bus_context_get_loop (connections->context),
-                                 connections->expire_timeout,
-                                 call_timeout_callback, NULL);
+                                 connections->expire_timeout);
       
       _dbus_timeout_unref (connections->expire_timeout);
       

diff --git a/bus/expirelist.c b/bus/expirelist.c
index 946a615..3c87c11 100644 (file)
--- a/bus/expirelist.c
+++ b/bus/expirelist.c
@@ -40,14 +40,6 @@ struct BusExpireList
 
 static dbus_bool_t expire_timeout_handler (void *data);
 
-static void
-call_timeout_callback (DBusTimeout   *timeout,
-                       void          *data)
-{
-  /* can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 BusExpireList*
 bus_expire_list_new (DBusLoop      *loop,
                      int            expire_after,
@@ -73,9 +65,7 @@ bus_expire_list_new (DBusLoop      *loop,
 
   _dbus_timeout_set_enabled (list->timeout, FALSE);
 
-  if (!_dbus_loop_add_timeout (list->loop,
-                               list->timeout,
-                               call_timeout_callback, NULL, NULL))
+  if (!_dbus_loop_add_timeout (list->loop, list->timeout))
     goto failed;
 
   return list;
@@ -94,8 +84,7 @@ bus_expire_list_free (BusExpireList *list)
 {
   _dbus_assert (list->items == NULL);
 
-  _dbus_loop_remove_timeout (list->loop, list->timeout,
-                             call_timeout_callback, NULL);
+  _dbus_loop_remove_timeout (list->loop, list->timeout);
 
   _dbus_timeout_unref (list->timeout);
 

diff --git a/bus/test.c b/bus/test.c
index 95c7cfb..8d16340 100644 (file)
--- a/bus/test.c
+++ b/bus/test.c
@@ -70,23 +70,13 @@ remove_client_watch (DBusWatch      *watch,
                            watch, client_watch_callback, connection);
 }
 
-static void
-client_timeout_callback (DBusTimeout   *timeout,
-                         void          *data)
-{
-  DBusConnection *connection = data;
-
-  /* can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 static dbus_bool_t
 add_client_timeout (DBusTimeout    *timeout,
                     void           *data)
 {
   DBusConnection *connection = data;
 
-  return _dbus_loop_add_timeout (client_loop, timeout, client_timeout_callback, connection, NULL);
+  return _dbus_loop_add_timeout (client_loop, timeout);
 }
 
 static void
@@ -95,7 +85,7 @@ remove_client_timeout (DBusTimeout    *timeout,
 {
   DBusConnection *connection = data;
 
-  _dbus_loop_remove_timeout (client_loop, timeout, client_timeout_callback, connection);
+  _dbus_loop_remove_timeout (client_loop, timeout);
 }
 
 static DBusHandlerResult

diff --git a/dbus/dbus-mainloop.c b/dbus/dbus-mainloop.c
index a25b31d..087bb5b 100644 (file)
--- a/dbus/dbus-mainloop.c
+++ b/dbus/dbus-mainloop.c
@@ -74,8 +74,6 @@ typedef struct
 {
   int refcount;
   CallbackType type;
-  void *data;
-  DBusFreeFunction free_data_func;
 } Callback;
 
 typedef struct
@@ -83,6 +81,8 @@ typedef struct
   Callback callback;
   DBusWatchFunction function;
   DBusWatch *watch;
+  void *data;
+  DBusFreeFunction free_data_func;
   /* last watch handle failed due to OOM */
   unsigned int last_iteration_oom : 1;
 } WatchCallback;
@@ -91,7 +91,6 @@ typedef struct
 {
   Callback callback;
   DBusTimeout *timeout;
-  DBusTimeoutFunction function;
   unsigned long last_tv_sec;
   unsigned long last_tv_usec;
 } TimeoutCallback;
@@ -116,17 +115,14 @@ watch_callback_new (DBusWatch         *watch,
   cb->last_iteration_oom = FALSE;
   cb->callback.refcount = 1;
   cb->callback.type = CALLBACK_WATCH;
-  cb->callback.data = data;
-  cb->callback.free_data_func = free_data_func;
+  cb->data = data;
+  cb->free_data_func = free_data_func;
   
   return cb;
 }
 
 static TimeoutCallback*
-timeout_callback_new (DBusTimeout         *timeout,
-                      DBusTimeoutFunction  function,
-                      void                *data,
-                      DBusFreeFunction     free_data_func)
+timeout_callback_new (DBusTimeout         *timeout)
 {
   TimeoutCallback *cb;
 
@@ -135,13 +131,10 @@ timeout_callback_new (DBusTimeout         *timeout,
     return NULL;
 
   cb->timeout = timeout;
-  cb->function = function;
   _dbus_get_current_time (&cb->last_tv_sec,
                           &cb->last_tv_usec);
   cb->callback.refcount = 1;    
   cb->callback.type = CALLBACK_TIMEOUT;
-  cb->callback.data = data;
-  cb->callback.free_data_func = free_data_func;
   
   return cb;
 }
@@ -165,8 +158,8 @@ callback_unref (Callback *cb)
 
   if (cb->refcount == 0)
     {
-      if (cb->free_data_func)
-        (* cb->free_data_func) (cb->data);
+      if (cb->type == CALLBACK_WATCH && WATCH_CALLBACK (cb)->free_data_func)
+        (WATCH_CALLBACK (cb)->free_data_func) (WATCH_CALLBACK (cb)->data);
       
       dbus_free (cb);
     }
@@ -275,7 +268,7 @@ _dbus_loop_add_watch (DBusLoop          *loop,
 
   if (!add_callback (loop, (Callback*) wcb))
     {
-      wcb->callback.free_data_func = NULL; /* don't want to have this side effect */
+      wcb->free_data_func = NULL; /* don't want to have this side effect */
       callback_unref ((Callback*) wcb);
       return FALSE;
     }
@@ -303,7 +296,7 @@ _dbus_loop_remove_watch (DBusLoop          *loop,
 
       if (this->type == CALLBACK_WATCH &&
           WATCH_CALLBACK (this)->watch == watch &&
-          this->data == data &&
+          WATCH_CALLBACK (this)->data == data &&
           WATCH_CALLBACK (this)->function == function)
         {
           remove_callback (loop, link);
@@ -319,21 +312,17 @@ _dbus_loop_remove_watch (DBusLoop          *loop,
 }
 
 dbus_bool_t
-_dbus_loop_add_timeout (DBusLoop            *loop,
-                        DBusTimeout        *timeout,
-                        DBusTimeoutFunction  function,
-                        void               *data,
-                        DBusFreeFunction    free_data_func)
+_dbus_loop_add_timeout (DBusLoop           *loop,
+                        DBusTimeout        *timeout)
 {
   TimeoutCallback *tcb;
 
-  tcb = timeout_callback_new (timeout, function, data, free_data_func);
+  tcb = timeout_callback_new (timeout);
   if (tcb == NULL)
     return FALSE;
 
   if (!add_callback (loop, (Callback*) tcb))
     {
-      tcb->callback.free_data_func = NULL; /* don't want to have this side effect */
       callback_unref ((Callback*) tcb);
       return FALSE;
     }
@@ -342,10 +331,8 @@ _dbus_loop_add_timeout (DBusLoop            *loop,
 }
 
 void
-_dbus_loop_remove_timeout (DBusLoop            *loop,
-                           DBusTimeout        *timeout,
-                           DBusTimeoutFunction  function,
-                           void               *data)
+_dbus_loop_remove_timeout (DBusLoop           *loop,
+                           DBusTimeout        *timeout)
 {
   DBusList *link;
   
@@ -356,9 +343,7 @@ _dbus_loop_remove_timeout (DBusLoop            *loop,
       Callback *this = link->data;
 
       if (this->type == CALLBACK_TIMEOUT &&
-          TIMEOUT_CALLBACK (this)->timeout == timeout &&
-          this->data == data &&
-          TIMEOUT_CALLBACK (this)->function == function)
+          TIMEOUT_CALLBACK (this)->timeout == timeout)
         {
           remove_callback (loop, link);
           
@@ -368,8 +353,7 @@ _dbus_loop_remove_timeout (DBusLoop            *loop,
       link = next;
     }
 
-  _dbus_warn ("could not find timeout %p function %p data %p to remove\n",
-              timeout, (void *)function, data);
+  _dbus_warn ("could not find timeout %p to remove\n", timeout);
 }
 
 /* Convolutions from GLib, there really must be a better way
@@ -613,7 +597,7 @@ _dbus_loop_iterate (DBusLoop     *loop,
               _dbus_warn ("watch %p was invalidated but not removed; "
                   "removing it now\n", wcb->watch);
               _dbus_loop_remove_watch (loop, wcb->watch, wcb->function,
-                  ((Callback *)wcb)->data);
+                  wcb->data);
             }
           else if (dbus_watch_get_enabled (wcb->watch))
             {
@@ -758,9 +742,11 @@ _dbus_loop_iterate (DBusLoop     *loop,
 #if MAINLOOP_SPEW
                   _dbus_verbose ("  invoking timeout\n");
 #endif
-                  
-                  (* tcb->function) (tcb->timeout,
-                                     cb->data);
+
+                  /* can theoretically return FALSE on OOM, but we just
+                   * let it fire again later - in practice that's what
+                   * every wrapper callback in dbus-daemon used to do */
+                  dbus_timeout_handle (tcb->timeout);
 
                   retval = TRUE;
                 }
@@ -821,9 +807,7 @@ _dbus_loop_iterate (DBusLoop     *loop,
               if (condition != 0 &&
                   dbus_watch_get_enabled (wcb->watch))
                 {
-                  if (!(* wcb->function) (wcb->watch,
-                                          condition,
-                                          ((Callback*)wcb)->data))
+                  if (!(* wcb->function) (wcb->watch, condition, wcb->data))
                     wcb->last_iteration_oom = TRUE;
 
 #if MAINLOOP_SPEW
@@ -841,7 +825,7 @@ _dbus_loop_iterate (DBusLoop     *loop,
                   _dbus_warn ("invalid request, socket fd %d not open\n",
                       fds[i].fd);
                   _dbus_loop_remove_watch (loop, watch, wcb->function,
-                      ((Callback *)wcb)->data);
+                      wcb->data);
                   _dbus_watch_invalidate (watch);
                   _dbus_watch_unref (watch);
                 }

diff --git a/dbus/dbus-mainloop.h b/dbus/dbus-mainloop.h
index 656f823..7d78c82 100644 (file)
--- a/dbus/dbus-mainloop.h
+++ b/dbus/dbus-mainloop.h
@@ -33,8 +33,6 @@ typedef struct DBusLoop DBusLoop;
 typedef dbus_bool_t (* DBusWatchFunction)   (DBusWatch     *watch,
                                              unsigned int   condition,
                                              void          *data);
-typedef void        (* DBusTimeoutFunction) (DBusTimeout   *timeout,
-                                             void          *data);
 
 DBusLoop*   _dbus_loop_new            (void);
 DBusLoop*   _dbus_loop_ref            (DBusLoop            *loop);
@@ -49,14 +47,9 @@ void        _dbus_loop_remove_watch   (DBusLoop            *loop,
                                        DBusWatchFunction    function,
                                        void                *data);
 dbus_bool_t _dbus_loop_add_timeout    (DBusLoop            *loop,
-                                       DBusTimeout         *timeout,
-                                       DBusTimeoutFunction  function,
-                                       void                *data,
-                                       DBusFreeFunction     free_data_func);
+                                       DBusTimeout         *timeout);
 void        _dbus_loop_remove_timeout (DBusLoop            *loop,
-                                       DBusTimeout         *timeout,
-                                       DBusTimeoutFunction  function,
-                                       void                *data);
+                                       DBusTimeout         *timeout);
 
 dbus_bool_t _dbus_loop_queue_dispatch (DBusLoop            *loop,
                                        DBusConnection      *connection);

diff --git a/test/test-utils.c b/test/test-utils.c
index 05cd753..f8e366b 100644 (file)
--- a/test/test-utils.c
+++ b/test/test-utils.c
@@ -38,22 +38,13 @@ remove_watch (DBusWatch *watch,
                            watch, connection_watch_callback, cd);  
 }
 
-static void
-connection_timeout_callback (DBusTimeout   *timeout,
-                             void          *data)
-{
-  /* Can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 static dbus_bool_t
 add_timeout (DBusTimeout *timeout,
             void        *data)
 {
   CData *cd = data;
 
-  return _dbus_loop_add_timeout (cd->loop,
-                                 timeout, connection_timeout_callback, cd, NULL);
+  return _dbus_loop_add_timeout (cd->loop, timeout);
 }
 
 static void
@@ -62,8 +53,7 @@ remove_timeout (DBusTimeout *timeout,
 {
   CData *cd = data;
 
-  _dbus_loop_remove_timeout (cd->loop,
-                             timeout, connection_timeout_callback, cd);
+  _dbus_loop_remove_timeout (cd->loop, timeout);
 }
 
 static void
@@ -259,22 +249,13 @@ remove_server_watch (DBusWatch  *watch,
                            watch, server_watch_callback, context);
 }
 
-static void
-server_timeout_callback (DBusTimeout   *timeout,
-                         void          *data)
-{
-  /* can return FALSE on OOM but we just let it fire again later */
-  dbus_timeout_handle (timeout);
-}
-
 static dbus_bool_t
 add_server_timeout (DBusTimeout *timeout,
                     void        *data)
 {
   ServerData *context = data;
 
-  return _dbus_loop_add_timeout (context->loop,
-                                 timeout, server_timeout_callback, context, NULL);
+  return _dbus_loop_add_timeout (context->loop, timeout);
 }
 
 static void
@@ -283,8 +264,7 @@ remove_server_timeout (DBusTimeout *timeout,
 {
   ServerData *context = data;
   
-  _dbus_loop_remove_timeout (context->loop,
-                             timeout, server_timeout_callback, context);
+  _dbus_loop_remove_timeout (context->loop, timeout);
 }
 
 dbus_bool_t