From 1af8f513460f2ad39321d47186720a59c9a49593 Mon Sep 17 00:00:00 2001 From: Simon McVittie 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