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 3177d023773e064bd6d37d073ba51226f2b154d7..31dbaf546346c457d972791545048bfd662e2e89 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 6b0dc088ec04c870cef42f86e196dc8c6da9e71d..2008d1de10ff5563532f5066de23d2f0679cfb58 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 8e7d222a71abaa79a603c51eb764464fa5108dd2..692948e0e43fecf80c4cbe97eebbdc5c6baef2e0 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 946a615c40bd07a29c510bdf318d5deb42cf94ec..3c87c1198aeb8f8dd5aeab2c83842a7917725a05 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 95c7cfb89dde518621a43ac9f5554d01005a0ac6..8d16340ed830ca8793c4cddb91613658fcc44597 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 a25b31d6059564fcbd4df4cf88be01a96241bfd9..087bb5bc309e2743aa04e5759cf5db0bdef9c088 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 656f82313476013270cc1285b444cf27d7e844c5..7d78c82e96982a515421f70043e399ac25faeef4 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 05cd753528c8561e4cadbf42c084d5b0a18fee17..f8e366b71071542fcf4c20cacb1241fb1f05f03c 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