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

[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

[Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h

[Stefan Hajnoczi]

glib has deprecated APIs like GStaticMutex, g_thread_create(), and others.  In
QEMU support both old and new APIs since using deprecated APIs would flood us
with warnings but legacy distros must continue to build the QEMU source code.

This patch series reduces ifdefs by moving glib compat functions into
glib-compat.h, where they can be reused.

There are two strategies for compat functions:

1. Implement the new API using the deprecated API.  This compat function is
   used when building on a legacy host.  Sometimes the API semantics are so
   different that this option is not feasible.

2. Add a new wrapper API that maps to the deprecated API.  The wrapper is not
   marked deprecated so it works as a drop-in replacement but is implemented
   using the new API where possible.

Stefan Hajnoczi (3):
  glib: move g_poll() replacement into glib-compat.h
  glib: add g_thread_new() compat function
  glib: add compat wrapper for GStaticMutex

 coroutine-gthread.c   | 26 ++++++++++----------------
 include/glib-compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/qemu-common.h | 12 ------------
 trace/simple.c        | 31 ++++++++++---------------------
 4 files changed, 64 insertions(+), 49 deletions(-)

-- 
1.8.5.3

[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