[PATCH] Privatize the DBusPendingCall struct
John (J5) Palmieri
johnp at redhat.com
Mon Jul 24 12:04:23 PDT 2006
I just did a release that fixes the make check errors and taking a ref
on a locked connection issue. I'm going to take a look at this patch
soon.
Havoc Pennington wrote:
> 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
>
>
> ------------------------------------------------------------------------
>
> ? 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
> ===
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> dbus mailing list
> dbus at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dbus
>
More information about the dbus
mailing list