[Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex

[Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex

[Stefan Hajnoczi]

Avoid duplicating ifdefs for the deprecated GStaticMutex API by
introducing compat_g_static_mutex_*() in glib-compat.h.

GStaticMutex users should use the compat API to avoid dealing with
deprecation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 coroutine-gthread.c   | 13 +++++++------
 include/glib-compat.h | 19 +++++++++++++++++++
 trace/simple.c        | 26 +++++++++-----------------
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/coroutine-gthread.c b/coroutine-gthread.c
index 695c113..28272f8 100644
--- a/coroutine-gthread.c
+++ b/coroutine-gthread.c
@@ -30,7 +30,7 @@ typedef struct {
     CoroutineAction action;
 } CoroutineGThread;
 
-static GStaticMutex coroutine_lock = G_STATIC_MUTEX_INIT;
+DEFINE_COMPAT_G_STATIC_MUTEX(coroutine_lock);
 
 /* GLib 2.31 and beyond deprecated various parts of the thread API,
  * but the new interfaces are not available in older GLib versions
@@ -123,15 +123,16 @@ static void __attribute__((constructor)) coroutine_init(void)
 static void coroutine_wait_runnable_locked(CoroutineGThread *co)
 {
     while (!co->runnable) {
-        g_cond_wait(coroutine_cond, g_static_mutex_get_mutex(&coroutine_lock));
+        g_cond_wait(coroutine_cond,
+                    compat_g_static_mutex_get_mutex(&coroutine_lock));
     }
 }
 
 static void coroutine_wait_runnable(CoroutineGThread *co)
 {
-    g_static_mutex_lock(&coroutine_lock);
+    compat_g_static_mutex_lock(&coroutine_lock);
     coroutine_wait_runnable_locked(co);
-    g_static_mutex_unlock(&coroutine_lock);
+    compat_g_static_mutex_unlock(&coroutine_lock);
 }
 
 static gpointer coroutine_thread(gpointer opaque)
@@ -173,7 +174,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_,
     CoroutineGThread *from = DO_UPCAST(CoroutineGThread, base, from_);
     CoroutineGThread *to = DO_UPCAST(CoroutineGThread, base, to_);
 
-    g_static_mutex_lock(&coroutine_lock);
+    compat_g_static_mutex_lock(&coroutine_lock);
     from->runnable = false;
     from->action = action;
     to->runnable = true;
@@ -183,7 +184,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_,
     if (action != COROUTINE_TERMINATE) {
         coroutine_wait_runnable_locked(from);
     }
-    g_static_mutex_unlock(&coroutine_lock);
+    compat_g_static_mutex_unlock(&coroutine_lock);
     return from->action;
 }
 
diff --git a/include/glib-compat.h b/include/glib-compat.h
index ea965df..48fa95c 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -49,4 +49,23 @@ static inline GThread *g_thread_new(const gchar *unused,
 }
 #endif
 
+/* GStaticMutex was deprecated in 2.32.0.  Use the compat_g_static_mutex_*()
+ * wrappers to hide the GStaticMutex/GMutex distinction.
+ */
+#if GLIB_CHECK_VERSION(2, 32, 0)
+#define DEFINE_COMPAT_G_STATIC_MUTEX(name) \
+static GMutex name
+
+#define compat_g_static_mutex_lock(m) g_mutex_lock(m)
+#define compat_g_static_mutex_unlock(m) g_mutex_unlock(m)
+#define compat_g_static_mutex_get_mutex(m) (m)
+#else
+#define DEFINE_COMPAT_G_STATIC_MUTEX(name) \
+static GStaticMutex name = G_STATIC_MUTEX_INIT
+
+#define compat_g_static_mutex_lock(m) g_static_mutex_lock(m)
+#define compat_g_static_mutex_unlock(m) g_static_mutex_unlock(m)
+#define compat_g_static_mutex_get_mutex(m) g_static_mutex_get_mutex(m)
+#endif
+
 #endif
diff --git a/trace/simple.c b/trace/simple.c
index 8e83e59..dc58afe 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -41,17 +41,7 @@
  * Trace records are written out by a dedicated thread.  The thread waits for
  * records to become available, writes them out, and then waits again.
  */
-#if GLIB_CHECK_VERSION(2, 32, 0)
-static GMutex trace_lock;
-#define lock_trace_lock() g_mutex_lock(&trace_lock)
-#define unlock_trace_lock() g_mutex_unlock(&trace_lock)
-#define get_trace_lock_mutex() (&trace_lock)
-#else
-static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
-#define lock_trace_lock() g_static_mutex_lock(&trace_lock)
-#define unlock_trace_lock() g_static_mutex_unlock(&trace_lock)
-#define get_trace_lock_mutex() g_static_mutex_get_mutex(&trace_lock)
-#endif
+DEFINE_COMPAT_G_STATIC_MUTEX(trace_lock);
 
 /* g_cond_new() was deprecated in glib 2.31 but we still need to support it */
 #if GLIB_CHECK_VERSION(2, 31, 0)
@@ -151,26 +141,28 @@ static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
  */
 static void flush_trace_file(bool wait)
 {
-    lock_trace_lock();
+    compat_g_static_mutex_lock(&trace_lock);
     trace_available = true;
     g_cond_signal(trace_available_cond);
 
     if (wait) {
-        g_cond_wait(trace_empty_cond, get_trace_lock_mutex());
+        g_cond_wait(trace_empty_cond,
+                    compat_g_static_mutex_get_mutex(&trace_lock));
     }
 
-    unlock_trace_lock();
+    compat_g_static_mutex_unlock(&trace_lock);
 }
 
 static void wait_for_trace_records_available(void)
 {
-    lock_trace_lock();
+    compat_g_static_mutex_lock(&trace_lock);
     while (!(trace_available && trace_writeout_enabled)) {
         g_cond_signal(trace_empty_cond);
-        g_cond_wait(trace_available_cond, get_trace_lock_mutex());
+        g_cond_wait(trace_available_cond,
+                    compat_g_static_mutex_get_mutex(&trace_lock));
     }
     trace_available = false;
-    unlock_trace_lock();
+    compat_g_static_mutex_unlock(&trace_lock);
 }
 
 static gpointer writeout_thread(gpointer opaque)
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex

[Michael Tokarev]

Stefan Hajnoczi:

> Avoid duplicating ifdefs for the deprecated GStaticMutex API by
> introducing compat_g_static_mutex_*() in glib-compat.h.
>
> GStaticMutex users should use the compat API to avoid dealing with
> deprecation.

To my taste this is a wrong(ish) approach.

GStaticMutex is nothing more than a pointer to GMutex wrapped in an internal
structure, and each function from g_static_mutex_* family first checks for
this pointer, whenever it is initialized, and calls g_mutex_new() if not.
So if there's a good place to actually init (allocate) a GMutex, it is better
to use it directly instead of checking for the init in every place.

(Note also that this checking itself may need a protection using a mutex,
to avoid 2 threads allocating the same GStaticMutex ;) -- this is done in
g_static_mutex_get_mutex_impl() inside glib).

But that's cosmetics.

More to the point is that this is re-introduction of old API for new program.
It'd be very nice if we were able to actually use the new API everywhere,
instead of re-introducing old API.

And this introduces some (light) wrapper #ifdefery too.

So we not only will use the old API in the code, we also will use wrappers
for almost everything.  So it becomes rather clumsy.

It is, I think, better to try to switch to the new API as much as possible,
or to use our own types and wrappers instead, without resoting to use
foo_compat_deprecated_* (where "deprecated" part comes from staticmutex --
ie, instead of regular g_mutex_foo, we use g_compat_static_mutex_foo).

Other the bit of clumsiness, this should work.


> +/* GStaticMutex was deprecated in 2.32.0.  Use the compat_g_static_mutex_*()
> + * wrappers to hide the GStaticMutex/GMutex distinction.
> + */
> +#if GLIB_CHECK_VERSION(2, 32, 0)

It is actually deprecated in 2.31, even if the glib docs says 2.32.
Probably not a big deal, as, I think, there's no 2.31 glib in the wild
due to bugs in there, so they had to quickly release 2.32.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex

[Stefan Hajnoczi]

On Fri, May 02, 2014 at 03:08:07PM +0400, Michael Tokarev wrote:
> Stefan Hajnoczi:
> 
> > Avoid duplicating ifdefs for the deprecated GStaticMutex API by
> > introducing compat_g_static_mutex_*() in glib-compat.h.
> >
> > GStaticMutex users should use the compat API to avoid dealing with
> > deprecation.
> 
> To my taste this is a wrong(ish) approach.

The different approaches to hiding gthread API deprecation have their
pros/cons but mostly a question of taste.  If you want to merge your
patch I'm fine with that.

Stefan