Attachment 57050 Details for Bug 43744 – [2/4] Distinguish between two flavours of mutex

[patch] [2/4] Distinguish between two flavours of mutex

0002-Distinguish-between-two-flavours-of-mutex.patch (text/plain), 35.26 KB, created by Simon McVittie on 2012-02-14 12:20:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2012-02-14 12:20:39 UTC

Size: 35.26 KB

patch

obsolete

>From baf1a8991f31239d3f9d37d2a14b7c5764145bd8 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 14 Feb 2012 19:58:56 +0000
>Subject: [PATCH 2/4] Distinguish between two flavours of mutex
>
>dbus-threads.h warns that recursive pthreads mutexes are not compatible
>with our expectations for condition variables. However, the only two
>condition variables we actually use only have their corresponding
>mutexes locked briefly (and we don't call out to user code from there),
>so the mutexes don't need to be recursive anyway. That's just as well,
>because it turns out our implementation of recursive mutexes on
>pthreads is broken!
>
>The goal here is to be able to distinguish between "cmutexes" (mutexes
>compatible with a condition variable) and "rmutexes" (mutexes which
>are recursive if possible, to avoid deadlocking if we hold them while
>calling user code).
>
>This is complicated by the fact that callers are not guaranteed to have
>provided us with both versions of mutexes, so we might have to implement
>one by using the other (in particular, DBusRMutex *aims to be*
>recursive, it is not *guaranteed to be* recursive).
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=43744
>Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
>---
> dbus/dbus-connection.c       |   62 ++++++------
> dbus/dbus-dataslot.c         |   30 +++---
> dbus/dbus-dataslot.h         |    4 +-
> dbus/dbus-internals.h        |    8 +-
> dbus/dbus-server-protected.h |    6 +-
> dbus/dbus-server.c           |    8 +-
> dbus/dbus-threads-internal.h |   36 +++++-
> dbus/dbus-threads.c          |  243 +++++++++++++++++++++++++++++++++---------
> 8 files changed, 283 insertions(+), 114 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index b8f48ad..74a9007 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -66,18 +66,18 @@
> 
> #define CONNECTION_LOCK(connection)   do {                                      \
>     if (TRACE_LOCKS) { _dbus_verbose ("LOCK\n"); }   \
>-    _dbus_mutex_lock ((connection)->mutex);                                      \
>+    _dbus_rmutex_lock ((connection)->mutex);                                    \
>     TOOK_LOCK_CHECK (connection);                                               \
>   } while (0)
> 
> #define CONNECTION_UNLOCK(connection) _dbus_connection_unlock (connection)
> 
> #define SLOTS_LOCK(connection) do {                     \
>-    _dbus_mutex_lock ((connection)->slot_mutex);        \
>+    _dbus_rmutex_lock ((connection)->slot_mutex);       \
>   } while (0)
> 
> #define SLOTS_UNLOCK(connection) do {                   \
>-    _dbus_mutex_unlock ((connection)->slot_mutex);      \
>+    _dbus_rmutex_unlock ((connection)->slot_mutex);     \
>   } while (0)
> 
> #define DISPATCH_STATUS_NAME(s)                                            \
>@@ -264,11 +264,11 @@ struct DBusConnection
> {
>   DBusAtomic refcount; /**< Reference count. */
> 
>-  DBusMutex *mutex; /**< Lock on the entire DBusConnection */
>+  DBusRMutex *mutex; /**< Lock on the entire DBusConnection */
> 
>-  DBusMutex *dispatch_mutex;     /**< Protects dispatch_acquired */
>+  DBusCMutex *dispatch_mutex;     /**< Protects dispatch_acquired */
>   DBusCondVar *dispatch_cond;    /**< Notify when dispatch_acquired is available */
>-  DBusMutex *io_path_mutex;      /**< Protects io_path_acquired */
>+  DBusCMutex *io_path_mutex;      /**< Protects io_path_acquired */
>   DBusCondVar *io_path_cond;     /**< Notify when io_path_acquired is available */
>   
>   DBusList *outgoing_messages; /**< Queue of messages we need to send, send the end of the list first. */
>@@ -290,7 +290,7 @@ struct DBusConnection
>   
>   DBusList *filter_list;        /**< List of filters. */
> 
>-  DBusMutex *slot_mutex;        /**< Lock on slot_list so overall connection lock need not be taken */
>+  DBusRMutex *slot_mutex;        /**< Lock on slot_list so overall connection lock need not be taken */
>   DBusDataSlotList slot_list;   /**< Data stored by allocated integer ID */
> 
>   DBusHashTable *pending_replies;  /**< Hash of message serials to #DBusPendingCall. */  
>@@ -419,7 +419,7 @@ _dbus_connection_unlock (DBusConnection *connection)
>   connection->expired_messages = NULL;
> 
>   RELEASING_LOCK_CHECK (connection);
>-  _dbus_mutex_unlock (connection->mutex);
>+  _dbus_rmutex_unlock (connection->mutex);
> 
>   for (iter = _dbus_list_pop_first_link (&expired_messages);
>       iter != NULL;
>@@ -467,9 +467,9 @@ _dbus_connection_test_get_locks (DBusConnection *connection,
>                                  DBusCondVar   **dispatch_cond_loc,
>                                  DBusCondVar   **io_path_cond_loc)
> {
>-  *mutex_loc = connection->mutex;
>-  *dispatch_mutex_loc = connection->dispatch_mutex;
>-  *io_path_mutex_loc = connection->io_path_mutex; 
>+  *mutex_loc = (DBusMutex *) connection->mutex;
>+  *dispatch_mutex_loc = (DBusMutex *) connection->dispatch_mutex;
>+  *io_path_mutex_loc = (DBusMutex *) connection->io_path_mutex;
>   *dispatch_cond_loc = connection->dispatch_cond;
>   *io_path_cond_loc = connection->io_path_cond;
> }
>@@ -1079,7 +1079,7 @@ _dbus_connection_acquire_io_path (DBusConnection *connection,
>   CONNECTION_UNLOCK (connection);
>   
>   _dbus_verbose ("locking io_path_mutex\n");
>-  _dbus_mutex_lock (connection->io_path_mutex);
>+  _dbus_cmutex_lock (connection->io_path_mutex);
> 
>   _dbus_verbose ("start connection->io_path_acquired = %d timeout = %d\n",
>                  connection->io_path_acquired, timeout_milliseconds);
>@@ -1128,7 +1128,7 @@ _dbus_connection_acquire_io_path (DBusConnection *connection,
>                  connection->io_path_acquired, we_acquired);
> 
>   _dbus_verbose ("unlocking io_path_mutex\n");
>-  _dbus_mutex_unlock (connection->io_path_mutex);
>+  _dbus_cmutex_unlock (connection->io_path_mutex);
> 
>   CONNECTION_LOCK (connection);
>   
>@@ -1152,7 +1152,7 @@ _dbus_connection_release_io_path (DBusConnection *connection)
>   HAVE_LOCK_CHECK (connection);
>   
>   _dbus_verbose ("locking io_path_mutex\n");
>-  _dbus_mutex_lock (connection->io_path_mutex);
>+  _dbus_cmutex_lock (connection->io_path_mutex);
>   
>   _dbus_assert (connection->io_path_acquired);
> 
>@@ -1163,7 +1163,7 @@ _dbus_connection_release_io_path (DBusConnection *connection)
>   _dbus_condvar_wake_one (connection->io_path_cond);
> 
>   _dbus_verbose ("unlocking io_path_mutex\n");
>-  _dbus_mutex_unlock (connection->io_path_mutex);
>+  _dbus_cmutex_unlock (connection->io_path_mutex);
> }
> 
> /**
>@@ -1292,15 +1292,15 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
>   if (connection == NULL)
>     goto error;
> 
>-  _dbus_mutex_new_at_location (&connection->mutex);
>+  _dbus_rmutex_new_at_location (&connection->mutex);
>   if (connection->mutex == NULL)
>     goto error;
> 
>-  _dbus_mutex_new_at_location (&connection->io_path_mutex);
>+  _dbus_cmutex_new_at_location (&connection->io_path_mutex);
>   if (connection->io_path_mutex == NULL)
>     goto error;
> 
>-  _dbus_mutex_new_at_location (&connection->dispatch_mutex);
>+  _dbus_cmutex_new_at_location (&connection->dispatch_mutex);
>   if (connection->dispatch_mutex == NULL)
>     goto error;
>   
>@@ -1312,7 +1312,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
>   if (connection->io_path_cond == NULL)
>     goto error;
> 
>-  _dbus_mutex_new_at_location (&connection->slot_mutex);
>+  _dbus_rmutex_new_at_location (&connection->slot_mutex);
>   if (connection->slot_mutex == NULL)
>     goto error;
> 
>@@ -1391,10 +1391,10 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
>     {
>       _dbus_condvar_free_at_location (&connection->io_path_cond);
>       _dbus_condvar_free_at_location (&connection->dispatch_cond);
>-      _dbus_mutex_free_at_location (&connection->mutex);
>-      _dbus_mutex_free_at_location (&connection->io_path_mutex);
>-      _dbus_mutex_free_at_location (&connection->dispatch_mutex);
>-      _dbus_mutex_free_at_location (&connection->slot_mutex);
>+      _dbus_rmutex_free_at_location (&connection->mutex);
>+      _dbus_cmutex_free_at_location (&connection->io_path_mutex);
>+      _dbus_cmutex_free_at_location (&connection->dispatch_mutex);
>+      _dbus_rmutex_free_at_location (&connection->slot_mutex);
>       dbus_free (connection);
>     }
>   if (pending_replies)
>@@ -2739,12 +2739,12 @@ _dbus_connection_last_unref (DBusConnection *connection)
>   _dbus_condvar_free_at_location (&connection->dispatch_cond);
>   _dbus_condvar_free_at_location (&connection->io_path_cond);
> 
>-  _dbus_mutex_free_at_location (&connection->io_path_mutex);
>-  _dbus_mutex_free_at_location (&connection->dispatch_mutex);
>+  _dbus_cmutex_free_at_location (&connection->io_path_mutex);
>+  _dbus_cmutex_free_at_location (&connection->dispatch_mutex);
> 
>-  _dbus_mutex_free_at_location (&connection->slot_mutex);
>+  _dbus_rmutex_free_at_location (&connection->slot_mutex);
> 
>-  _dbus_mutex_free_at_location (&connection->mutex);
>+  _dbus_rmutex_free_at_location (&connection->mutex);
>   
>   dbus_free (connection);
> }
>@@ -4088,7 +4088,7 @@ _dbus_connection_acquire_dispatch (DBusConnection *connection)
>   CONNECTION_UNLOCK (connection);
>   
>   _dbus_verbose ("locking dispatch_mutex\n");
>-  _dbus_mutex_lock (connection->dispatch_mutex);
>+  _dbus_cmutex_lock (connection->dispatch_mutex);
> 
>   while (connection->dispatch_acquired)
>     {
>@@ -4102,7 +4102,7 @@ _dbus_connection_acquire_dispatch (DBusConnection *connection)
>   connection->dispatch_acquired = TRUE;
> 
>   _dbus_verbose ("unlocking dispatch_mutex\n");
>-  _dbus_mutex_unlock (connection->dispatch_mutex);
>+  _dbus_cmutex_unlock (connection->dispatch_mutex);
>   
>   CONNECTION_LOCK (connection);
>   _dbus_connection_unref_unlocked (connection);
>@@ -4121,7 +4121,7 @@ _dbus_connection_release_dispatch (DBusConnection *connection)
>   HAVE_LOCK_CHECK (connection);
>   
>   _dbus_verbose ("locking dispatch_mutex\n");
>-  _dbus_mutex_lock (connection->dispatch_mutex);
>+  _dbus_cmutex_lock (connection->dispatch_mutex);
>   
>   _dbus_assert (connection->dispatch_acquired);
> 
>@@ -4129,7 +4129,7 @@ _dbus_connection_release_dispatch (DBusConnection *connection)
>   _dbus_condvar_wake_one (connection->dispatch_cond);
> 
>   _dbus_verbose ("unlocking dispatch_mutex\n");
>-  _dbus_mutex_unlock (connection->dispatch_mutex);
>+  _dbus_cmutex_unlock (connection->dispatch_mutex);
> }
> 
> static void
>diff --git a/dbus/dbus-dataslot.c b/dbus/dbus-dataslot.c
>index fd25c41..0369612 100644
>--- a/dbus/dbus-dataslot.c
>+++ b/dbus/dbus-dataslot.c
>@@ -67,12 +67,12 @@ _dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator)
>  */
> dbus_bool_t
> _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator,
>-                                 DBusMutex             **mutex_loc,
>+                                 DBusRMutex            **mutex_loc,
>                                  dbus_int32_t          *slot_id_p)
> {
>   dbus_int32_t slot;
> 
>-  _dbus_mutex_lock (*mutex_loc);
>+  _dbus_rmutex_lock (*mutex_loc);
> 
>   if (allocator->n_allocated_slots == 0)
>     {
>@@ -146,7 +146,7 @@ _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator,
>                  slot, allocator, allocator->n_allocated_slots, allocator->n_used_slots);
>   
>  out:
>-  _dbus_mutex_unlock (*(allocator->lock_loc));
>+  _dbus_rmutex_unlock (*(allocator->lock_loc));
>   return slot >= 0;
> }
> 
>@@ -165,7 +165,7 @@ void
> _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator,
>                                 dbus_int32_t          *slot_id_p)
> {
>-  _dbus_mutex_lock (*(allocator->lock_loc));
>+  _dbus_rmutex_lock (*(allocator->lock_loc));
>   
>   _dbus_assert (*slot_id_p < allocator->n_allocated_slots);
>   _dbus_assert (allocator->allocated_slots[*slot_id_p].slot_id == *slot_id_p);
>@@ -175,7 +175,7 @@ _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator,
> 
>   if (allocator->allocated_slots[*slot_id_p].refcount > 0)
>     {
>-      _dbus_mutex_unlock (*(allocator->lock_loc));
>+      _dbus_rmutex_unlock (*(allocator->lock_loc));
>       return;
>     }
> 
>@@ -190,18 +190,18 @@ _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator,
>   
>   if (allocator->n_used_slots == 0)
>     {
>-      DBusMutex **mutex_loc = allocator->lock_loc;
>+      DBusRMutex **mutex_loc = allocator->lock_loc;
>       
>       dbus_free (allocator->allocated_slots);
>       allocator->allocated_slots = NULL;
>       allocator->n_allocated_slots = 0;
>       allocator->lock_loc = NULL;
> 
>-      _dbus_mutex_unlock (*mutex_loc);
>+      _dbus_rmutex_unlock (*mutex_loc);
>     }
>   else
>     {
>-      _dbus_mutex_unlock (*(allocator->lock_loc));
>+      _dbus_rmutex_unlock (*(allocator->lock_loc));
>     }
> }
> 
>@@ -247,10 +247,10 @@ _dbus_data_slot_list_set  (DBusDataSlotAllocator *allocator,
>    * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts
>    * are disabled, since then the asserts are empty.
>    */
>-  _dbus_mutex_lock (*(allocator->lock_loc));
>+  _dbus_rmutex_lock (*(allocator->lock_loc));
>   _dbus_assert (slot < allocator->n_allocated_slots);
>   _dbus_assert (allocator->allocated_slots[slot].slot_id == slot);
>-  _dbus_mutex_unlock (*(allocator->lock_loc));
>+  _dbus_rmutex_unlock (*(allocator->lock_loc));
> #endif
>   
>   if (slot >= list->n_slots)
>@@ -304,11 +304,11 @@ _dbus_data_slot_list_get  (DBusDataSlotAllocator *allocator,
>    * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts
>    * are disabled, since then the asserts are empty.
>    */
>-  _dbus_mutex_lock (*(allocator->lock_loc));
>+  _dbus_rmutex_lock (*(allocator->lock_loc));
>   _dbus_assert (slot >= 0);
>   _dbus_assert (slot < allocator->n_allocated_slots);
>   _dbus_assert (allocator->allocated_slots[slot].slot_id == slot);
>-  _dbus_mutex_unlock (*(allocator->lock_loc));
>+  _dbus_rmutex_unlock (*(allocator->lock_loc));
> #endif
> 
>   if (slot >= list->n_slots)
>@@ -384,14 +384,14 @@ _dbus_data_slot_test (void)
>   int i;
>   DBusFreeFunction old_free_func;
>   void *old_data;
>-  DBusMutex *mutex;
>+  DBusRMutex *mutex;
>   
>   if (!_dbus_data_slot_allocator_init (&allocator))
>     _dbus_assert_not_reached ("no memory for allocator");
> 
>   _dbus_data_slot_list_init (&list);
> 
>-  _dbus_mutex_new_at_location (&mutex);
>+  _dbus_rmutex_new_at_location (&mutex);
>   if (mutex == NULL)
>     _dbus_assert_not_reached ("failed to alloc mutex");
>   
>@@ -471,7 +471,7 @@ _dbus_data_slot_test (void)
>       ++i;
>     }
> 
>-  _dbus_mutex_free_at_location (&mutex);
>+  _dbus_rmutex_free_at_location (&mutex);
>   
>   return TRUE;
> }
>diff --git a/dbus/dbus-dataslot.h b/dbus/dbus-dataslot.h
>index 2e706f7..3d9d5ed 100644
>--- a/dbus/dbus-dataslot.h
>+++ b/dbus/dbus-dataslot.h
>@@ -57,7 +57,7 @@ struct DBusDataSlotAllocator
>   DBusAllocatedSlot *allocated_slots; /**< Allocated slots */
>   int  n_allocated_slots; /**< number of slots malloc'd */
>   int  n_used_slots;      /**< number of slots used */
>-  DBusMutex **lock_loc;   /**< location of thread lock */
>+  DBusRMutex **lock_loc;  /**< location of thread lock */
> };
> 
> /**
>@@ -72,7 +72,7 @@ struct DBusDataSlotList
> 
> dbus_bool_t _dbus_data_slot_allocator_init  (DBusDataSlotAllocator  *allocator);
> dbus_bool_t _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator  *allocator,
>-                                             DBusMutex              **mutex_loc,
>+                                             DBusRMutex             **mutex_loc,
>                                              int                    *slot_id_p);
> void        _dbus_data_slot_allocator_free  (DBusDataSlotAllocator  *allocator,
>                                              int                    *slot_id_p);
>diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h
>index 162a222..8036a2b 100644
>--- a/dbus/dbus-internals.h
>+++ b/dbus/dbus-internals.h
>@@ -304,10 +304,10 @@ extern int _dbus_current_generation;
> 
> /* Thread initializers */
> #define _DBUS_LOCK_NAME(name)           _dbus_lock_##name
>-#define _DBUS_DECLARE_GLOBAL_LOCK(name) extern DBusMutex  *_dbus_lock_##name
>-#define _DBUS_DEFINE_GLOBAL_LOCK(name)  DBusMutex         *_dbus_lock_##name  
>-#define _DBUS_LOCK(name)                _dbus_mutex_lock   (_dbus_lock_##name)
>-#define _DBUS_UNLOCK(name)              _dbus_mutex_unlock (_dbus_lock_##name)
>+#define _DBUS_DECLARE_GLOBAL_LOCK(name) extern DBusRMutex *_dbus_lock_##name
>+#define _DBUS_DEFINE_GLOBAL_LOCK(name)  DBusRMutex        *_dbus_lock_##name
>+#define _DBUS_LOCK(name)                _dbus_rmutex_lock   (_dbus_lock_##name)
>+#define _DBUS_UNLOCK(name)              _dbus_rmutex_unlock (_dbus_lock_##name)
> 
> /* 1-5 */
> _DBUS_DECLARE_GLOBAL_LOCK (list);
>diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h
>index df306f7..dd5234b 100644
>--- a/dbus/dbus-server-protected.h
>+++ b/dbus/dbus-server-protected.h
>@@ -57,7 +57,7 @@ struct DBusServer
> {
>   DBusAtomic refcount;                        /**< Reference count. */
>   const DBusServerVTable *vtable;             /**< Virtual methods for this instance. */
>-  DBusMutex *mutex;                           /**< Lock on the server object */
>+  DBusRMutex *mutex;                          /**< Lock on the server object */
> 
>   DBusGUID guid;                              /**< Globally unique ID of server */
> 
>@@ -161,14 +161,14 @@ void _dbus_server_trace_ref (DBusServer *server,
> 
> #define SERVER_LOCK(server)   do {                                              \
>     if (TRACE_LOCKS) { _dbus_verbose ("LOCK\n"); }   \
>-    _dbus_mutex_lock ((server)->mutex);                                          \
>+    _dbus_rmutex_lock ((server)->mutex);                                        \
>     TOOK_LOCK_CHECK (server);                                                   \
>   } while (0)
> 
> #define SERVER_UNLOCK(server) do {                                                      \
>     if (TRACE_LOCKS) { _dbus_verbose ("UNLOCK\n");  }        \
>     RELEASING_LOCK_CHECK (server);                                                      \
>-    _dbus_mutex_unlock ((server)->mutex);                                                \
>+    _dbus_rmutex_unlock ((server)->mutex);                                              \
>   } while (0)
> 
> DBUS_END_DECLS
>diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c
>index 157557c..b62c2b4 100644
>--- a/dbus/dbus-server.c
>+++ b/dbus/dbus-server.c
>@@ -142,7 +142,7 @@ _dbus_server_init_base (DBusServer             *server,
>   if (server->address == NULL)
>     goto failed;
>   
>-  _dbus_mutex_new_at_location (&server->mutex);
>+  _dbus_rmutex_new_at_location (&server->mutex);
>   if (server->mutex == NULL)
>     goto failed;
>   
>@@ -161,7 +161,7 @@ _dbus_server_init_base (DBusServer             *server,
>   return TRUE;
> 
>  failed:
>-  _dbus_mutex_free_at_location (&server->mutex);
>+  _dbus_rmutex_free_at_location (&server->mutex);
>   server->mutex = NULL;
>   if (server->watches)
>     {
>@@ -208,7 +208,7 @@ _dbus_server_finalize_base (DBusServer *server)
>   _dbus_watch_list_free (server->watches);
>   _dbus_timeout_list_free (server->timeouts);
> 
>-  _dbus_mutex_free_at_location (&server->mutex);
>+  _dbus_rmutex_free_at_location (&server->mutex);
>   
>   dbus_free (server->address);
> 
>@@ -1093,7 +1093,7 @@ dbus_bool_t
> dbus_server_allocate_data_slot (dbus_int32_t *slot_p)
> {
>   return _dbus_data_slot_allocator_alloc (&slot_allocator,
>-                                          (DBusMutex **)&_DBUS_LOCK_NAME (server_slots),
>+                                          (DBusRMutex **)&_DBUS_LOCK_NAME (server_slots),
>                                           slot_p);
> }
> 
>diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h
>index 68aa20a..2561136 100644
>--- a/dbus/dbus-threads-internal.h
>+++ b/dbus/dbus-threads-internal.h
>@@ -27,19 +27,43 @@
> #include <dbus/dbus-types.h>
> #include <dbus/dbus-threads.h>
> 
>+/**
>+ * @addtogroup DBusThreadsInternals
>+ * @{
>+ */
>+
>+/**
>+ * A mutex which is recursive if possible, else non-recursive.
>+ * This is typically recursive, but that cannot be relied upon.
>+ */
>+typedef struct DBusRMutex DBusRMutex;
>+
>+/**
>+ * A mutex suitable for use with condition variables.
>+ * This is typically non-recursive.
>+ */
>+typedef struct DBusCMutex DBusCMutex;
>+
>+/** @} */
>+
> DBUS_BEGIN_DECLS
> 
>-void         _dbus_mutex_lock                (DBusMutex         *mutex);
>-void         _dbus_mutex_unlock              (DBusMutex         *mutex);
>-void         _dbus_mutex_new_at_location     (DBusMutex        **location_p);
>-void         _dbus_mutex_free_at_location    (DBusMutex        **location_p);
>+void         _dbus_rmutex_lock               (DBusRMutex       *mutex);
>+void         _dbus_rmutex_unlock             (DBusRMutex       *mutex);
>+void         _dbus_rmutex_new_at_location    (DBusRMutex      **location_p);
>+void         _dbus_rmutex_free_at_location   (DBusRMutex      **location_p);
>+
>+void         _dbus_cmutex_lock               (DBusCMutex       *mutex);
>+void         _dbus_cmutex_unlock             (DBusCMutex       *mutex);
>+void         _dbus_cmutex_new_at_location    (DBusCMutex      **location_p);
>+void         _dbus_cmutex_free_at_location   (DBusCMutex      **location_p);
> 
> DBusCondVar* _dbus_condvar_new               (void);
> void         _dbus_condvar_free              (DBusCondVar       *cond);
> void         _dbus_condvar_wait              (DBusCondVar       *cond,
>-                                              DBusMutex         *mutex);
>+                                              DBusCMutex        *mutex);
> dbus_bool_t  _dbus_condvar_wait_timeout      (DBusCondVar       *cond,
>-                                              DBusMutex         *mutex,
>+                                              DBusCMutex        *mutex,
>                                               int                timeout_milliseconds);
> void         _dbus_condvar_wake_one          (DBusCondVar       *cond);
> void         _dbus_condvar_wake_all          (DBusCondVar       *cond);
>diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c
>index 5eda6f1..eff3054 100644
>--- a/dbus/dbus-threads.c
>+++ b/dbus/dbus-threads.c
>@@ -38,7 +38,8 @@ static DBusThreadFunctions thread_functions =
> 
> static int thread_init_generation = 0;
>  
>-static DBusList *uninitialized_mutex_list = NULL;
>+static DBusList *uninitialized_rmutex_list = NULL;
>+static DBusList *uninitialized_cmutex_list = NULL;
> static DBusList *uninitialized_condvar_list = NULL;
> 
> /** This is used for the no-op default mutex pointer, just to be distinct from #NULL */
>@@ -47,12 +48,19 @@ static DBusList *uninitialized_condvar_list = NULL;
> /** This is used for the no-op default mutex pointer, just to be distinct from #NULL */
> #define _DBUS_DUMMY_CONDVAR ((DBusCondVar*)0xABCDEF2)
> 
>-static void _dbus_mutex_free (DBusMutex *mutex);
>+static void _dbus_mutex_free (DBusMutex *mutex, DBusMutexFreeFunction dtor);
>+static void _dbus_mutex_new_at_location (DBusMutex            **location_p,
>+                                         DBusMutexNewFunction   ctor,
>+                                         DBusMutexFreeFunction  dtor,
>+                                         DBusList             **uninitialized);
>+static void _dbus_mutex_free_at_location (DBusMutex            **location_p,
>+                                          DBusMutexFreeFunction  dtor,
>+                                          DBusList             **uninitialized);
> 
> /**
>  * @defgroup DBusThreadsInternals Thread functions
>  * @ingroup  DBusInternals
>- * @brief _dbus_mutex_lock(), etc.
>+ * @brief _dbus_rmutex_lock(), etc.
>  *
>  * Functions and macros related to threads and thread locks.
>  *
>@@ -60,9 +68,11 @@ static void _dbus_mutex_free (DBusMutex *mutex);
>  */
> 
> static DBusMutex *
>-_dbus_mutex_new (void)
>+_dbus_mutex_new (DBusMutexNewFunction ctor)
> {
>-  if (thread_functions.recursive_mutex_new)
>+  if (ctor)
>+    return ctor ();
>+  else if (thread_functions.recursive_mutex_new)
>     return (* thread_functions.recursive_mutex_new) ();
>   else if (thread_functions.mutex_new)
>     return (* thread_functions.mutex_new) ();
>@@ -76,6 +86,33 @@ _dbus_mutex_new (void)
>  * May return #NULL even if threads are initialized, indicating
>  * out-of-memory.
>  *
>+ * If possible, the mutex returned by this function is recursive, to
>+ * avoid deadlocks. However, that cannot be relied on.
>+ *
>+ * The extra level of indirection given by allocating a pointer
>+ * to point to the mutex location allows the threading
>+ * module to swap out dummy mutexes for real a real mutex so libraries
>+ * can initialize threads even after the D-Bus API has been used.
>+ *
>+ * @param location_p the location of the new mutex, can return #NULL on OOM
>+ */
>+void
>+_dbus_rmutex_new_at_location (DBusRMutex **location_p)
>+{
>+  _dbus_mutex_new_at_location ((DBusMutex **) location_p,
>+                               thread_functions.recursive_mutex_new,
>+                               thread_functions.recursive_mutex_free,
>+                               &uninitialized_rmutex_list);
>+}
>+
>+/**
>+ * Creates a new mutex using the function supplied to dbus_threads_init(),
>+ * or creates a no-op mutex if threads are not initialized.
>+ * May return #NULL even if threads are initialized, indicating
>+ * out-of-memory.
>+ *
>+ * The returned mutex is suitable for use with condition variables.
>+ *
>  * The extra level of indirection given by allocating a pointer
>  * to point to the mutex location allows the threading
>  * module to swap out dummy mutexes for real a real mutex so libraries
>@@ -84,65 +121,120 @@ _dbus_mutex_new (void)
>  * @param location_p the location of the new mutex, can return #NULL on OOM
>  */
> void
>-_dbus_mutex_new_at_location (DBusMutex **location_p)
>+_dbus_cmutex_new_at_location (DBusCMutex **location_p)
>+{
>+  _dbus_mutex_new_at_location ((DBusMutex **) location_p,
>+                               thread_functions.mutex_new,
>+                               thread_functions.mutex_free,
>+                               &uninitialized_cmutex_list);
>+}
>+
>+static void
>+_dbus_mutex_new_at_location (DBusMutex            **location_p,
>+                             DBusMutexNewFunction   ctor,
>+                             DBusMutexFreeFunction  dtor,
>+                             DBusList             **uninitialized)
> {
>   _dbus_assert (location_p != NULL);
> 
>-  *location_p = _dbus_mutex_new();
>+  *location_p = _dbus_mutex_new (ctor);
> 
>   if (thread_init_generation != _dbus_current_generation && *location_p)
>     {
>-      if (!_dbus_list_append (&uninitialized_mutex_list, location_p))
>+      if (!_dbus_list_append (uninitialized, location_p))
>         {
>-	  _dbus_mutex_free (*location_p);
>+	  _dbus_mutex_free (*location_p, dtor);
> 	  *location_p = NULL;
> 	}
>     }
> }
> 
> static void
>-_dbus_mutex_free (DBusMutex *mutex)
>+_dbus_mutex_free (DBusMutex             *mutex,
>+                  DBusMutexFreeFunction  dtor)
> {
>   if (mutex)
>     {
>-      if (mutex && thread_functions.recursive_mutex_free)
>+      if (dtor)
>+        dtor (mutex);
>+      else if (mutex && thread_functions.recursive_mutex_free)
>         (* thread_functions.recursive_mutex_free) (mutex);
>       else if (mutex && thread_functions.mutex_free)
>         (* thread_functions.mutex_free) (mutex);
>     }
> }
> 
>-/**
>- * Frees a mutex and removes it from the 
>- * uninitialized_mutex_list;
>- * does nothing if passed a #NULL pointer.
>- */
>-void
>-_dbus_mutex_free_at_location (DBusMutex **location_p)
>+static void
>+_dbus_mutex_free_at_location (DBusMutex            **location_p,
>+                              DBusMutexFreeFunction  dtor,
>+                              DBusList             **uninitialized)
> {
>   if (location_p)
>     {
>       if (thread_init_generation != _dbus_current_generation)
>-        _dbus_list_remove (&uninitialized_mutex_list, location_p);
>+        _dbus_list_remove (uninitialized, location_p);
> 
>-      _dbus_mutex_free (*location_p);
>+      _dbus_mutex_free (*location_p, dtor);
>     }
> }
> 
> /**
>+ * Frees a DBusRMutex and removes it from the
>+ * uninitialized mutex list;
>+ * does nothing if passed a #NULL pointer.
>+ */
>+void
>+_dbus_rmutex_free_at_location (DBusRMutex **location_p)
>+{
>+  _dbus_mutex_free_at_location ((DBusMutex **) location_p,
>+                                thread_functions.recursive_mutex_free,
>+                                &uninitialized_rmutex_list);
>+}
>+
>+/**
>+ * Frees a DBusCMutex and removes it from the
>+ * uninitialized mutex list;
>+ * does nothing if passed a #NULL pointer.
>+ */
>+void
>+_dbus_cmutex_free_at_location (DBusCMutex **location_p)
>+{
>+  _dbus_mutex_free_at_location ((DBusMutex **) location_p,
>+                                thread_functions.mutex_free,
>+                                &uninitialized_cmutex_list);
>+}
>+
>+/**
>  * Locks a mutex. Does nothing if passed a #NULL pointer.
>  * Locks may be recursive if threading implementation initialized
>  * recursive locks.
>  */
> void
>-_dbus_mutex_lock (DBusMutex *mutex)
>+_dbus_rmutex_lock (DBusRMutex *mutex)
> {
>-  if (mutex) 
>+  if (mutex)
>     {
>       if (thread_functions.recursive_mutex_lock)
>-        (* thread_functions.recursive_mutex_lock) (mutex);
>+        (* thread_functions.recursive_mutex_lock) ((DBusMutex *) mutex);
>       else if (thread_functions.mutex_lock)
>-        (* thread_functions.mutex_lock) (mutex);
>+        (* thread_functions.mutex_lock) ((DBusMutex *) mutex);
>+    }
>+}
>+
>+/**
>+ * Locks a mutex. Does nothing if passed a #NULL pointer.
>+ * Locks may be recursive if threading implementation initialized
>+ * recursive locks.
>+ */
>+void
>+_dbus_cmutex_lock (DBusCMutex *mutex)
>+{
>+  if (mutex)
>+    {
>+      if (thread_functions.mutex_lock)
>+        (* thread_functions.mutex_lock) ((DBusMutex *) mutex);
>+      else if (thread_functions.recursive_mutex_lock)
>+        (* thread_functions.recursive_mutex_lock) ((DBusMutex *) mutex);
>     }
> }
> 
>@@ -152,14 +244,31 @@ _dbus_mutex_lock (DBusMutex *mutex)
>  * @returns #TRUE on success
>  */
> void
>-_dbus_mutex_unlock (DBusMutex *mutex)
>+_dbus_rmutex_unlock (DBusRMutex *mutex)
> {
>   if (mutex)
>     {
>       if (thread_functions.recursive_mutex_unlock)
>-        (* thread_functions.recursive_mutex_unlock) (mutex);
>+        (* thread_functions.recursive_mutex_unlock) ((DBusMutex *) mutex);
>       else if (thread_functions.mutex_unlock)
>-        (* thread_functions.mutex_unlock) (mutex);
>+        (* thread_functions.mutex_unlock) ((DBusMutex *) mutex);
>+    }
>+}
>+
>+/**
>+ * Unlocks a mutex. Does nothing if passed a #NULL pointer.
>+ *
>+ * @returns #TRUE on success
>+ */
>+void
>+_dbus_cmutex_unlock (DBusCMutex *mutex)
>+{
>+  if (mutex)
>+    {
>+      if (thread_functions.mutex_unlock)
>+        (* thread_functions.mutex_unlock) ((DBusMutex *) mutex);
>+      else if (thread_functions.recursive_mutex_unlock)
>+        (* thread_functions.recursive_mutex_unlock) ((DBusMutex *) mutex);
>     }
> }
> 
>@@ -243,10 +352,10 @@ _dbus_condvar_free_at_location (DBusCondVar **location_p)
>  */
> void
> _dbus_condvar_wait (DBusCondVar *cond,
>-                    DBusMutex   *mutex)
>+                    DBusCMutex  *mutex)
> {
>   if (cond && mutex && thread_functions.condvar_wait)
>-    (* thread_functions.condvar_wait) (cond, mutex);
>+    (* thread_functions.condvar_wait) (cond, (DBusMutex *) mutex);
> }
> 
> /**
>@@ -262,11 +371,13 @@ _dbus_condvar_wait (DBusCondVar *cond,
>  */
> dbus_bool_t
> _dbus_condvar_wait_timeout (DBusCondVar               *cond,
>-                            DBusMutex                 *mutex,
>+                            DBusCMutex                *mutex,
>                             int                        timeout_milliseconds)
> {
>   if (cond && mutex && thread_functions.condvar_wait)
>-    return (* thread_functions.condvar_wait_timeout) (cond, mutex, timeout_milliseconds);
>+    return (* thread_functions.condvar_wait_timeout) (cond,
>+                                                      (DBusMutex *) mutex,
>+                                                      timeout_milliseconds);
>   else
>     return TRUE;
> }
>@@ -304,7 +415,7 @@ shutdown_global_locks (void *data)
>   i = 0;
>   while (i < _DBUS_N_GLOBAL_LOCKS)
>     {
>-      _dbus_mutex_free (*(locks[i]));
>+      _dbus_mutex_free (*(locks[i]), thread_functions.recursive_mutex_free);
>       *(locks[i]) = NULL;
>       ++i;
>     }
>@@ -315,7 +426,8 @@ shutdown_global_locks (void *data)
> static void
> shutdown_uninitialized_locks (void *data)
> {
>-  _dbus_list_clear (&uninitialized_mutex_list);
>+  _dbus_list_clear (&uninitialized_rmutex_list);
>+  _dbus_list_clear (&uninitialized_cmutex_list);
>   _dbus_list_clear (&uninitialized_condvar_list);
> }
> 
>@@ -326,19 +438,34 @@ init_uninitialized_locks (void)
> 
>   _dbus_assert (thread_init_generation != _dbus_current_generation);
> 
>-  link = uninitialized_mutex_list;
>+  link = uninitialized_rmutex_list;
>+  while (link != NULL)
>+    {
>+      DBusMutex **mp;
>+
>+      mp = link->data;
>+      _dbus_assert (*mp == _DBUS_DUMMY_MUTEX);
>+
>+      *mp = _dbus_mutex_new (thread_functions.recursive_mutex_new);
>+      if (*mp == NULL)
>+        goto fail_mutex;
>+
>+      link = _dbus_list_get_next_link (&uninitialized_rmutex_list, link);
>+    }
>+
>+  link = uninitialized_cmutex_list;
>   while (link != NULL)
>     {
>       DBusMutex **mp;
> 
>-      mp = (DBusMutex **)link->data;
>+      mp = link->data;
>       _dbus_assert (*mp == _DBUS_DUMMY_MUTEX);
> 
>-      *mp = _dbus_mutex_new ();
>+      *mp = _dbus_mutex_new (thread_functions.mutex_new);
>       if (*mp == NULL)
>         goto fail_mutex;
> 
>-      link = _dbus_list_get_next_link (&uninitialized_mutex_list, link);
>+      link = _dbus_list_get_next_link (&uninitialized_cmutex_list, link);
>     }
> 
>   link = uninitialized_condvar_list;
>@@ -356,7 +483,8 @@ init_uninitialized_locks (void)
>       link = _dbus_list_get_next_link (&uninitialized_condvar_list, link);
>     }
> 
>-  _dbus_list_clear (&uninitialized_mutex_list);
>+  _dbus_list_clear (&uninitialized_rmutex_list);
>+  _dbus_list_clear (&uninitialized_cmutex_list);
>   _dbus_list_clear (&uninitialized_condvar_list);
> 
>   if (!_dbus_register_shutdown_func (shutdown_uninitialized_locks,
>@@ -384,21 +512,38 @@ init_uninitialized_locks (void)
>     }
> 
>  fail_mutex:
>-  link = uninitialized_mutex_list;
>+  link = uninitialized_rmutex_list;
>   while (link != NULL)
>     {
>       DBusMutex **mp;
> 
>-      mp = (DBusMutex **)link->data;
>+      mp = link->data;
> 
>       if (*mp != _DBUS_DUMMY_MUTEX)
>-        _dbus_mutex_free (*mp);
>+        _dbus_mutex_free (*mp, thread_functions.recursive_mutex_free);
>       else
>         break;
> 
>       *mp = _DBUS_DUMMY_MUTEX;
> 
>-      link = _dbus_list_get_next_link (&uninitialized_mutex_list, link);
>+      link = _dbus_list_get_next_link (&uninitialized_rmutex_list, link);
>+    }
>+
>+  link = uninitialized_cmutex_list;
>+  while (link != NULL)
>+    {
>+      DBusMutex **mp;
>+
>+      mp = link->data;
>+
>+      if (*mp != _DBUS_DUMMY_MUTEX)
>+        _dbus_mutex_free (*mp, thread_functions.mutex_free);
>+      else
>+        break;
>+
>+      *mp = _DBUS_DUMMY_MUTEX;
>+
>+      link = _dbus_list_get_next_link (&uninitialized_cmutex_list, link);
>     }
> 
>   return FALSE;
>@@ -408,9 +553,8 @@ static dbus_bool_t
> init_locks (void)
> {
>   int i;
>-  DBusMutex ***dynamic_global_locks;
>-  
>-  DBusMutex **global_locks[] = {
>+  DBusRMutex ***dynamic_global_locks;
>+  DBusRMutex **global_locks[] = {
> #define LOCK_ADDR(name) (& _dbus_lock_##name)
>     LOCK_ADDR (win_fds),
>     LOCK_ADDR (sid_atom_cache),
>@@ -437,14 +581,14 @@ init_locks (void)
> 
>   i = 0;
>   
>-  dynamic_global_locks = dbus_new (DBusMutex**, _DBUS_N_GLOBAL_LOCKS);
>+  dynamic_global_locks = dbus_new (DBusRMutex**, _DBUS_N_GLOBAL_LOCKS);
>   if (dynamic_global_locks == NULL)
>     goto failed;
>   
>   while (i < _DBUS_N_ELEMENTS (global_locks))
>     {
>-      *global_locks[i] = _dbus_mutex_new ();
>-      
>+      *global_locks[i] = (DBusRMutex *) _dbus_mutex_new (thread_functions.recursive_mutex_new);
>+
>       if (*global_locks[i] == NULL)
>         goto failed;
> 
>@@ -467,7 +611,8 @@ init_locks (void)
>                                      
>   for (i = i - 1; i >= 0; i--)
>     {
>-      _dbus_mutex_free (*global_locks[i]);
>+      _dbus_mutex_free ((DBusMutex *) *global_locks[i],
>+                        thread_functions.recursive_mutex_free);
>       *global_locks[i] = NULL;
>     }
>   return FALSE;
>-- 
>1.7.9
>

Actions: View | Splinter Review

Attachments on bug 43744: 54357 | 57049 | 57050 | 57051 | 57052 | 57078 | 57079 | 57166 | 57212 | 57326 | 57327 | 57328 | 57329 | 57330 | 57331 | 57332 | 57761 | 57762

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.