summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSimon McVittie <simon.mcvittie@collabora.co.uk>2011-06-10 18:08:11 +0100
committerSimon McVittie <simon.mcvittie@collabora.co.uk>2011-06-10 18:08:11 +0100
commit50732523a78254b28fad9427c91f774292de69cd (patch)
tree35c3dc842fae2466700a0d26a43c31a4693191be
parent73e259322de5d4e46bbac8113ab5dcb38c69dd94 (diff)
Consistently use atomic operations for all access to DBusConnection refcount
Trying to mix atomic operations with locked non-atomic operations is broken: the atomic ops aren't necessarily atomic with respect to the locked non-atomic ops, and the non-atomic ops aren't protected by the lock because the atomic ops can change the refcount behind their back. In theory we could use the connection lock if atomic ops aren't supported (making a per-connection lock cheaper than the global lock used to implement atomic ops) *and* our mutexes are recursive (making it safe against deadlocks)... but life's too short. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=38005 Tested-by: Will Manley <freedesktop williammanley net> Reviewed-by: Will Thompson <will.thompson@collabora.co.uk>
-rw-r--r--dbus/dbus-connection.c84
1 files changed, 18 insertions, 66 deletions
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index d0aca256..aabd7a0e 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -1399,13 +1399,8 @@ _dbus_connection_ref_unlocked (DBusConnection *connection)
_dbus_assert (connection->generation == _dbus_current_generation);
HAVE_LOCK_CHECK (connection);
-
-#ifdef DBUS_HAVE_ATOMIC_INT
+
_dbus_atomic_inc (&connection->refcount);
-#else
- _dbus_assert (connection->refcount.value > 0);
- connection->refcount.value += 1;
-#endif
return connection;
}
@@ -1425,22 +1420,8 @@ _dbus_connection_unref_unlocked (DBusConnection *connection)
_dbus_assert (connection != NULL);
- /* The connection lock is better than the global
- * lock in the atomic increment fallback
- */
-
-#ifdef DBUS_HAVE_ATOMIC_INT
last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
-#else
- _dbus_assert (connection->refcount.value > 0);
- connection->refcount.value -= 1;
- last_unref = (connection->refcount.value == 0);
-#if 0
- printf ("unref_unlocked() connection %p count = %d\n", connection, connection->refcount.value);
-#endif
-#endif
-
if (last_unref)
_dbus_connection_last_unref (connection);
}
@@ -2126,11 +2107,23 @@ _dbus_connection_send_and_unlock (DBusConnection *connection,
void
_dbus_connection_close_if_only_one_ref (DBusConnection *connection)
{
+ dbus_int32_t tmp_refcount;
+
CONNECTION_LOCK (connection);
-
- _dbus_assert (connection->refcount.value > 0);
- if (connection->refcount.value == 1)
+ /* We increment and then decrement the refcount, because there is no
+ * _dbus_atomic_get (mirroring the fact that there's no InterlockedGet
+ * on Windows). */
+ _dbus_atomic_inc (&connection->refcount);
+ tmp_refcount = _dbus_atomic_dec (&connection->refcount);
+
+ /* The caller should have one ref, and this function temporarily took
+ * one more, which is reflected in this count even though we already
+ * released it (relying on the caller's ref) due to _dbus_atomic_dec
+ * semantics */
+ _dbus_assert (tmp_refcount >= 2);
+
+ if (tmp_refcount == 2)
_dbus_connection_close_possibly_shared_and_unlock (connection);
else
CONNECTION_UNLOCK (connection);
@@ -2632,25 +2625,8 @@ dbus_connection_ref (DBusConnection *connection)
{
_dbus_return_val_if_fail (connection != NULL, NULL);
_dbus_return_val_if_fail (connection->generation == _dbus_current_generation, NULL);
-
- /* The connection lock is better than the global
- * lock in the atomic increment fallback
- *
- * (FIXME but for now we always use the atomic version,
- * to avoid taking the connection lock, due to
- * the mess with set_timeout_functions()/set_watch_functions()
- * calling out to the app without dropping locks)
- */
-
-#if 1
- _dbus_atomic_inc (&connection->refcount);
-#else
- CONNECTION_LOCK (connection);
- _dbus_assert (connection->refcount.value > 0);
- connection->refcount.value += 1;
- CONNECTION_UNLOCK (connection);
-#endif
+ _dbus_atomic_inc (&connection->refcount);
return connection;
}
@@ -2787,33 +2763,9 @@ dbus_connection_unref (DBusConnection *connection)
_dbus_return_if_fail (connection != NULL);
_dbus_return_if_fail (connection->generation == _dbus_current_generation);
-
- /* The connection lock is better than the global
- * lock in the atomic increment fallback
- *
- * (FIXME but for now we always use the atomic version,
- * to avoid taking the connection lock, due to
- * the mess with set_timeout_functions()/set_watch_functions()
- * calling out to the app without dropping locks)
- */
-
-#if 1
- last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
-#else
- CONNECTION_LOCK (connection);
-
- _dbus_assert (connection->refcount.value > 0);
- connection->refcount.value -= 1;
- last_unref = (connection->refcount.value == 0);
+ last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
-#if 0
- printf ("unref() connection %p count = %d\n", connection, connection->refcount.value);
-#endif
-
- CONNECTION_UNLOCK (connection);
-#endif
-
if (last_unref)
{
#ifndef DBUS_DISABLE_CHECKS