Attachment 85113 Details for Bug 68610 – _dbus_modify_sigpipe: be thread-safe

[patch] _dbus_modify_sigpipe: be thread-safe

0001-_dbus_modify_sigpipe-be-thread-safe.patch (text/plain), 7.58 KB, created by Simon McVittie on 2013-09-03 12:29:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-09-03 12:29:22 UTC

Size: 7.58 KB

patch

obsolete

>From b35d0d30f520fd24109b73f7cb977666c12c8b3b Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 3 Sep 2013 13:28:49 +0100
>Subject: [PATCH] _dbus_modify_sigpipe: be thread-safe
>
>This needs new atomic primitives: we don't have "set to a value",
>and in fact that's a bit annoying to implement in terms of gcc
>intrinsics. "Set to 0" and "set to nonzero" are easy, though.
>Also add a trivial sanity-check for the atomic primitives (it doesn't
>verify that they're atomic, but does verify that they return the right
>things).
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=68610
>---
> dbus/dbus-connection.c   | 13 +++++----
> dbus/dbus-sysdeps-unix.c | 36 +++++++++++++++++++++++++
> dbus/dbus-sysdeps-win.c  | 22 ++++++++++++++++
> dbus/dbus-sysdeps.h      |  2 ++
> test/Makefile.am         |  5 ++++
> test/internals/atomic.c  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 141 insertions(+), 5 deletions(-)
> create mode 100644 test/internals/atomic.c
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index 759a649..f3718dd 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -245,9 +245,9 @@ struct DBusPreallocatedSend
> };
> 
> #if HAVE_DECL_MSG_NOSIGNAL
>-static dbus_bool_t _dbus_modify_sigpipe = FALSE;
>+static DBusAtomic _dbus_modify_sigpipe = { FALSE };
> #else
>-static dbus_bool_t _dbus_modify_sigpipe = TRUE;
>+static DBusAtomic _dbus_modify_sigpipe = { TRUE };
> #endif
> 
> /**
>@@ -1328,7 +1328,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
>   if (objects == NULL)
>     goto error;
>   
>-  if (_dbus_modify_sigpipe)
>+  if (_dbus_atomic_get (&_dbus_modify_sigpipe) != 0)
>     _dbus_disable_sigpipe ();
> 
>   /* initialized to 0: use atomic op to avoid mixing atomic and non-atomic */
>@@ -6014,8 +6014,11 @@ dbus_connection_get_data (DBusConnection   *connection,
>  */
> void
> dbus_connection_set_change_sigpipe (dbus_bool_t will_modify_sigpipe)
>-{  
>-  _dbus_modify_sigpipe = will_modify_sigpipe != FALSE;
>+{
>+  if (will_modify_sigpipe)
>+    _dbus_atomic_set_nonzero (&_dbus_modify_sigpipe);
>+  else
>+    _dbus_atomic_set_zero (&_dbus_modify_sigpipe);
> }
> 
> /**
>diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
>index 1cb4a58..7ac3b93 100644
>--- a/dbus/dbus-sysdeps-unix.c
>+++ b/dbus/dbus-sysdeps-unix.c
>@@ -2511,6 +2511,42 @@ _dbus_atomic_get (DBusAtomic *atomic)
> }
> 
> /**
>+ * Atomically set the value of an integer to 0.
>+ *
>+ * @param atomic pointer to the integer to set
>+ */
>+void
>+_dbus_atomic_set_zero (DBusAtomic *atomic)
>+{
>+#if DBUS_USE_SYNC
>+  /* Atomic version of "*atomic &= 0; return *atomic" */
>+  __sync_and_and_fetch (&atomic->value, 0);
>+#else
>+  pthread_mutex_lock (&atomic_mutex);
>+  atomic->value = 0;
>+  pthread_mutex_unlock (&atomic_mutex);
>+#endif
>+}
>+
>+/**
>+ * Atomically set the value of an integer to something nonzero.
>+ *
>+ * @param atomic pointer to the integer to set
>+ */
>+void
>+_dbus_atomic_set_nonzero (DBusAtomic *atomic)
>+{
>+#if DBUS_USE_SYNC
>+  /* Atomic version of "*atomic |= 1; return *atomic" */
>+  __sync_or_and_fetch (&atomic->value, 1);
>+#else
>+  pthread_mutex_lock (&atomic_mutex);
>+  atomic->value = 1;
>+  pthread_mutex_unlock (&atomic_mutex);
>+#endif
>+}
>+
>+/**
>  * Wrapper for poll().
>  *
>  * @param fds the file descriptors to poll
>diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
>index 9c4a06b..22cefc3 100644
>--- a/dbus/dbus-sysdeps-win.c
>+++ b/dbus/dbus-sysdeps-win.c
>@@ -3247,6 +3247,28 @@ _dbus_atomic_get (DBusAtomic *atomic)
> }
> 
> /**
>+ * Atomically set the value of an integer to 0.
>+ *
>+ * @param atomic pointer to the integer to set
>+ */
>+void
>+_dbus_atomic_set_zero (DBusAtomic *atomic)
>+{
>+  InterlockedExchange (&atomic->value, 0);
>+}
>+
>+/**
>+ * Atomically set the value of an integer to something nonzero.
>+ *
>+ * @param atomic pointer to the integer to set
>+ */
>+void
>+_dbus_atomic_set_nonzero (DBusAtomic *atomic)
>+{
>+  InterlockedExchange (&atomic->value, 1);
>+}
>+
>+/**
>  * Called when the bus daemon is signaled to reload its configuration; any
>  * caches should be nuked. Of course any caches that need explicit reload
>  * are probably broken, but c'est la vie.
>diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
>index e586946..8b48d92 100644
>--- a/dbus/dbus-sysdeps.h
>+++ b/dbus/dbus-sysdeps.h
>@@ -246,6 +246,8 @@ struct DBusAtomic
> dbus_int32_t _dbus_atomic_inc (DBusAtomic *atomic);
> dbus_int32_t _dbus_atomic_dec (DBusAtomic *atomic);
> dbus_int32_t _dbus_atomic_get (DBusAtomic *atomic);
>+void         _dbus_atomic_set_zero    (DBusAtomic *atomic);
>+void         _dbus_atomic_set_nonzero (DBusAtomic *atomic);
> 
> 
> /* AIX uses different values for poll */
>diff --git a/test/Makefile.am b/test/Makefile.am
>index 405aa1c..bff342d 100644
>--- a/test/Makefile.am
>+++ b/test/Makefile.am
>@@ -105,6 +105,10 @@ shell_test_LDADD = libdbus-testutils-internal.la
> spawn_test_CPPFLAGS = $(static_cppflags)
> spawn_test_LDADD = $(top_builddir)/dbus/libdbus-internal.la
> 
>+test_atomic_SOURCES = internals/atomic.c
>+test_atomic_CPPFLAGS = $(static_cppflags)
>+test_atomic_LDADD = $(top_builddir)/dbus/libdbus-internal.la
>+
> test_printf_SOURCES = internals/printf.c
> test_printf_CPPFLAGS = $(static_cppflags)
> test_printf_LDADD = $(top_builddir)/dbus/libdbus-internal.la
>@@ -125,6 +129,7 @@ testexec_PROGRAMS =
> 
> installable_tests = \
> 	shell-test \
>+	test-atomic \
> 	test-printf \
> 	$(NULL)
> installable_manual_tests = \
>diff --git a/test/internals/atomic.c b/test/internals/atomic.c
>new file mode 100644
>index 0000000..9e3276e
>--- /dev/null
>+++ b/test/internals/atomic.c
>@@ -0,0 +1,68 @@
>+/* Regression test for atomic ops
>+ *
>+ * Author: Simon McVittie <simon.mcvittie@collabora.co.uk>
>+ * Copyright Â© 2013 Collabora Ltd.
>+ *
>+ * Permission is hereby granted, free of charge, to any person
>+ * obtaining a copy of this software and associated documentation files
>+ * (the "Software"), to deal in the Software without restriction,
>+ * including without limitation the rights to use, copy, modify, merge,
>+ * publish, distribute, sublicense, and/or sell copies of the Software,
>+ * and to permit persons to whom the Software is furnished to do so,
>+ * subject to the following conditions:
>+ *
>+ * The above copyright notice and this permission notice shall be
>+ * included in all copies or substantial portions of the Software.
>+ *
>+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>+ * SOFTWARE.
>+ */
>+
>+#include <config.h>
>+
>+#include <dbus/dbus.h>
>+#include <dbus/dbus-internals.h>
>+#include <dbus/dbus-sysdeps.h>
>+
>+int
>+main (int argc,
>+    char **argv)
>+{
>+  DBusAtomic a = { 0 };
>+  DBusAtomic b = { 123 };
>+  dbus_int32_t old;
>+
>+  _dbus_assert (_dbus_atomic_get (&a) == 0);
>+  _dbus_assert (_dbus_atomic_get (&b) == 123);
>+
>+  _dbus_assert (a.value == 0);
>+  old = _dbus_atomic_dec (&a);
>+  _dbus_assert (old == 0);
>+  _dbus_assert (a.value == -1);
>+
>+  _dbus_assert (b.value == 123);
>+  old = _dbus_atomic_inc (&b);
>+  _dbus_assert (old == 123);
>+  _dbus_assert (b.value == 124);
>+
>+  _dbus_atomic_set_nonzero (&a);
>+  /* careful: this is not necessarily 1 */
>+  _dbus_assert (a.value != 0);
>+
>+  _dbus_atomic_set_nonzero (&b);
>+  _dbus_assert (b.value != 0);
>+
>+  _dbus_atomic_set_zero (&a);
>+  _dbus_assert (a.value == 0);
>+
>+  _dbus_atomic_set_zero (&b);
>+  _dbus_assert (b.value == 0);
>+
>+  return 0;
>+}
>-- 
>1.8.4.rc3
>

Actions: View | Splinter Review

Attachments on bug 68610: 84714 | 84715 | 84830 | 84831 | 84832 | 84833 | 84834 | 84837 | 85080 | 85113 | 85241

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.