diff options
| -rw-r--r-- | ChangeLog | 19 | ||||
| -rw-r--r-- | dbus/dbus-server-protected.h | 1 | ||||
| -rw-r--r-- | dbus/dbus-server.c | 191 | ||||
| -rw-r--r-- | doc/TODO | 5 |
4 files changed, 196 insertions, 20 deletions
@@ -1,3 +1,22 @@ +2005-02-20 Havoc Pennington <hp@redhat.com> + + Fix bugs reported by Daniel P. Berrange + + * dbus/dbus-server.c (_dbus_server_unref_unlocked): new function + (protected_change_watch): new function + (_dbus_server_toggle_watch, _dbus_server_remove_watch) + (_dbus_server_add_watch): change to work like the + dbus-connection.c equivalents; like those, probably kind of + busted, but should at least mostly work for now + (dbus_server_disconnect): drop the lock if we were already + disconnected, patch from Daniel P. Berrange + + * dbus/dbus-server.c (_dbus_server_toggle_timeout) + (_dbus_server_remove_timeout, _dbus_server_add_timeout): all the + same stuff + + * doc/TODO: todo about unscrewing this mess + 2005-02-19 Colin Walters <walters@verbum.org> * glib/dbus-binding-tool-glib.c diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h index c8aa8601..5862c6c2 100644 --- a/dbus/dbus-server-protected.h +++ b/dbus/dbus-server-protected.h @@ -102,6 +102,7 @@ void _dbus_server_toggle_timeout (DBusServer *server, dbus_bool_t enabled); void _dbus_server_ref_unlocked (DBusServer *server); +void _dbus_server_unref_unlocked (DBusServer *server); #ifdef DBUS_DISABLE_CHECKS #define TOOK_LOCK_CHECK(server) diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c index 788aaad7..c11fbd48 100644 --- a/dbus/dbus-server.c +++ b/dbus/dbus-server.c @@ -1,7 +1,7 @@ /* -*- mode: C; c-file-style: "gnu" -*- */ /* dbus-server.c DBusServer object * - * Copyright (C) 2002, 2003, 2004 Red Hat Inc. + * Copyright (C) 2002, 2003, 2004, 2005 Red Hat Inc. * * Licensed under the Academic Free License version 2.1 * @@ -146,6 +146,62 @@ _dbus_server_finalize_base (DBusServer *server) dbus_free_string_array (server->auth_mechanisms); } + +typedef dbus_bool_t (* DBusWatchAddFunction) (DBusWatchList *list, + DBusWatch *watch); +typedef void (* DBusWatchRemoveFunction) (DBusWatchList *list, + DBusWatch *watch); +typedef void (* DBusWatchToggleFunction) (DBusWatchList *list, + DBusWatch *watch, + dbus_bool_t enabled); + +static dbus_bool_t +protected_change_watch (DBusServer *server, + DBusWatch *watch, + DBusWatchAddFunction add_function, + DBusWatchRemoveFunction remove_function, + DBusWatchToggleFunction toggle_function, + dbus_bool_t enabled) +{ + DBusWatchList *watches; + dbus_bool_t retval; + + HAVE_LOCK_CHECK (server); + + /* This isn't really safe or reasonable; a better pattern is the "do everything, then + * drop lock and call out" one; but it has to be propagated up through all callers + */ + + watches = server->watches; + if (watches) + { + server->watches = NULL; + _dbus_server_ref_unlocked (server); + SERVER_UNLOCK (server); + + if (add_function) + retval = (* add_function) (watches, watch); + else if (remove_function) + { + retval = TRUE; + (* remove_function) (watches, watch); + } + else + { + retval = TRUE; + (* toggle_function) (watches, watch, enabled); + } + + SERVER_LOCK (server); + server->watches = watches; + _dbus_server_unref_unlocked (server); + + return retval; + } + else + return FALSE; +} + /** * Adds a watch for this server, chaining out to application-provided * watch handlers. @@ -157,8 +213,9 @@ dbus_bool_t _dbus_server_add_watch (DBusServer *server, DBusWatch *watch) { - HAVE_LOCK_CHECK (server); - return _dbus_watch_list_add_watch (server->watches, watch); + return protected_change_watch (server, watch, + _dbus_watch_list_add_watch, + NULL, NULL, FALSE); } /** @@ -171,8 +228,10 @@ void _dbus_server_remove_watch (DBusServer *server, DBusWatch *watch) { - HAVE_LOCK_CHECK (server); - _dbus_watch_list_remove_watch (server->watches, watch); + protected_change_watch (server, watch, + NULL, + _dbus_watch_list_remove_watch, + NULL, FALSE); } /** @@ -189,11 +248,69 @@ _dbus_server_toggle_watch (DBusServer *server, DBusWatch *watch, dbus_bool_t enabled) { + _dbus_assert (watch != NULL); + + protected_change_watch (server, watch, + NULL, NULL, + _dbus_watch_list_toggle_watch, + enabled); +} + + +typedef dbus_bool_t (* DBusTimeoutAddFunction) (DBusTimeoutList *list, + DBusTimeout *timeout); +typedef void (* DBusTimeoutRemoveFunction) (DBusTimeoutList *list, + DBusTimeout *timeout); +typedef void (* DBusTimeoutToggleFunction) (DBusTimeoutList *list, + DBusTimeout *timeout, + dbus_bool_t enabled); + + +static dbus_bool_t +protected_change_timeout (DBusServer *server, + DBusTimeout *timeout, + DBusTimeoutAddFunction add_function, + DBusTimeoutRemoveFunction remove_function, + DBusTimeoutToggleFunction toggle_function, + dbus_bool_t enabled) +{ + DBusTimeoutList *timeouts; + dbus_bool_t retval; + HAVE_LOCK_CHECK (server); + + /* This isn't really safe or reasonable; a better pattern is the "do everything, then + * drop lock and call out" one; but it has to be propagated up through all callers + */ - if (server->watches) /* null during finalize */ - _dbus_watch_list_toggle_watch (server->watches, - watch, enabled); + timeouts = server->timeouts; + if (timeouts) + { + server->timeouts = NULL; + _dbus_server_ref_unlocked (server); + SERVER_UNLOCK (server); + + if (add_function) + retval = (* add_function) (timeouts, timeout); + else if (remove_function) + { + retval = TRUE; + (* remove_function) (timeouts, timeout); + } + else + { + retval = TRUE; + (* toggle_function) (timeouts, timeout, enabled); + } + + SERVER_LOCK (server); + server->timeouts = timeouts; + _dbus_server_unref_unlocked (server); + + return retval; + } + else + return FALSE; } /** @@ -209,9 +326,9 @@ dbus_bool_t _dbus_server_add_timeout (DBusServer *server, DBusTimeout *timeout) { - HAVE_LOCK_CHECK (server); - - return _dbus_timeout_list_add_timeout (server->timeouts, timeout); + return protected_change_timeout (server, timeout, + _dbus_timeout_list_add_timeout, + NULL, NULL, FALSE); } /** @@ -224,9 +341,10 @@ void _dbus_server_remove_timeout (DBusServer *server, DBusTimeout *timeout) { - HAVE_LOCK_CHECK (server); - - _dbus_timeout_list_remove_timeout (server->timeouts, timeout); + protected_change_timeout (server, timeout, + NULL, + _dbus_timeout_list_remove_timeout, + NULL, FALSE); } /** @@ -243,11 +361,10 @@ _dbus_server_toggle_timeout (DBusServer *server, DBusTimeout *timeout, dbus_bool_t enabled) { - HAVE_LOCK_CHECK (server); - - if (server->timeouts) /* null during finalize */ - _dbus_timeout_list_toggle_timeout (server->timeouts, - timeout, enabled); + protected_change_timeout (server, timeout, + NULL, NULL, + _dbus_timeout_list_toggle_timeout, + enabled); } @@ -548,6 +665,37 @@ _dbus_server_ref_unlocked (DBusServer *server) } /** + * Like dbus_server_unref() but does not acquire the lock (must already be held) + * + * @param server the server. + */ +void +_dbus_server_unref_unlocked (DBusServer *server) +{ + dbus_bool_t last_unref; + + _dbus_assert (server != NULL); + + HAVE_LOCK_CHECK (server); + +#ifdef DBUS_HAVE_ATOMIC_INT + last_unref = (_dbus_atomic_dec (&server->refcount) == 1); +#else + _dbus_assert (server->refcount.value > 0); + + server->refcount.value -= 1; + last_unref = (server->refcount.value == 0); +#endif + + if (last_unref) + { + _dbus_assert (server->vtable->finalize != NULL); + + (* server->vtable->finalize) (server); + } +} + +/** * Releases the server's address and stops listening for * new clients. If called more than once, only the first * call has an effect. Does not modify the server's @@ -565,7 +713,10 @@ dbus_server_disconnect (DBusServer *server) _dbus_assert (server->vtable->disconnect != NULL); if (server->disconnected) - return; + { + SERVER_UNLOCK (server); + return; + } (* server->vtable->disconnect) (server); server->disconnected = TRUE; @@ -76,6 +76,11 @@ Might as Well for 1.0 Can Be Post 1.0 === + - DBusWatchList/TimeoutList duplicate a lot of code, as do + protected_change_watch/protected_change_timeout in dbus-connection.c + and dbus-server.c. This could all be mopped up, cut-and-paste + fixed, code size reduced. + - change .service files to allow Names=list in addition to Name=string - The message bus internal code still says "service" for |