Attachment 42376 Details for Bug 33342 – [3/11] DBusLoop: remove second layer of watch callbacks where possible

[patch] [3/11] DBusLoop: remove second layer of watch callbacks where possible

DBusLoop-remove-second-layer-of-watch-callbacks-wh.patch (text/plain), 16.33 KB, created by Simon McVittie on 2011-01-24 07:51:10 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

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

Size: 16.33 KB

patch

obsolete

>From 36a91bfdb8f20a7602d2286c80aaa1870752bc95 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 21 Jan 2011 18:54:33 +0000
>Subject: [PATCH] DBusLoop: remove second layer of watch callbacks where possible
>
>Similar to the previous commit, almost every use of DBusWatch can just
>have the main loop call dbus_watch_handle.
>
>The one exception is the bus activation code; it's had a comment
>explaining why it's wrong since 2003. We should fix that one day, but for
>now, just migrate it to a new _dbus_loop_add_watch_full which preserves
>the second-layer callback.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33342
>---
> bus/activation.c        |    8 ++++----
> bus/bus.c               |   20 ++------------------
> bus/connection.c        |   23 ++---------------------
> bus/dir-watch-inotify.c |   11 ++---------
> bus/dir-watch-kqueue.c  |   11 ++---------
> bus/main.c              |   14 ++------------
> bus/test.c              |   20 ++------------------
> dbus/dbus-mainloop.c    |   45 ++++++++++++++++++++++++++-------------------
> dbus/dbus-mainloop.h    |    6 +++---
> test/test-utils.c       |   36 ++++--------------------------------
> 10 files changed, 49 insertions(+), 145 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index 348aca8..2349612 100644
>--- a/bus/activation.c
>+++ b/bus/activation.c
>@@ -1403,9 +1403,9 @@ add_babysitter_watch (DBusWatch      *watch,
> {
>   BusPendingActivation *pending_activation = data;
> 
>-  return _dbus_loop_add_watch (bus_context_get_loop (pending_activation->activation->context),
>-                               watch, babysitter_watch_callback, pending_activation,
>-                               NULL);
>+  return _dbus_loop_add_watch_full (
>+      bus_context_get_loop (pending_activation->activation->context),
>+      watch, babysitter_watch_callback, pending_activation, NULL);
> }
> 
> static void
>@@ -1415,7 +1415,7 @@ remove_babysitter_watch (DBusWatch      *watch,
>   BusPendingActivation *pending_activation = data;
> 
>   _dbus_loop_remove_watch (bus_context_get_loop (pending_activation->activation->context),
>-                           watch, babysitter_watch_callback, pending_activation);
>+                           watch);
> }
> 
> static dbus_bool_t
>diff --git a/bus/bus.c b/bus/bus.c
>index b512ab5..cebdb7c 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -99,19 +99,6 @@ server_get_context (DBusServer *server)
> }
> 
> static dbus_bool_t
>-server_watch_callback (DBusWatch     *watch,
>-                       unsigned int   condition,
>-                       void          *data)
>-{
>-  /* FIXME this can be done in dbus-mainloop.c
>-   * if the code in activation.c for the babysitter
>-   * watch handler is fixed.
>-   */
>-
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> add_server_watch (DBusWatch  *watch,
>                   void       *data)
> {
>@@ -120,9 +107,7 @@ add_server_watch (DBusWatch  *watch,
> 
>   context = server_get_context (server);
> 
>-  return _dbus_loop_add_watch (context->loop,
>-                               watch, server_watch_callback, server,
>-                               NULL);
>+  return _dbus_loop_add_watch (context->loop, watch);
> }
> 
> static void
>@@ -134,8 +119,7 @@ remove_server_watch (DBusWatch  *watch,
> 
>   context = server_get_context (server);
> 
>-  _dbus_loop_remove_watch (context->loop,
>-                           watch, server_watch_callback, server);
>+  _dbus_loop_remove_watch (context->loop, watch);
> }
> 
> static dbus_bool_t
>diff --git a/bus/connection.c b/bus/connection.c
>index 692948e..d74b3261 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -295,30 +295,12 @@ bus_connection_disconnected (DBusConnection *connection)
> }
> 
> static dbus_bool_t
>-connection_watch_callback (DBusWatch     *watch,
>-                           unsigned int   condition,
>-                           void          *data)
>-{
>- /* FIXME this can be done in dbus-mainloop.c
>-  * if the code in activation.c for the babysitter
>-  * watch handler is fixed.
>-  */
>-  
>-#if 0
>-  _dbus_verbose ("Calling handle_watch\n");
>-#endif
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> add_connection_watch (DBusWatch      *watch,
>                       void           *data)
> {
>   DBusConnection *connection = data;
> 
>-  return _dbus_loop_add_watch (connection_get_loop (connection),
>-                               watch, connection_watch_callback, connection,
>-                               NULL);
>+  return _dbus_loop_add_watch (connection_get_loop (connection), watch);
> }
> 
> static void
>@@ -327,8 +309,7 @@ remove_connection_watch (DBusWatch      *watch,
> {
>   DBusConnection *connection = data;
>   
>-  _dbus_loop_remove_watch (connection_get_loop (connection),
>-                           watch, connection_watch_callback, connection);
>+  _dbus_loop_remove_watch (connection_get_loop (connection), watch);
> }
> 
> static dbus_bool_t
>diff --git a/bus/dir-watch-inotify.c b/bus/dir-watch-inotify.c
>index 54704a4..2e9be98 100644
>--- a/bus/dir-watch-inotify.c
>+++ b/bus/dir-watch-inotify.c
>@@ -50,12 +50,6 @@ static DBusWatch *watch = NULL;
> static DBusLoop *loop = NULL;
> 
> static dbus_bool_t
>-_inotify_watch_callback (DBusWatch *watch, unsigned int condition, void *data)
>-{
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> _handle_inotify_watch (DBusWatch *passed_watch, unsigned int flags, void *data)
> {
>   char buffer[INOTIFY_BUF_LEN];
>@@ -206,7 +200,7 @@ _shutdown_inotify (void *data)
> 
>   if (watch != NULL)
>     {
>-      _dbus_loop_remove_watch (loop, watch, _inotify_watch_callback, NULL);
>+      _dbus_loop_remove_watch (loop, watch);
>       _dbus_watch_invalidate (watch);
>       _dbus_watch_unref (watch);
>       _dbus_loop_unref (loop);
>@@ -252,8 +246,7 @@ _init_inotify (BusContext *context)
>           goto out;
>         }
> 
>-      if (!_dbus_loop_add_watch (loop, watch, _inotify_watch_callback,
>-                                 NULL, NULL))
>+      if (!_dbus_loop_add_watch (loop, watch))
>         {
>           _dbus_warn ("Unable to add reload watch to main loop");
>           _dbus_watch_unref (watch);
>diff --git a/bus/dir-watch-kqueue.c b/bus/dir-watch-kqueue.c
>index 62f7b3d..ac6290c 100644
>--- a/bus/dir-watch-kqueue.c
>+++ b/bus/dir-watch-kqueue.c
>@@ -50,12 +50,6 @@ static DBusWatch *watch = NULL;
> static DBusLoop *loop = NULL;
> 
> static dbus_bool_t
>-_kqueue_watch_callback (DBusWatch *watch, unsigned int condition, void *data)
>-{
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> _handle_kqueue_watch (DBusWatch *watch, unsigned int flags, void *data)
> {
>   struct kevent ev;
>@@ -80,7 +74,7 @@ _handle_kqueue_watch (DBusWatch *watch, unsigned int flags, void *data)
>       kq = -1;
>       if (watch != NULL)
> 	{
>-	  _dbus_loop_remove_watch (loop, watch, _kqueue_watch_callback, NULL);
>+	  _dbus_loop_remove_watch (loop, watch);
>           _dbus_watch_invalidate (watch);
>           _dbus_watch_unref (watch);
> 	  watch = NULL;
>@@ -121,8 +115,7 @@ _init_kqueue (BusContext *context)
> 	    goto out;
> 	  }
> 
>-	if (!_dbus_loop_add_watch (loop, watch, _kqueue_watch_callback,
>-                                   NULL, NULL))
>+	if (!_dbus_loop_add_watch (loop, watch))
>           {
>             _dbus_warn ("Unable to add reload watch to main loop");
> 	    _dbus_watch_invalidate (watch);
>diff --git a/bus/main.c b/bus/main.c
>index 1af588e..2a03266 100644
>--- a/bus/main.c
>+++ b/bus/main.c
>@@ -201,14 +201,6 @@ handle_reload_watch (DBusWatch    *watch,
>   return TRUE;
> }
> 
>-static dbus_bool_t
>-reload_watch_callback (DBusWatch    *watch,
>-		       unsigned int  condition,
>-		       void         *data)
>-{
>-  return dbus_watch_handle (watch, condition);
>-}
>-
> static void
> setup_reload_pipe (DBusLoop *loop)
> {
>@@ -238,8 +230,7 @@ setup_reload_pipe (DBusLoop *loop)
>       exit (1);
>     }
> 
>-  if (!_dbus_loop_add_watch (loop, watch, reload_watch_callback,
>-			     NULL, NULL))
>+  if (!_dbus_loop_add_watch (loop, watch))
>     {
>       _dbus_warn ("Unable to add reload watch to main loop: %s\n",
> 		  error.message);
>@@ -252,8 +243,7 @@ setup_reload_pipe (DBusLoop *loop)
> static void
> close_reload_pipe (DBusWatch **watch)
> {
>-    _dbus_loop_remove_watch (bus_context_get_loop (context),
>-        *watch, reload_watch_callback, NULL);
>+    _dbus_loop_remove_watch (bus_context_get_loop (context), *watch);
>     _dbus_watch_invalidate (*watch);
>     _dbus_watch_unref (*watch);
>     *watch = NULL;
>diff --git a/bus/test.c b/bus/test.c
>index 8d16340..049fae6 100644
>--- a/bus/test.c
>+++ b/bus/test.c
>@@ -37,27 +37,12 @@ static DBusList *clients = NULL;
> static DBusLoop *client_loop = NULL;
> 
> static dbus_bool_t
>-client_watch_callback (DBusWatch     *watch,
>-                       unsigned int   condition,
>-                       void          *data)
>-{
>-  /* FIXME this can be done in dbus-mainloop.c
>-   * if the code in activation.c for the babysitter
>-   * watch handler is fixed.
>-   */
>-
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> add_client_watch (DBusWatch      *watch,
>                   void           *data)
> {
>   DBusConnection *connection = data;
> 
>-  return _dbus_loop_add_watch (client_loop,
>-                               watch, client_watch_callback, connection,
>-                               NULL);
>+  return _dbus_loop_add_watch (client_loop, watch);
> }
> 
> static void
>@@ -66,8 +51,7 @@ remove_client_watch (DBusWatch      *watch,
> {
>   DBusConnection *connection = data;
> 
>-  _dbus_loop_remove_watch (client_loop,
>-                           watch, client_watch_callback, connection);
>+  _dbus_loop_remove_watch (client_loop, watch);
> }
> 
> static dbus_bool_t
>diff --git a/dbus/dbus-mainloop.c b/dbus/dbus-mainloop.c
>index 087bb5b..436cfee 100644
>--- a/dbus/dbus-mainloop.c
>+++ b/dbus/dbus-mainloop.c
>@@ -254,11 +254,18 @@ _dbus_loop_unref (DBusLoop *loop)
> }
> 
> dbus_bool_t
>-_dbus_loop_add_watch (DBusLoop          *loop,
>-                      DBusWatch        *watch,
>-                      DBusWatchFunction  function,
>-                      void             *data,
>-                      DBusFreeFunction  free_data_func)
>+_dbus_loop_add_watch (DBusLoop  *loop,
>+                      DBusWatch *watch)
>+{
>+  return _dbus_loop_add_watch_full (loop, watch, NULL, NULL, NULL);
>+}
>+
>+dbus_bool_t
>+_dbus_loop_add_watch_full (DBusLoop          *loop,
>+                           DBusWatch         *watch,
>+                           DBusWatchFunction  function,
>+                           void              *data,
>+                           DBusFreeFunction   free_data_func)
> {
>   WatchCallback *wcb;
> 
>@@ -277,10 +284,8 @@ _dbus_loop_add_watch (DBusLoop          *loop,
> }
> 
> void
>-_dbus_loop_remove_watch (DBusLoop          *loop,
>-                         DBusWatch        *watch,
>-                         DBusWatchFunction  function,
>-                         void             *data)
>+_dbus_loop_remove_watch (DBusLoop         *loop,
>+                         DBusWatch        *watch)
> {
>   DBusList *link;
> 
>@@ -295,9 +300,7 @@ _dbus_loop_remove_watch (DBusLoop          *loop,
>       Callback *this = link->data;
> 
>       if (this->type == CALLBACK_WATCH &&
>-          WATCH_CALLBACK (this)->watch == watch &&
>-          WATCH_CALLBACK (this)->data == data &&
>-          WATCH_CALLBACK (this)->function == function)
>+          WATCH_CALLBACK (this)->watch == watch)
>         {
>           remove_callback (loop, link);
>           
>@@ -307,8 +310,7 @@ _dbus_loop_remove_watch (DBusLoop          *loop,
>       link = next;
>     }
> 
>-  _dbus_warn ("could not find watch %p function %p data %p to remove\n",
>-              watch, (void *)function, data);
>+  _dbus_warn ("could not find watch %p to remove\n", watch);
> }
> 
> dbus_bool_t
>@@ -596,8 +598,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,
>-                  wcb->data);
>+              _dbus_loop_remove_watch (loop, wcb->watch);
>             }
>           else if (dbus_watch_get_enabled (wcb->watch))
>             {
>@@ -807,7 +808,14 @@ _dbus_loop_iterate (DBusLoop     *loop,
>               if (condition != 0 &&
>                   dbus_watch_get_enabled (wcb->watch))
>                 {
>-                  if (!(* wcb->function) (wcb->watch, condition, wcb->data))
>+                  dbus_bool_t oom;
>+
>+                  if (wcb->function)
>+                    oom = !(* wcb->function) (wcb->watch, condition, wcb->data);
>+                  else
>+                    oom = !dbus_watch_handle (wcb->watch, condition);
>+
>+                  if (oom)
>                     wcb->last_iteration_oom = TRUE;
> 
> #if MAINLOOP_SPEW
>@@ -824,8 +832,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,
>-                      wcb->data);
>+                  _dbus_loop_remove_watch (loop, watch);
>                   _dbus_watch_invalidate (watch);
>                   _dbus_watch_unref (watch);
>                 }
>diff --git a/dbus/dbus-mainloop.h b/dbus/dbus-mainloop.h
>index 7d78c82..5be955b 100644
>--- a/dbus/dbus-mainloop.h
>+++ b/dbus/dbus-mainloop.h
>@@ -38,14 +38,14 @@ DBusLoop*   _dbus_loop_new            (void);
> DBusLoop*   _dbus_loop_ref            (DBusLoop            *loop);
> void        _dbus_loop_unref          (DBusLoop            *loop);
> dbus_bool_t _dbus_loop_add_watch      (DBusLoop            *loop,
>+                                       DBusWatch           *watch);
>+dbus_bool_t _dbus_loop_add_watch_full (DBusLoop            *loop,
>                                        DBusWatch           *watch,
>                                        DBusWatchFunction    function,
>                                        void                *data,
>                                        DBusFreeFunction     free_data_func);
> void        _dbus_loop_remove_watch   (DBusLoop            *loop,
>-                                       DBusWatch           *watch,
>-                                       DBusWatchFunction    function,
>-                                       void                *data);
>+                                       DBusWatch           *watch);
> dbus_bool_t _dbus_loop_add_timeout    (DBusLoop            *loop,
>                                        DBusTimeout         *timeout);
> void        _dbus_loop_remove_timeout (DBusLoop            *loop,
>diff --git a/test/test-utils.c b/test/test-utils.c
>index 0811735..81aac9e 100644
>--- a/test/test-utils.c
>+++ b/test/test-utils.c
>@@ -9,23 +9,12 @@ typedef struct
> } CData;
> 
> static dbus_bool_t
>-connection_watch_callback (DBusWatch     *watch,
>-                           unsigned int   condition,
>-                           void          *data)
>-{
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> add_watch (DBusWatch *watch,
> 	   void      *data)
> {
>   CData *cd = data;
> 
>-  return _dbus_loop_add_watch (cd->loop,
>-                               watch,
>-                               connection_watch_callback,
>-                               cd, NULL);
>+  return _dbus_loop_add_watch (cd->loop, watch);
> }
> 
> static void
>@@ -34,8 +23,7 @@ remove_watch (DBusWatch *watch,
> {
>   CData *cd = data;
>   
>-  _dbus_loop_remove_watch (cd->loop,
>-                           watch, connection_watch_callback, cd);  
>+  _dbus_loop_remove_watch (cd->loop, watch);
> }
> 
> static dbus_bool_t
>@@ -216,27 +204,12 @@ serverdata_new (DBusLoop       *loop,
> }
> 
> static dbus_bool_t
>-server_watch_callback (DBusWatch     *watch,
>-                       unsigned int   condition,
>-                       void          *data)
>-{
>-  /* FIXME this can be done in dbus-mainloop.c
>-   * if the code in activation.c for the babysitter
>-   * watch handler is fixed.
>-   */
>-
>-  return dbus_watch_handle (watch, condition);
>-}
>-
>-static dbus_bool_t
> add_server_watch (DBusWatch  *watch,
>                   void       *data)
> {
>   ServerData *context = data;
> 
>-  return _dbus_loop_add_watch (context->loop,
>-                               watch, server_watch_callback, context,
>-                               NULL);
>+  return _dbus_loop_add_watch (context->loop, watch);
> }
> 
> static void
>@@ -245,8 +218,7 @@ remove_server_watch (DBusWatch  *watch,
> {
>   ServerData *context = data;
>   
>-  _dbus_loop_remove_watch (context->loop,
>-                           watch, server_watch_callback, context);
>+  _dbus_loop_remove_watch (context->loop, watch);
> }
> 
> static 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.