From eaefe03a8891b84e3f9e1f99f9098d65567e3092 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sat, 28 Oct 2006 01:41:37 +0000 Subject: 2006-10-27 Havoc Pennington * dbus/dbus-test.c: enclose more of the file in the DBUS_BUILD_TESTS check. * dbus/dbus-sysdeps-pthread.c (PTHREAD_CHECK): fix for DBUS_DISABLE_ASSERT case. * dbus/dbus-connection.c (dbus_connection_get_unix_user): document that it only works on the server side * dbus/dbus-bus.c: add a global lock covering the BusData we attach to each connection (internal_bus_get): lock our access to the BusData (dbus_bus_register): lock the entire registration process with _DBUS_LOCK(bus_datas). If we get the lock and registration is already complete, silently return (vs. previous behavior of aborting). (dbus_bus_set_unique_name): lock the BusData (dbus_bus_get_unique_name): lock the BusData --- ChangeLog | 21 +++++++ dbus/dbus-bus.c | 139 ++++++++++++++++++++++++++++++++++++-------- dbus/dbus-connection.c | 19 ++++-- dbus/dbus-internals.h | 6 +- dbus/dbus-sysdeps-pthread.c | 5 +- dbus/dbus-test.c | 5 +- dbus/dbus-threads.c | 1 + 7 files changed, 163 insertions(+), 33 deletions(-) diff --git a/ChangeLog b/ChangeLog index b9b2ac65..f0d4d89f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,24 @@ +2006-10-27 Havoc Pennington + + * dbus/dbus-test.c: enclose more of the file in the + DBUS_BUILD_TESTS check. + + * dbus/dbus-sysdeps-pthread.c (PTHREAD_CHECK): fix for + DBUS_DISABLE_ASSERT case. + + * dbus/dbus-connection.c (dbus_connection_get_unix_user): document + that it only works on the server side + + * dbus/dbus-bus.c: add a global lock covering the BusData we + attach to each connection + (internal_bus_get): lock our access to the BusData + (dbus_bus_register): lock the entire registration process + with _DBUS_LOCK(bus_datas). If we get the lock and + registration is already complete, silently return (vs. previous + behavior of aborting). + (dbus_bus_set_unique_name): lock the BusData + (dbus_bus_get_unique_name): lock the BusData + 2006-10-27 John (J5) Palmieri * bus/config-parser.c (service_dirs_find_dir, diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c index b2072b88..23428268 100644 --- a/dbus/dbus-bus.c +++ b/dbus/dbus-bus.c @@ -99,6 +99,14 @@ static dbus_bool_t initialized = FALSE; */ _DBUS_DEFINE_GLOBAL_LOCK (bus); +/** + * Global lock covering all BusData on any connection. The bet is + * that some lock contention is better than more memory + * for a per-connection lock, but it's tough to imagine it mattering + * either way. + */ +_DBUS_DEFINE_GLOBAL_LOCK (bus_datas); + static void addresses_shutdown_func (void *data) { @@ -452,12 +460,15 @@ internal_bus_get (DBusBusType type, */ bus_connections[type] = connection; } - - bd = ensure_bus_data (connection); - _dbus_assert (bd != NULL); + _DBUS_LOCK (bus_datas); + bd = ensure_bus_data (connection); + _dbus_assert (bd != NULL); /* it should have been created on + register, so OOM not possible */ bd->is_well_known = TRUE; + _DBUS_UNLOCK (bus_datas); + _DBUS_UNLOCK (bus); /* Return a reference to the caller */ @@ -542,15 +553,44 @@ dbus_bus_get_private (DBusBusType type, * If registration succeeds, the unique name will be set, * and can be obtained using dbus_bus_get_unique_name(). * + * This function will block until registration is complete. + * + * If the connection has already registered with the bus + * (determined by checking whether dbus_bus_get_unique_name() + * returns a non-#NULL value), then this function does nothing. + * * If you use dbus_bus_get() or dbus_bus_get_private() this * function will be called for you. + * + * @note Just use dbus_bus_get() or dbus_bus_get_private() instead of + * dbus_bus_register() and save yourself some pain. Using + * dbus_bus_register() manually is only useful if you have your + * own custom message bus not found in #DBusBusType. * * If you open a bus connection with dbus_connection_open() or * dbus_connection_open_private() you will have to dbus_bus_register() * yourself, or make the appropriate registration method calls - * yourself. + * yourself. If you send the method calls yourself, call + * dbus_bus_set_unique_name() with the unique bus name you get from + * the bus. * - * This function will block until registration is complete. + * For shared connections (created with dbus_connection_open()) in a + * multithreaded application, you can't really make the registration + * calls yourself, because you don't know whether some other thread is + * also registering, and the bus will kick you off if you send two + * registration messages. + * + * If you use dbus_bus_register() however, there is a lock that + * keeps both apps from registering at the same time. + * + * The rule in a multithreaded app, then, is that dbus_bus_register() + * must be used to register, or you need to have your own locks that + * all threads in the app will respect. + * + * In a single-threaded application you can register by hand instead + * of using dbus_bus_register(), as long as you check + * dbus_bus_get_unique_name() to see if a unique name has already been + * stored by another thread before you send the registration messages. * * @param connection the connection * @param error place to store errors @@ -569,19 +609,24 @@ dbus_bus_register (DBusConnection *connection, _dbus_return_val_if_error_is_set (error, FALSE); retval = FALSE; - + + _DBUS_LOCK (bus_datas); + bd = ensure_bus_data (connection); if (bd == NULL) { _DBUS_SET_OOM (error); + _DBUS_UNLOCK (bus_datas); return FALSE; } if (bd->unique_name != NULL) { - _dbus_warn_check_failed ("Attempt to register the same DBusConnection %s with the message bus a second time.\n", - bd->unique_name); - /* This isn't an error, it's a programming bug. so return TRUE */ + _dbus_verbose ("Ignoring attempt to register the same DBusConnection %s with the message bus a second time.\n", + bd->unique_name); + _DBUS_UNLOCK (bus_datas); + + /* Success! */ return TRUE; } @@ -593,6 +638,8 @@ dbus_bus_register (DBusConnection *connection, if (!message) { _DBUS_SET_OOM (error); + + _DBUS_UNLOCK (bus_datas); return FALSE; } @@ -624,6 +671,8 @@ dbus_bus_register (DBusConnection *connection, if (!retval) _DBUS_ASSERT_ERROR_IS_SET (error); + + _DBUS_UNLOCK (bus_datas); return retval; } @@ -636,6 +685,29 @@ dbus_bus_register (DBusConnection *connection, * once per connection. After the unique name is set, you can get it * with dbus_bus_get_unique_name(). * + * The only reason to use this function is to re-implement the + * equivalent of dbus_bus_register() yourself. One (probably unusual) + * reason to do that might be to do the bus registration call + * asynchronously instead of synchronously. + * + * @note Just use dbus_bus_get() or dbus_bus_get_private(), or worst + * case dbus_bus_register(), instead of messing with this + * function. There's really no point creating pain for yourself by + * doing things manually. + * + * It's hard to use this function safely on shared connections + * (created by dbus_connection_open()) in a multithreaded application, + * because only one registration attempt can be sent to the bus. If + * two threads are both sending the registration message, there is no + * mechanism in libdbus itself to avoid sending it twice. + * + * Thus, you need a way to coordinate which thread sends the + * registration attempt; which also means you know which thread + * will call dbus_bus_set_unique_name(). If you don't know + * about all threads in the app (for example, if some libraries + * you're using might start libdbus-using threads), then you + * need to avoid using this function on shared connections. + * * @param connection the connection * @param unique_name the unique name * @returns #FALSE if not enough memory @@ -645,9 +717,12 @@ dbus_bus_set_unique_name (DBusConnection *connection, const char *unique_name) { BusData *bd; + dbus_bool_t success; _dbus_return_val_if_fail (connection != NULL, FALSE); _dbus_return_val_if_fail (unique_name != NULL, FALSE); + + _DBUS_LOCK (bus_datas); bd = ensure_bus_data (connection); if (bd == NULL) @@ -656,51 +731,69 @@ dbus_bus_set_unique_name (DBusConnection *connection, _dbus_assert (bd->unique_name == NULL); bd->unique_name = _dbus_strdup (unique_name); - return bd->unique_name != NULL; + success = bd->unique_name != NULL; + + _DBUS_UNLOCK (bus_datas); + + return success; } /** * Gets the unique name of the connection as assigned by the message * bus. Only possible after the connection has been registered with - * the message bus. + * the message bus. All connections returned by dbus_bus_get() or + * dbus_bus_get_private() have been successfully registered. * * The name remains valid until the connection is freed, and * should not be freed by the caller. * - * There are two ways to set the unique name; one is - * dbus_bus_register(), the other is dbus_bus_set_unique_name(). - * You are responsible for calling dbus_bus_set_unique_name() - * if you register by hand instead of using dbus_bus_register(). + * Other than dbus_bus_get(), there are two ways to set the unique + * name; one is dbus_bus_register(), the other is + * dbus_bus_set_unique_name(). You are responsible for calling + * dbus_bus_set_unique_name() if you register by hand instead of using + * dbus_bus_register(). * * @param connection the connection - * @returns the unique name or NULL on error + * @returns the unique name or #NULL on error */ const char* dbus_bus_get_unique_name (DBusConnection *connection) { BusData *bd; + const char *unique_name; _dbus_return_val_if_fail (connection != NULL, NULL); + + _DBUS_LOCK (bus_datas); bd = ensure_bus_data (connection); if (bd == NULL) return NULL; + + unique_name = bd->unique_name; + + _DBUS_UNLOCK (bus_datas); - return bd->unique_name; + return unique_name; } /** - * Asks the bus to return the uid of the named connection. - * Only works on UNIX; only works for connections on the same - * machine as the bus. If you are not on the same machine - * as the bus, then calling this is probably a bad idea, - * since the uid will mean little to your application. + * Asks the bus to return the UID the named connection authenticated + * as, if any. Only works on UNIX; only works for connections on the + * same machine as the bus. If you are not on the same machine as the + * bus, then calling this is probably a bad idea, since the UID will + * mean little to your application. * * For the system message bus you're guaranteed to be on the same * machine since it only listens on a UNIX domain socket (at least, * as shipped by default). * - * This function will just return an error on Windows. + * This function only works for connections that authenticated as + * a UNIX user, right now that includes all bus connections, but + * it's very possible to have connections with no associated UID. + * So check for errors and do something sensible if they happen. + * + * This function will always return an error on Windows. * * @param connection the connection * @param name a name owned by the connection diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index e2debdd0..db625588 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -4752,11 +4752,20 @@ dbus_connection_get_socket(DBusConnection *connection, /** - * Gets the UNIX user ID of the connection if any. - * Returns #TRUE if the uid is filled in. - * Always returns #FALSE on non-UNIX platforms. - * Always returns #FALSE prior to authenticating the - * connection. + * Gets the UNIX user ID of the connection if known. Returns #TRUE if + * the uid is filled in. Always returns #FALSE on non-UNIX platforms. + * Always returns #FALSE prior to authenticating the connection. + * + * The UID is only read by servers from clients; clients can't usually + * get the UID of servers, because servers do not authenticate to + * clients. The returned UID is the UID the connection authenticated + * as. + * + * The message bus is a server and the apps connecting to the bus + * are clients. + * + * You can ask the bus to tell you the UID of another connection though + * if you like; this is done with dbus_bus_get_unix_user(). * * @param connection the connection * @param uid return location for the user ID diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index c0422c83..4d839241 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -287,21 +287,25 @@ extern int _dbus_current_generation; #define _DBUS_LOCK(name) _dbus_mutex_lock (_dbus_lock_##name) #define _DBUS_UNLOCK(name) _dbus_mutex_unlock (_dbus_lock_##name) +/* 1-5 */ _DBUS_DECLARE_GLOBAL_LOCK (list); _DBUS_DECLARE_GLOBAL_LOCK (connection_slots); _DBUS_DECLARE_GLOBAL_LOCK (pending_call_slots); _DBUS_DECLARE_GLOBAL_LOCK (server_slots); _DBUS_DECLARE_GLOBAL_LOCK (message_slots); +/* 5-10 */ _DBUS_DECLARE_GLOBAL_LOCK (atomic); _DBUS_DECLARE_GLOBAL_LOCK (bus); +_DBUS_DECLARE_GLOBAL_LOCK (bus_datas); _DBUS_DECLARE_GLOBAL_LOCK (shutdown_funcs); _DBUS_DECLARE_GLOBAL_LOCK (system_users); +/* 10-15 */ _DBUS_DECLARE_GLOBAL_LOCK (message_cache); _DBUS_DECLARE_GLOBAL_LOCK (shared_connections); _DBUS_DECLARE_GLOBAL_LOCK (win_fds); _DBUS_DECLARE_GLOBAL_LOCK (sid_atom_cache); _DBUS_DECLARE_GLOBAL_LOCK (machine_uuid); -#define _DBUS_N_GLOBAL_LOCKS (14) +#define _DBUS_N_GLOBAL_LOCKS (15) dbus_bool_t _dbus_threads_init_debug (void); diff --git a/dbus/dbus-sysdeps-pthread.c b/dbus/dbus-sysdeps-pthread.c index fc2acd40..2f33b1c4 100644 --- a/dbus/dbus-sysdeps-pthread.c +++ b/dbus/dbus-sysdeps-pthread.c @@ -50,8 +50,9 @@ typedef struct { #ifdef DBUS_DISABLE_ASSERT -#define PTHREAD_CHECK(func_name, result_or_call) do { \ - do { (result_or_call) } while (0) +/* (tmp != 0) is a no-op usage to silence compiler */ +#define PTHREAD_CHECK(func_name, result_or_call) \ + do { int tmp = (result_or_call); if (tmp != 0) {;} } while (0) #else #define PTHREAD_CHECK(func_name, result_or_call) do { \ int tmp = (result_or_call); \ diff --git a/dbus/dbus-test.c b/dbus/dbus-test.c index 658d2d8a..603a5092 100644 --- a/dbus/dbus-test.c +++ b/dbus/dbus-test.c @@ -50,8 +50,6 @@ check_memleaks (void) } } -#endif /* DBUS_BUILD_TESTS */ - typedef dbus_bool_t (*TestFunc)(void); typedef dbus_bool_t (*TestDataFunc)(const char *data); @@ -86,6 +84,8 @@ run_data_test (const char *test_name, check_memleaks (); } +#endif /* DBUS_BUILD_TESTS */ + /** * An exported symbol to be run in order to execute * unit tests. Should not be used by @@ -179,3 +179,4 @@ dbus_internal_do_not_use_run_tests (const char *test_data_dir, const char *speci printf ("Not compiled with unit tests, not running any\n"); #endif } + diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c index f962316b..a8712ff6 100644 --- a/dbus/dbus-threads.c +++ b/dbus/dbus-threads.c @@ -426,6 +426,7 @@ init_locks (void) LOCK_ADDR (message_slots), LOCK_ADDR (atomic), LOCK_ADDR (bus), + LOCK_ADDR (bus_datas), LOCK_ADDR (shutdown_funcs), LOCK_ADDR (system_users), LOCK_ADDR (message_cache), -- cgit