Attachment 42375 Details for Bug 33342 – [2/11] DBusLoop: remove a layer of pointless abstraction around timeouts

[patch] [2/11] DBusLoop: remove a layer of pointless abstraction around timeouts

DBusLoop-remove-a-layer-of-pointless-abstraction-a.patch (text/plain), 19.01 KB, created by Simon McVittie on 2011-01-24 07:51:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-01-24 07:51:07 UTC

Size: 19.01 KB

patch

obsolete

>From 1af8f513460f2ad39321d47186720a59c9a49593 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 21 Jan 2011 18:54:09 +0000
>Subject: [PATCH] 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
>---
> bus/activation.c     |   18 +------------
> bus/bus.c            |   15 +----------
> bus/connection.c     |   22 +++--------------
> bus/expirelist.c     |   15 +----------
> bus/test.c           |   14 +---------
> dbus/dbus-mainloop.c |   64 ++++++++++++++++++-------------------------------
> dbus/dbus-mainloop.h |   11 +-------
> test/test-utils.c    |   28 +++------------------
> 8 files changed, 42 insertions(+), 145 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index ee5efa8..348aca8 100644
>--- 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;
>     }
> 
>@@ -1832,10 +1821,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 f892e26..b512ab5 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -138,15 +138,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)
>@@ -156,8 +147,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
>@@ -169,8 +159,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
>--- 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
>--- 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
>--- 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
>--- 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
>--- 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 67e207c..0811735 100644
>--- 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
>-- 
>1.7.2.3

Actions: View | Splinter Review

Attachments on bug 33342: 42374 | 42375 | 42376 | 42377 | 42378 | 42379 | 42380 | 42381 | 42382 | 42383 | 42384 | 42450 | 42451 | 42452 | 42453

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.