[PATCH] once: Fix race in implementation

[david.henningsson@xxxxxxxxxxxxx (David Henningsson)]

TODO: Write a nice commit message

Signed-off-by: David Henningsson <david.henningsson at canonical.com>
---
 src/pulsecore/once.c |   54 +++++++++++++++++++++++---------------------------
 src/pulsecore/once.h |    5 ++---
 2 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/src/pulsecore/once.c b/src/pulsecore/once.c
index 4e509e0..4be590e 100644
--- a/src/pulsecore/once.c
+++ b/src/pulsecore/once.c
@@ -28,42 +28,43 @@
 
 #include "once.h"
 
+#define PA_ONCE_STATE_INIT 0
+#define PA_ONCE_STATE_RUNNING 1
+#define PA_ONCE_STATE_DONE 2
+
 pa_bool_t pa_once_begin(pa_once *control) {
+    pa_mutex *m;
+
     pa_assert(control);
 
-    if (pa_atomic_load(&control->done))
+    if (pa_atomic_load(&control->state) == PA_ONCE_STATE_DONE)
         return FALSE;
 
-    pa_atomic_inc(&control->ref);
+    if (pa_atomic_cmpxchg(&control->state, PA_ONCE_STATE_INIT, PA_ONCE_STATE_RUNNING)) {
+        /* Ok, we're good to run */
+
+        pa_assert_se(m = pa_mutex_new(FALSE, FALSE));
+        pa_mutex_lock(m);
+        pa_atomic_ptr_store(&control->mutex, m);
+        return TRUE;
+    }
 
     /* Caveat: We have to make sure that the once func has completed
      * before returning, even if the once func is not actually
      * executed by us. Hence the awkward locking. */
 
-    for (;;) {
-        pa_mutex *m;
-
-        if ((m = pa_atomic_ptr_load(&control->mutex))) {
+    do {
+        m = pa_atomic_ptr_load(&control->mutex);
+    } while (!m);
 
-            /* The mutex is stored in locked state, hence let's just
-             * wait until it is unlocked */
-            pa_mutex_lock(m);
+    /* The mutex is stored in locked state, hence let's just
+     * wait until it is unlocked */
+    pa_mutex_lock(m);
 
-            pa_assert(pa_atomic_load(&control->done));
+    pa_assert(pa_atomic_load(&control->state) == PA_ONCE_STATE_DONE);
 
-            pa_once_end(control);
-            return FALSE;
-        }
-
-        pa_assert_se(m = pa_mutex_new(FALSE, FALSE));
-        pa_mutex_lock(m);
-
-        if (pa_atomic_ptr_cmpxchg(&control->mutex, NULL, m))
-            return TRUE;
-
-        pa_mutex_unlock(m);
-        pa_mutex_free(m);
-    }
+    pa_once_end(control);
+    return FALSE;
 }
 
 void pa_once_end(pa_once *control) {
@@ -71,15 +72,10 @@ void pa_once_end(pa_once *control) {
 
     pa_assert(control);
 
-    pa_atomic_store(&control->done, 1);
+    pa_atomic_store(&control->state, PA_ONCE_STATE_DONE);
 
     pa_assert_se(m = pa_atomic_ptr_load(&control->mutex));
     pa_mutex_unlock(m);
-
-    if (pa_atomic_dec(&control->ref) <= 1) {
-        pa_assert_se(pa_atomic_ptr_cmpxchg(&control->mutex, m, NULL));
-        pa_mutex_free(m);
-    }
 }
 
 /* Not reentrant -- how could it be? */
diff --git a/src/pulsecore/once.h b/src/pulsecore/once.h
index edc8188..eb85f75 100644
--- a/src/pulsecore/once.h
+++ b/src/pulsecore/once.h
@@ -26,14 +26,13 @@
 
 typedef struct pa_once {
     pa_atomic_ptr_t mutex;
-    pa_atomic_t ref, done;
+    pa_atomic_t state;
 } pa_once;
 
 #define PA_ONCE_INIT                                                    \
     {                                                                   \
         .mutex = PA_ATOMIC_PTR_INIT(NULL),                              \
-        .ref = PA_ATOMIC_INIT(0),                                       \
-        .done = PA_ATOMIC_INIT(0)                                       \
+        .state = PA_ATOMIC_INIT(0)                                      \
     }
 
 /* Not to be called directly, use the macros defined below instead */
-- 
1.7.9.5


--------------080608070605010202060104--