[PATCH] Privatize the DBusPendingCall struct
Havoc Pennington
hp at redhat.com
Fri Jul 21 16:31:07 PDT 2006
Here is what the full fix might look like. I haven't really tested it;
if someone could test/debug that would be a good idea. I don't have a
lot of time to work on this.
I see a bunch of errors in 'make check' about removing timeouts that
weren't added (mixed in with the "out of memory" errors that I believe
are expected, but the timeout ones are not).
Don't know if this patch introduces those, but they need fixing in any case.
I added a TODO item for 1.0 which amounts to testing and committing the
attached patch (including that error about removing timeouts, that
should also be a 1.0 blocker as it surely indicates some real bug)
btw, I think I'm still the only or one of the few people getting dbus
bugzilla mail, but I'm not doing anything about any of it, so maybe
others want to jump on that cc list...
Havoc
-------------- next part --------------
? test/name-test/.deps
? test/name-test/.libs
? test/name-test/Makefile
? test/name-test/Makefile.in
? test/name-test/run-with-tmp-session-bus.conf
? test/name-test/test-names
? test/name-test/test-pending-call-dispatch
Index: ChangeLog
===================================================================
RCS file: /cvs/dbus/dbus/ChangeLog,v
retrieving revision 1.1056
diff -u -r1.1056 ChangeLog
--- ChangeLog 21 Jul 2006 19:28:56 -0000 1.1056
+++ ChangeLog 21 Jul 2006 23:24:23 -0000
@@ -1,3 +1,16 @@
+2006-07-21 Havoc Pennington <hp at redhat.com>
+
+ * configure.in: add -Wdeclaration-after-statement
+
+ * dbus/dbus-connection.c: change all the pending call stuff to
+ reflect the fact that pending call operations use the connection
+ lock
+
+ * dbus/dbus-pending-call.c: add locking here
+
+ * dbus/dbus-errors.c (struct DBusRealError): don't make the name
+ field const consistent with how message field is done
+
2006-07-21 John (J5) Palmieri <johnp at redhat.com>
* Removed some extra bindings stuff lingering around (thanks timo)
Index: configure.in
===================================================================
RCS file: /cvs/dbus/dbus/configure.in,v
retrieving revision 1.158
diff -u -r1.158 configure.in
--- configure.in 21 Jul 2006 19:28:56 -0000 1.158
+++ configure.in 21 Jul 2006 23:24:24 -0000
@@ -138,6 +138,11 @@
*) CFLAGS="$CFLAGS -Wsign-compare" ;;
esac
+ case " $CFLAGS " in
+ *[\ \ ]-Wdeclaration-after-statement[\ \ ]*) ;;
+ *) CFLAGS="$CFLAGS -Wdeclaration-after-statement" ;;
+ esac
+
if test "x$enable_ansi" = "xyes"; then
case " $CFLAGS " in
*[\ \ ]-ansi[\ \ ]*) ;;
Index: dbus/dbus-connection.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-connection.c,v
retrieving revision 1.122
diff -u -r1.122 dbus-connection.c
--- dbus/dbus-connection.c 14 Jul 2006 03:09:22 -0000 1.122
+++ dbus/dbus-connection.c 21 Jul 2006 23:24:31 -0000
@@ -260,6 +260,7 @@
static void _dbus_connection_last_unref (DBusConnection *connection);
static void _dbus_connection_acquire_dispatch (DBusConnection *connection);
static void _dbus_connection_release_dispatch (DBusConnection *connection);
+static DBusDispatchStatus _dbus_connection_flush_unlocked (DBusConnection *connection);
static DBusMessageFilter *
_dbus_message_filter_ref (DBusMessageFilter *filter)
@@ -378,11 +379,11 @@
reply_serial);
if (pending != NULL)
{
- if (_dbus_pending_call_is_timeout_added (pending))
+ if (_dbus_pending_call_is_timeout_added_unlocked (pending))
_dbus_connection_remove_timeout_unlocked (connection,
- _dbus_pending_call_get_timeout (pending));
+ _dbus_pending_call_get_timeout_unlocked (pending));
- _dbus_pending_call_set_timeout_added (pending, FALSE);
+ _dbus_pending_call_set_timeout_added_unlocked (pending, FALSE);
}
}
@@ -783,11 +784,11 @@
HAVE_LOCK_CHECK (connection);
- reply_serial = _dbus_pending_call_get_reply_serial (pending);
+ reply_serial = _dbus_pending_call_get_reply_serial_unlocked (pending);
_dbus_assert (reply_serial != 0);
- timeout = _dbus_pending_call_get_timeout (pending);
+ timeout = _dbus_pending_call_get_timeout_unlocked (pending);
if (!_dbus_connection_add_timeout_unlocked (connection, timeout))
return FALSE;
@@ -802,9 +803,9 @@
return FALSE;
}
- _dbus_pending_call_set_timeout_added (pending, TRUE);
+ _dbus_pending_call_set_timeout_added_unlocked (pending, TRUE);
- dbus_pending_call_ref (pending);
+ _dbus_pending_call_ref_unlocked (pending);
HAVE_LOCK_CHECK (connection);
@@ -815,39 +816,44 @@
free_pending_call_on_hash_removal (void *data)
{
DBusPendingCall *pending;
- DBusConnection *connection;
-
+ DBusConnection *connection;
+
if (data == NULL)
return;
pending = data;
- connection = _dbus_pending_call_get_connection (pending);
+ connection = _dbus_pending_call_get_connection_unlocked (pending);
- if (connection)
+ HAVE_LOCK_CHECK (connection);
+
+ if (_dbus_pending_call_is_timeout_added_unlocked (pending))
{
- if (_dbus_pending_call_is_timeout_added (pending))
- {
- _dbus_connection_remove_timeout_unlocked (connection,
- _dbus_pending_call_get_timeout (pending));
+ _dbus_connection_remove_timeout_unlocked (connection,
+ _dbus_pending_call_get_timeout_unlocked (pending));
- _dbus_pending_call_set_timeout_added (pending, FALSE);
- }
-
- dbus_pending_call_unref (pending);
+ _dbus_pending_call_set_timeout_added_unlocked (pending, FALSE);
}
+
+ /* FIXME this is sort of dangerous and undesirable to drop the lock here, but
+ * the pending call finalizer could in principle call out to application code
+ * so we pretty much have to... some larger code reorg might be needed.
+ */
+ _dbus_connection_ref_unlocked (connection);
+ _dbus_pending_call_unref_and_unlock (pending);
+ CONNECTION_LOCK (connection);
+ _dbus_connection_unref_unlocked (connection);
}
static void
_dbus_connection_detach_pending_call_unlocked (DBusConnection *connection,
DBusPendingCall *pending)
{
- /* Can't have a destroy notifier on the pending call if we're going to do this */
-
- dbus_pending_call_ref (pending);
+ /* This ends up unlocking to call the pending call finalizer, which is unexpected to
+ * say the least.
+ */
_dbus_hash_table_remove_int (connection->pending_replies,
- _dbus_pending_call_get_reply_serial (pending));
- dbus_pending_call_unref (pending);
+ _dbus_pending_call_get_reply_serial_unlocked (pending));
}
static void
@@ -857,12 +863,14 @@
/* The idea here is to avoid finalizing the pending call
* with the lock held, since there's a destroy notifier
* in pending call that goes out to application code.
+ *
+ * There's an extra unlock inside the hash table
+ * "free pending call" function FIXME...
*/
- dbus_pending_call_ref (pending);
+ _dbus_pending_call_ref_unlocked (pending);
_dbus_hash_table_remove_int (connection->pending_replies,
- _dbus_pending_call_get_reply_serial (pending));
- CONNECTION_UNLOCK (connection);
- dbus_pending_call_unref (pending);
+ _dbus_pending_call_get_reply_serial_unlocked (pending));
+ _dbus_pending_call_unref_and_unlock (pending);
}
/**
@@ -2310,14 +2318,13 @@
DBusDispatchStatus status;
DBusPendingCall *pending = data;
- connection = _dbus_pending_call_get_connection (pending);
-
- CONNECTION_LOCK (connection);
- _dbus_pending_call_queue_timeout_error (pending,
- connection);
+ connection = _dbus_pending_call_get_connection_and_lock (pending);
+
+ _dbus_pending_call_queue_timeout_error_unlocked (pending,
+ connection);
_dbus_connection_remove_timeout_unlocked (connection,
- _dbus_pending_call_get_timeout (pending));
- _dbus_pending_call_set_timeout_added (pending, FALSE);
+ _dbus_pending_call_get_timeout_unlocked (pending));
+ _dbus_pending_call_set_timeout_added_unlocked (pending, FALSE);
_dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME);
status = _dbus_connection_get_dispatch_status_unlocked (connection);
@@ -2393,9 +2400,9 @@
return TRUE;
}
- pending = _dbus_pending_call_new (connection,
- timeout_milliseconds,
- reply_handler_timeout);
+ pending = _dbus_pending_call_new_unlocked (connection,
+ timeout_milliseconds,
+ reply_handler_timeout);
if (pending == NULL)
{
@@ -2411,7 +2418,7 @@
_dbus_message_set_serial (message, serial);
}
- if (!_dbus_pending_call_set_timeout_error (pending, message, serial))
+ if (!_dbus_pending_call_set_timeout_error_unlocked (pending, message, serial))
goto error;
/* Insert the serial in the pending replies hash;
@@ -2430,19 +2437,23 @@
}
if (pending_return)
- *pending_return = pending;
+ *pending_return = pending; /* hand off refcount */
else
{
_dbus_connection_detach_pending_call_unlocked (connection, pending);
- dbus_pending_call_unref (pending);
+ /* we still have a ref to the pending call in this case, we unref
+ * after unlocking, below
+ */
}
- _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME);
status = _dbus_connection_get_dispatch_status_unlocked (connection);
/* this calls out to user code */
_dbus_connection_update_dispatch_status_and_unlock (connection, status);
+ if (pending_return == NULL)
+ dbus_pending_call_unref (pending);
+
return TRUE;
error:
@@ -2484,37 +2495,43 @@
static void
connection_timeout_and_complete_all_pending_calls_unlocked (DBusConnection *connection)
{
- DBusHashIter iter;
-
- _dbus_hash_iter_init (connection->pending_replies, &iter);
-
- /* create list while we remove the iters from the hash
- because we need to go over it a couple of times */
- while (_dbus_hash_iter_next (&iter))
+ /* We can't iterate over the hash in the normal way since we'll be
+ * dropping the lock for each item. So we restart the
+ * iter each time as we drain the hash table.
+ */
+
+ while (_dbus_hash_table_get_n_entries (connection->pending_replies) > 0)
{
DBusPendingCall *pending;
-
+ DBusHashIter iter;
+
+ _dbus_hash_iter_init (connection->pending_replies, &iter);
+ _dbus_hash_iter_next (&iter);
+
pending = (DBusPendingCall *) _dbus_hash_iter_get_value (&iter);
- dbus_pending_call_ref (pending);
+ _dbus_pending_call_ref_unlocked (pending);
- _dbus_pending_call_queue_timeout_error (pending,
- connection);
+ _dbus_pending_call_queue_timeout_error_unlocked (pending,
+ connection);
_dbus_connection_remove_timeout_unlocked (connection,
- _dbus_pending_call_get_timeout (pending));
+ _dbus_pending_call_get_timeout_unlocked (pending));
_dbus_hash_iter_remove_entry (&iter);
- dbus_pending_call_unref (pending);
+ _dbus_pending_call_unref_and_unlock (pending);
+ CONNECTION_LOCK (connection);
}
+ HAVE_LOCK_CHECK (connection);
}
static void
-complete_pending_call_and_unlock (DBusPendingCall *pending,
+complete_pending_call_and_unlock (DBusConnection *connection,
+ DBusPendingCall *pending,
DBusMessage *message)
{
- _dbus_pending_call_set_reply (pending, message);
- dbus_pending_call_ref (pending); /* in case there's no app with a ref held */
- _dbus_connection_detach_pending_call_and_unlock (_dbus_pending_call_get_connection (pending), pending);
+ _dbus_pending_call_set_reply_unlocked (pending, message);
+ _dbus_pending_call_ref_unlocked (pending); /* in case there's no app with a ref held */
+ _dbus_connection_detach_pending_call_and_unlock (connection, pending);
/* Must be called unlocked since it invokes app callback */
_dbus_pending_call_complete (pending);
@@ -2522,23 +2539,21 @@
}
static dbus_bool_t
-check_for_reply_and_update_dispatch_unlocked (DBusPendingCall *pending)
+check_for_reply_and_update_dispatch_unlocked (DBusConnection *connection,
+ DBusPendingCall *pending)
{
DBusMessage *reply;
DBusDispatchStatus status;
- DBusConnection *connection;
-
- connection = _dbus_pending_call_get_connection (pending);
reply = check_for_reply_unlocked (connection,
- _dbus_pending_call_get_reply_serial (pending));
+ _dbus_pending_call_get_reply_serial_unlocked (pending));
if (reply != NULL)
{
_dbus_verbose ("%s checked for reply\n", _DBUS_FUNCTION_NAME);
_dbus_verbose ("dbus_connection_send_with_reply_and_block(): got reply\n");
- complete_pending_call_and_unlock (pending, reply);
+ complete_pending_call_and_unlock (connection, pending, reply);
dbus_message_unref (reply);
CONNECTION_LOCK (connection);
@@ -2604,24 +2619,21 @@
if (dbus_pending_call_get_completed (pending))
return;
- connection = _dbus_pending_call_get_connection (pending);
- if (connection == NULL)
- return; /* call already detached */
-
dbus_pending_call_ref (pending); /* necessary because the call could be canceled */
- client_serial = _dbus_pending_call_get_reply_serial (pending);
+
+ connection = _dbus_pending_call_get_connection_and_lock (pending);
+
+ /* Flush message queue - note, can affect dispatch status */
+ _dbus_connection_flush_unlocked (connection);
+
+ client_serial = _dbus_pending_call_get_reply_serial_unlocked (pending);
/* note that timeout_milliseconds is limited to a smallish value
* in _dbus_pending_call_new() so overflows aren't possible
* below
*/
- timeout_milliseconds = dbus_timeout_get_interval (_dbus_pending_call_get_timeout (pending));
-
- /* Flush message queue */
- dbus_connection_flush (connection);
-
- CONNECTION_LOCK (connection);
-
+ timeout_milliseconds = dbus_timeout_get_interval (_dbus_pending_call_get_timeout_unlocked (pending));
+
_dbus_get_current_time (&start_tv_sec, &start_tv_usec);
end_tv_sec = start_tv_sec + timeout_milliseconds / 1000;
end_tv_usec = start_tv_usec + (timeout_milliseconds % 1000) * 1000;
@@ -2636,7 +2648,7 @@
/* check to see if we already got the data off the socket */
/* from another blocked pending call */
- if (check_for_reply_and_update_dispatch_unlocked (pending))
+ if (check_for_reply_and_update_dispatch_unlocked (connection, pending))
return;
/* Now we wait... */
@@ -2659,7 +2671,7 @@
/* the get_completed() is in case a dispatch() while we were blocking
* got the reply instead of us.
*/
- if (dbus_pending_call_get_completed (pending))
+ if (_dbus_pending_call_get_completed_unlocked (pending))
{
_dbus_verbose ("Pending call completed by dispatch in %s\n", _DBUS_FUNCTION_NAME);
_dbus_connection_update_dispatch_status_and_unlock (connection, status);
@@ -2667,9 +2679,10 @@
return;
}
- if (status == DBUS_DISPATCH_DATA_REMAINS)
- if (check_for_reply_and_update_dispatch_unlocked (pending))
- return;
+ if (status == DBUS_DISPATCH_DATA_REMAINS) {
+ if (check_for_reply_and_update_dispatch_unlocked (connection, pending))
+ return;
+ }
_dbus_get_current_time (&tv_sec, &tv_usec);
@@ -2680,7 +2693,7 @@
* confusing
*/
- complete_pending_call_and_unlock (pending, NULL);
+ complete_pending_call_and_unlock (connection, pending, NULL);
dbus_pending_call_unref (pending);
return;
}
@@ -2721,10 +2734,10 @@
_dbus_verbose ("dbus_connection_send_with_reply_and_block(): Waited %ld milliseconds and got no reply\n",
(tv_sec - start_tv_sec) * 1000 + (tv_usec - start_tv_usec) / 1000);
- _dbus_assert (!dbus_pending_call_get_completed (pending));
+ _dbus_assert (!_dbus_pending_call_get_completed_unlocked (pending));
/* unlock and call user code */
- complete_pending_call_and_unlock (pending, NULL);
+ complete_pending_call_and_unlock (connection, pending, NULL);
/* update user code on dispatch status */
CONNECTION_LOCK (connection);
@@ -2799,11 +2812,14 @@
/**
* Blocks until the outgoing message queue is empty.
+ * Assumes connection lock already held.
*
+ * If you call this, you MUST call update_dispatch_status afterword...
+ *
* @param connection the connection.
*/
-void
-dbus_connection_flush (DBusConnection *connection)
+DBusDispatchStatus
+_dbus_connection_flush_unlocked (DBusConnection *connection)
{
/* We have to specify DBUS_ITERATION_DO_READING here because
* otherwise we could have two apps deadlock if they are both doing
@@ -2812,9 +2828,8 @@
*/
DBusDispatchStatus status;
- _dbus_return_if_fail (connection != NULL);
+ HAVE_LOCK_CHECK (connection);
- CONNECTION_LOCK (connection);
while (connection->n_outgoing > 0 &&
_dbus_connection_get_is_connected_unlocked (connection))
{
@@ -2832,6 +2847,31 @@
status = _dbus_connection_get_dispatch_status_unlocked (connection);
HAVE_LOCK_CHECK (connection);
+ return status;
+}
+
+/**
+ * Blocks until the outgoing message queue is empty.
+ *
+ * @param connection the connection.
+ */
+void
+dbus_connection_flush (DBusConnection *connection)
+{
+ /* We have to specify DBUS_ITERATION_DO_READING here because
+ * otherwise we could have two apps deadlock if they are both doing
+ * a flush(), and the kernel buffers fill up. This could change the
+ * dispatch status.
+ */
+ DBusDispatchStatus status;
+
+ _dbus_return_if_fail (connection != NULL);
+
+ CONNECTION_LOCK (connection);
+
+ status = _dbus_connection_flush_unlocked (connection);
+
+ HAVE_LOCK_CHECK (connection);
/* Unlocks and calls out to user code */
_dbus_connection_update_dispatch_status_and_unlock (connection, status);
@@ -3588,7 +3628,7 @@
if (pending)
{
_dbus_verbose ("Dispatching a pending reply\n");
- complete_pending_call_and_unlock (pending, message);
+ complete_pending_call_and_unlock (connection, pending, message);
pending = NULL; /* it's probably unref'd */
CONNECTION_LOCK (connection);
Index: dbus/dbus-errors.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-errors.c,v
retrieving revision 1.29
diff -u -r1.29 dbus-errors.c
--- dbus/dbus-errors.c 17 Jul 2006 17:44:07 -0000 1.29
+++ dbus/dbus-errors.c 21 Jul 2006 23:24:31 -0000
@@ -40,7 +40,7 @@
*/
typedef struct
{
- const char *name; /**< error name */
+ char *name; /**< error name */
char *message; /**< error message */
unsigned int const_message : 1; /**< Message is not owned by DBusError */
@@ -219,7 +219,7 @@
real = (DBusRealError *)error;
- real->name = name;
+ real->name = (char*) name;
real->message = (char *)message;
real->const_message = TRUE;
}
Index: dbus/dbus-pending-call-internal.h
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-pending-call-internal.h,v
retrieving revision 1.2
diff -u -r1.2 dbus-pending-call-internal.h
--- dbus/dbus-pending-call-internal.h 14 Jul 2006 03:09:22 -0000 1.2
+++ dbus/dbus-pending-call-internal.h 21 Jul 2006 23:24:31 -0000
@@ -31,30 +31,35 @@
DBUS_BEGIN_DECLS
-dbus_bool_t _dbus_pending_call_is_timeout_added (DBusPendingCall *pending);
-void _dbus_pending_call_set_timeout_added (DBusPendingCall *pending,
- dbus_bool_t is_added);
-DBusTimeout *_dbus_pending_call_get_timeout (DBusPendingCall *pending);
-dbus_uint32_t _dbus_pending_call_get_reply_serial (DBusPendingCall *pending);
-void _dbus_pending_call_set_reply_serial (DBusPendingCall *pending,
- dbus_uint32_t serial);
-DBusConnection *_dbus_pending_call_get_connection (DBusPendingCall *pending);
-
-void _dbus_pending_call_complete (DBusPendingCall *pending);
-void _dbus_pending_call_set_reply (DBusPendingCall *pending,
- DBusMessage *message);
-void _dbus_pending_call_clear_connection (DBusPendingCall *pending);
-
-void _dbus_pending_call_queue_timeout_error (DBusPendingCall *pending,
- DBusConnection *connection);
-void _dbus_pending_call_set_reply_serial (DBusPendingCall *pending,
- dbus_uint32_t serial);
-dbus_bool_t _dbus_pending_call_set_timeout_error (DBusPendingCall *pending,
- DBusMessage *message,
- dbus_uint32_t serial);
-DBusPendingCall* _dbus_pending_call_new (DBusConnection *connection,
- int timeout_milliseconds,
- DBusTimeoutHandler timeout_handler);
+dbus_bool_t _dbus_pending_call_is_timeout_added_unlocked (DBusPendingCall *pending);
+void _dbus_pending_call_set_timeout_added_unlocked (DBusPendingCall *pending,
+ dbus_bool_t is_added);
+DBusTimeout * _dbus_pending_call_get_timeout_unlocked (DBusPendingCall *pending);
+dbus_uint32_t _dbus_pending_call_get_reply_serial_unlocked (DBusPendingCall *pending);
+void _dbus_pending_call_set_reply_serial_unlocked (DBusPendingCall *pending,
+ dbus_uint32_t serial);
+DBusConnection * _dbus_pending_call_get_connection_and_lock (DBusPendingCall *pending);
+DBusConnection * _dbus_pending_call_get_connection_unlocked (DBusPendingCall *pending);
+dbus_bool_t _dbus_pending_call_get_completed_unlocked (DBusPendingCall *pending);
+void _dbus_pending_call_complete (DBusPendingCall *pending);
+void _dbus_pending_call_set_reply_unlocked (DBusPendingCall *pending,
+ DBusMessage *message);
+void _dbus_pending_call_queue_timeout_error_unlocked (DBusPendingCall *pending,
+ DBusConnection *connection);
+void _dbus_pending_call_set_reply_serial_unlocked (DBusPendingCall *pending,
+ dbus_uint32_t serial);
+dbus_bool_t _dbus_pending_call_set_timeout_error_unlocked (DBusPendingCall *pending,
+ DBusMessage *message,
+ dbus_uint32_t serial);
+DBusPendingCall* _dbus_pending_call_new_unlocked (DBusConnection *connection,
+ int timeout_milliseconds,
+ DBusTimeoutHandler timeout_handler);
+DBusPendingCall* _dbus_pending_call_ref_unlocked (DBusPendingCall *pending);
+void _dbus_pending_call_unref_and_unlock (DBusPendingCall *pending);
+dbus_bool_t _dbus_pending_call_set_data_unlocked (DBusPendingCall *pending,
+ dbus_int32_t slot,
+ void *data,
+ DBusFreeFunction free_data_func);
DBUS_END_DECLS
Index: dbus/dbus-pending-call.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-pending-call.c,v
retrieving revision 1.15
diff -u -r1.15 dbus-pending-call.c
--- dbus/dbus-pending-call.c 21 Jul 2006 19:28:56 -0000 1.15
+++ dbus/dbus-pending-call.c 21 Jul 2006 23:24:32 -0000
@@ -44,6 +44,9 @@
*
* Opaque object representing a reply message that we're waiting for.
*/
+#define CONNECTION_LOCK(connection) _dbus_connection_lock(connection)
+#define CONNECTION_UNLOCK(connection) _dbus_connection_unlock(connection)
+
struct DBusPendingCall
{
DBusAtomic refcount; /**< reference count */
@@ -75,9 +78,9 @@
* @returns a new #DBusPendingCall or #NULL if no memory.
*/
DBusPendingCall*
-_dbus_pending_call_new (DBusConnection *connection,
- int timeout_milliseconds,
- DBusTimeoutHandler timeout_handler)
+_dbus_pending_call_new_unlocked (DBusConnection *connection,
+ int timeout_milliseconds,
+ DBusTimeoutHandler timeout_handler)
{
DBusPendingCall *pending;
DBusTimeout *timeout;
@@ -138,8 +141,8 @@
* to time out the call
*/
void
-_dbus_pending_call_set_reply (DBusPendingCall *pending,
- DBusMessage *message)
+_dbus_pending_call_set_reply_unlocked (DBusPendingCall *pending,
+ DBusMessage *message)
{
if (message == NULL)
{
@@ -187,9 +190,11 @@
}
void
-_dbus_pending_call_queue_timeout_error (DBusPendingCall *pending,
- DBusConnection *connection)
+_dbus_pending_call_queue_timeout_error_unlocked (DBusPendingCall *pending,
+ DBusConnection *connection)
{
+ _dbus_assert (connection == pending->connection);
+
if (pending->timeout_link)
{
_dbus_connection_queue_synthesized_message_link (connection,
@@ -205,7 +210,7 @@
* @returns #TRUE if there is a timeout or #FALSE if not
*/
dbus_bool_t
-_dbus_pending_call_is_timeout_added (DBusPendingCall *pending)
+_dbus_pending_call_is_timeout_added_unlocked (DBusPendingCall *pending)
{
_dbus_assert (pending != NULL);
@@ -220,8 +225,8 @@
* @param is_added whether or not a timeout is added
*/
void
-_dbus_pending_call_set_timeout_added (DBusPendingCall *pending,
- dbus_bool_t is_added)
+_dbus_pending_call_set_timeout_added_unlocked (DBusPendingCall *pending,
+ dbus_bool_t is_added)
{
_dbus_assert (pending != NULL);
@@ -236,7 +241,7 @@
* @returns a timeout object
*/
DBusTimeout *
-_dbus_pending_call_get_timeout (DBusPendingCall *pending)
+_dbus_pending_call_get_timeout_unlocked (DBusPendingCall *pending)
{
_dbus_assert (pending != NULL);
@@ -250,7 +255,7 @@
* @returns a serial number for the reply or 0
*/
dbus_uint32_t
-_dbus_pending_call_get_reply_serial (DBusPendingCall *pending)
+_dbus_pending_call_get_reply_serial_unlocked (DBusPendingCall *pending)
{
_dbus_assert (pending != NULL);
@@ -264,8 +269,8 @@
* @param serial the serial number
*/
void
-_dbus_pending_call_set_reply_serial (DBusPendingCall *pending,
- dbus_uint32_t serial)
+_dbus_pending_call_set_reply_serial_unlocked (DBusPendingCall *pending,
+ dbus_uint32_t serial)
{
_dbus_assert (pending != NULL);
_dbus_assert (pending->reply_serial == 0);
@@ -274,17 +279,32 @@
}
/**
- * Gets the connection associated with this pending call
+ * Gets the connection associated with this pending call.
*
* @param pending the pending_call
* @returns the connection associated with the pending call
*/
DBusConnection *
-_dbus_pending_call_get_connection (DBusPendingCall *pending)
+_dbus_pending_call_get_connection_and_lock (DBusPendingCall *pending)
{
_dbus_assert (pending != NULL);
- return pending->connection;
+ CONNECTION_LOCK (pending->connection);
+ return pending->connection;
+}
+
+/**
+ * Gets the connection associated with this pending call.
+ *
+ * @param pending the pending_call
+ * @returns the connection associated with the pending call
+ */
+DBusConnection *
+_dbus_pending_call_get_connection_unlocked (DBusPendingCall *pending)
+{
+ _dbus_assert (pending != NULL);
+
+ return pending->connection;
}
/**
@@ -296,9 +316,9 @@
* @return #FALSE on OOM
*/
dbus_bool_t
-_dbus_pending_call_set_timeout_error (DBusPendingCall *pending,
- DBusMessage *message,
- dbus_uint32_t serial)
+_dbus_pending_call_set_timeout_error_unlocked (DBusPendingCall *pending,
+ DBusMessage *message,
+ dbus_uint32_t serial)
{
DBusList *reply_link;
DBusMessage *reply;
@@ -321,7 +341,7 @@
pending->timeout_link = reply_link;
- _dbus_pending_call_set_reply_serial (pending, serial);
+ _dbus_pending_call_set_reply_serial_unlocked (pending, serial);
return TRUE;
}
@@ -347,6 +367,21 @@
*/
/**
+ * Increments the reference count on a pending call,
+ * while the lock on its connection is already held.
+ *
+ * @param pending the pending call object
+ * @returns the pending call object
+ */
+DBusPendingCall *
+_dbus_pending_call_ref_unlocked (DBusPendingCall *pending)
+{
+ pending->refcount.value += 1;
+
+ return pending;
+}
+
+/**
* Increments the reference count on a pending call.
*
* @param pending the pending call object
@@ -357,11 +392,87 @@
{
_dbus_return_val_if_fail (pending != NULL, NULL);
+ /* The connection lock is better than the global
+ * lock in the atomic increment fallback
+ */
+#ifdef DBUS_HAVE_ATOMIC_INT
_dbus_atomic_inc (&pending->refcount);
-
+#else
+ CONNECTION_LOCK (pending->connection);
+ _dbus_assert (pending->refcount.value > 0);
+
+ pending->refcount.value += 1;
+ CONNECTION_UNLOCK (pending->connection);
+#endif
+
return pending;
}
+static void
+_dbus_pending_call_last_unref (DBusPendingCall *pending)
+{
+ DBusConnection *connection;
+
+ /* If we get here, we should be already detached
+ * from the connection, or never attached.
+ */
+ _dbus_assert (!pending->timeout_added);
+
+ connection = pending->connection;
+
+ /* this assumes we aren't holding connection lock... */
+ _dbus_data_slot_list_free (&pending->slot_list);
+
+ if (pending->timeout != NULL)
+ _dbus_timeout_unref (pending->timeout);
+
+ if (pending->timeout_link)
+ {
+ dbus_message_unref ((DBusMessage *)pending->timeout_link->data);
+ _dbus_list_free_link (pending->timeout_link);
+ pending->timeout_link = NULL;
+ }
+
+ if (pending->reply)
+ {
+ dbus_message_unref (pending->reply);
+ pending->reply = NULL;
+ }
+
+ dbus_free (pending);
+
+ dbus_pending_call_free_data_slot (¬ify_user_data_slot);
+
+ /* connection lock should not be held. */
+ /* Free the connection last to avoid a weird state while
+ * calling out to application code where the pending exists
+ * but not the connection.
+ */
+ dbus_connection_unref (connection);
+}
+
+/**
+ * Decrements the reference count on a pending call,
+ * freeing it if the count reaches 0. Assumes
+ * connection lock is already held.
+ *
+ * @param pending the pending call object
+ */
+void
+_dbus_pending_call_unref_and_unlock (DBusPendingCall *pending)
+{
+ dbus_bool_t last_unref;
+
+ _dbus_assert (pending->refcount.value > 0);
+
+ pending->refcount.value -= 1;
+ last_unref = pending->refcount.value == 0;
+
+ CONNECTION_UNLOCK (pending->connection);
+ if (last_unref)
+ _dbus_pending_call_last_unref (pending);
+}
+
/**
* Decrements the reference count on a pending call,
* freeing it if the count reaches 0.
@@ -375,40 +486,21 @@
_dbus_return_if_fail (pending != NULL);
+ /* More efficient to use the connection lock instead of atomic
+ * int fallback if we lack atomic int decrement
+ */
+#ifdef DBUS_HAVE_ATOMIC_INT
last_unref = (_dbus_atomic_dec (&pending->refcount) == 1);
-
+#else
+ CONNECTION_LOCK (pending->connection);
+ _dbus_assert (pending->refcount.value > 0);
+ pending->refcount.value -= 1;
+ last_unref = pending->refcount.value == 0;
+ CONNECTION_UNLOCK (pending->connection);
+#endif
+
if (last_unref)
- {
- /* If we get here, we should be already detached
- * from the connection, or never attached.
- */
- _dbus_assert (!pending->timeout_added);
-
- dbus_connection_unref (pending->connection);
-
- /* this assumes we aren't holding connection lock... */
- _dbus_data_slot_list_free (&pending->slot_list);
-
- if (pending->timeout != NULL)
- _dbus_timeout_unref (pending->timeout);
-
- if (pending->timeout_link)
- {
- dbus_message_unref ((DBusMessage *)pending->timeout_link->data);
- _dbus_list_free_link (pending->timeout_link);
- pending->timeout_link = NULL;
- }
-
- if (pending->reply)
- {
- dbus_message_unref (pending->reply);
- pending->reply = NULL;
- }
-
- dbus_free (pending);
-
- dbus_pending_call_free_data_slot (¬ify_user_data_slot);
- }
+ _dbus_pending_call_last_unref(pending);
}
/**
@@ -429,13 +521,17 @@
{
_dbus_return_val_if_fail (pending != NULL, FALSE);
+ CONNECTION_LOCK (pending->connection);
+
/* could invoke application code! */
- if (!dbus_pending_call_set_data (pending, notify_user_data_slot,
- user_data, free_user_data))
+ if (!_dbus_pending_call_set_data_unlocked (pending, notify_user_data_slot,
+ user_data, free_user_data))
return FALSE;
pending->function = function;
+ CONNECTION_UNLOCK (pending->connection);
+
return TRUE;
}
@@ -451,23 +547,40 @@
void
dbus_pending_call_cancel (DBusPendingCall *pending)
{
- if (pending->connection)
- _dbus_connection_remove_pending_call (pending->connection,
- pending);
+ _dbus_connection_remove_pending_call (pending->connection,
+ pending);
}
/**
* Checks whether the pending call has received a reply
- * yet, or not.
+ * yet, or not. Assumes connection lock is held.
*
- * @todo not thread safe? I guess it has to lock though it sucks
+ * @param pending the pending call
+ * @returns #TRUE if a reply has been received
+ */
+dbus_bool_t
+_dbus_pending_call_get_completed_unlocked (DBusPendingCall *pending)
+{
+ return pending->completed;
+}
+
+/**
+ * Checks whether the pending call has received a reply
+ * yet, or not.
*
* @param pending the pending call
- * @returns #TRUE if a reply has been received */
+ * @returns #TRUE if a reply has been received
+ */
dbus_bool_t
dbus_pending_call_get_completed (DBusPendingCall *pending)
{
- return pending->completed;
+ dbus_bool_t completed;
+
+ CONNECTION_LOCK (pending->connection);
+ completed = pending->completed;
+ CONNECTION_UNLOCK (pending->connection);
+
+ return completed;
}
/**
@@ -486,10 +599,14 @@
_dbus_return_val_if_fail (pending->completed, NULL);
_dbus_return_val_if_fail (pending->reply != NULL, NULL);
+
+ CONNECTION_LOCK (pending->connection);
message = pending->reply;
pending->reply = NULL;
+ CONNECTION_UNLOCK (pending->connection);
+
return message;
}
@@ -553,7 +670,7 @@
dbus_pending_call_free_data_slot (dbus_int32_t *slot_p)
{
_dbus_return_if_fail (*slot_p >= 0);
-
+
_dbus_data_slot_allocator_free (&slot_allocator, slot_p);
}
@@ -571,29 +688,62 @@
* @returns #TRUE if there was enough memory to store the data
*/
dbus_bool_t
-dbus_pending_call_set_data (DBusPendingCall *pending,
- dbus_int32_t slot,
- void *data,
- DBusFreeFunction free_data_func)
+_dbus_pending_call_set_data_unlocked (DBusPendingCall *pending,
+ dbus_int32_t slot,
+ void *data,
+ DBusFreeFunction free_data_func)
{
DBusFreeFunction old_free_func;
void *old_data;
dbus_bool_t retval;
- _dbus_return_val_if_fail (pending != NULL, FALSE);
- _dbus_return_val_if_fail (slot >= 0, FALSE);
-
retval = _dbus_data_slot_list_set (&slot_allocator,
&pending->slot_list,
slot, data, free_data_func,
&old_free_func, &old_data);
+ /* Drop locks to call out to app code */
+ CONNECTION_UNLOCK (pending->connection);
+
if (retval)
{
if (old_free_func)
(* old_free_func) (old_data);
}
+ CONNECTION_LOCK (pending->connection);
+
+ return retval;
+}
+
+/**
+ * Stores a pointer on a #DBusPendingCall, along
+ * with an optional function to be used for freeing
+ * the data when the data is set again, or when
+ * the pending call is finalized. The slot number
+ * must have been allocated with dbus_pending_call_allocate_data_slot().
+ *
+ * @param pending the pending_call
+ * @param slot the slot number
+ * @param data the data to store
+ * @param free_data_func finalizer function for the data
+ * @returns #TRUE if there was enough memory to store the data
+ */
+dbus_bool_t
+dbus_pending_call_set_data (DBusPendingCall *pending,
+ dbus_int32_t slot,
+ void *data,
+ DBusFreeFunction free_data_func)
+{
+ dbus_bool_t retval;
+
+ _dbus_return_val_if_fail (pending != NULL, FALSE);
+ _dbus_return_val_if_fail (slot >= 0, FALSE);
+
+
+ CONNECTION_LOCK (pending->connection);
+ retval = _dbus_pending_call_set_data_unlocked (pending, slot, data, free_data_func);
+ CONNECTION_UNLOCK (pending->connection);
return retval;
}
@@ -613,9 +763,11 @@
_dbus_return_val_if_fail (pending != NULL, NULL);
+ CONNECTION_LOCK (pending->connection);
res = _dbus_data_slot_list_get (&slot_allocator,
&pending->slot_list,
slot);
+ CONNECTION_UNLOCK (pending->connection);
return res;
}
Index: doc/TODO
===================================================================
RCS file: /cvs/dbus/dbus/doc/TODO,v
retrieving revision 1.103
diff -u -r1.103 TODO
--- doc/TODO 21 Jul 2006 23:21:54 -0000 1.103
+++ doc/TODO 21 Jul 2006 23:24:32 -0000
@@ -21,8 +21,6 @@
- publish the introspection dtd at its URL
- - fix locking on DBusPendingCall
-
Important for 1.0 GLib Bindings
===
More information about the dbus
mailing list