2006-10-27 Havoc Pennington <hp@redhat.com>

	* 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

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  <hp@redhat.com>
+
+	* 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  <johnp@redhat.com>
 
 	* 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),