Attachment 136956 Details for Bug 102839 – [2/3] DBusPendingCall: Only update ->completed under the connection lock

[patch] [2/3] DBusPendingCall: Only update ->completed under the connection lock

0002-DBusPendingCall-Only-update-completed-under-the-conn.patch (text/plain), 4.07 KB, created by Simon McVittie on 2018-01-25 13:22:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2018-01-25 13:22:39 UTC

Size: 4.07 KB

patch

obsolete

>From 5c66205723d16659cd6a4131c22a7e5ac57c6b5d Mon Sep 17 00:00:00 2001
>From: Manish Narang <Manish.Narang@kpit.com>
>Date: Thu, 25 Jan 2018 11:39:44 +0000
>Subject: [PATCH 2/3] DBusPendingCall: Only update ->completed under the
> connection lock
>
>If one thread is blocking on a pending call, and another thread is
>dispatching the connection, then we need them to agree on the value
>of the completed flag by protecting all accesses with a lock. Reads
>for this member seem to have the connection lock already, so it's
>sufficient to make sure that the only write also happens under the
>connection lock.
>
>We already set the completed flag before calling the callback, so it
>seems OK to stretch it to meaning that some thread has merely *taken
>responsibility for* calling the callback.
>
>The completed flag shares a bitfield with timeout_added, but that
>flag is protected by the connection lock already.
>
>Based on suggestions from Simon McVittie on
><https://bugs.freedesktop.org/show_bug.cgi?id=102839>.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=102839
>[smcv: Revert indentation changes; add commit message]
>Reviewed-by: Simon McVittie <smcv@collabora.com>
>---
> dbus/dbus-connection.c            |  5 +++--
> dbus/dbus-pending-call-internal.h |  5 ++++-
> dbus/dbus-pending-call.c          | 17 +++++++++++++----
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index 2b0a5507..c525b6dc 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -2325,10 +2325,11 @@ complete_pending_call_and_unlock (DBusConnection  *connection,
> {
>   _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_pending_call_start_completion_unlocked(pending);
>   _dbus_connection_detach_pending_call_and_unlock (connection, pending);
>- 
>+
>   /* Must be called unlocked since it invokes app callback */
>-  _dbus_pending_call_complete (pending);
>+  _dbus_pending_call_finish_completion (pending);
>   dbus_pending_call_unref (pending);
> }
> 
>diff --git a/dbus/dbus-pending-call-internal.h b/dbus/dbus-pending-call-internal.h
>index a6d7fba7..4f379b4f 100644
>--- a/dbus/dbus-pending-call-internal.h
>+++ b/dbus/dbus-pending-call-internal.h
>@@ -41,7 +41,10 @@ void             _dbus_pending_call_set_reply_serial_unlocked    (DBusPendingCal
> 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_start_completion_unlocked    (DBusPendingCall    *pending);
>+void             _dbus_pending_call_finish_completion            (DBusPendingCall    *pending);
>+
> void             _dbus_pending_call_set_reply_unlocked           (DBusPendingCall    *pending,
>                                                                   DBusMessage        *message);
> void             _dbus_pending_call_queue_timeout_error_unlocked (DBusPendingCall    *pending,
>diff --git a/dbus/dbus-pending-call.c b/dbus/dbus-pending-call.c
>index be534105..1bc5d1e5 100644
>--- a/dbus/dbus-pending-call.c
>+++ b/dbus/dbus-pending-call.c
>@@ -194,18 +194,27 @@ _dbus_pending_call_set_reply_unlocked (DBusPendingCall *pending,
> }
> 
> /**
>- * Calls notifier function for the pending call
>- * and sets the call to completed.
>+ * Sets the pending call to completed
>  *
>  * @param pending the pending call
>- * 
>  */
> void
>-_dbus_pending_call_complete (DBusPendingCall *pending)
>+_dbus_pending_call_start_completion_unlocked (DBusPendingCall *pending)
> {
>   _dbus_assert (!pending->completed);
>   
>   pending->completed = TRUE;
>+}
>+
>+/**
>+ * Call the notifier function for the pending call.
>+ *
>+ * @param pending the pending call
>+ */
>+void
>+_dbus_pending_call_finish_completion (DBusPendingCall *pending)
>+{
>+  _dbus_assert (pending->completed);
> 
>   if (pending->function)
>     {
>-- 
>2.15.1
>

Actions: View | Splinter Review

Attachments on bug 102839: 134315 | 134343 | 134357 | 134467 | 136953 | 136954 | 136955 | 136956 | 136957 | 136959 | 137120 | 137121 | 137122 | 137190 | 137191 | 137200

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.