summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--ChangeLog19
-rw-r--r--dbus/dbus-server-protected.h1
-rw-r--r--dbus/dbus-server.c191
-rw-r--r--doc/TODO5
4 files changed, 196 insertions, 20 deletions
diff --git a/ChangeLog b/ChangeLog
index fe3e7763..7a93648c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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;
diff --git a/doc/TODO b/doc/TODO
index 4b69b623..d58bc3ab 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -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