Re: [Qemu-devel] [PATCH] trace: add glib 2.32+ static GMutex support

[Michael Tokarev <mjt@tls.msk.ru>]

12.12.2013 18:52, Stefan Hajnoczi wrote:
> The GStaticMutex API was deprecated in glib 2.32.  We cannot switch over
> to GMutex unconditionally since we would drop support for older glib
> versions.  But the deprecated API warnings during build are annoying so
> use static GMutex when possible.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  trace/simple.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/trace/simple.c b/trace/simple.c
> index 1e3f691..941f7ea 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -39,7 +39,11 @@
>   * 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;
> +#else
>  static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
> +#endif
>  
>  /* g_cond_new() was deprecated in glib 2.31 but we still need to support it */
>  #if GLIB_CHECK_VERSION(2, 31, 0)
> @@ -86,6 +90,34 @@ typedef struct {
>  static void read_from_buffer(unsigned int idx, void *dataptr, size_t size);
>  static unsigned int write_to_buffer(unsigned int idx, void *dataptr, size_t size);
>  
> +/* Hide changes in glib mutex APIs */
> +static void lock_trace_lock(void)
> +{
> +#if GLIB_CHECK_VERSION(2, 32, 0)
> +    g_mutex_lock(&trace_lock);
> +#else
> +    g_static_mutex_lock(&trace_lock);
> +#endif
> +}
> +
> +static void unlock_trace_lock(void)
> +{
> +#if GLIB_CHECK_VERSION(2, 32, 0)
> +    g_mutex_unlock(&trace_lock);
> +#else
> +    g_static_mutex_unlock(&trace_lock);
> +#endif
> +}
> +
> +static GMutex *get_trace_lock_mutex(void)
> +{
> +#if GLIB_CHECK_VERSION(2, 32, 0)
> +    return &trace_lock;
> +#else
> +    return g_static_mutex_get_mutex(&trace_lock);
> +#endif
> +}


I'd group mutex definition above with all the functions accessing it,
and also make the functions inline.

Well, to my taste, this is a good example where #define is better than
an inline function.  Compare the above with:

diff --git a/trace/simple.c b/trace/simple.c
index 1e3f691..2e55ac1 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -39,7 +39,17 @@
  * 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

 /* g_cond_new() was deprecated in glib 2.31 but we still need to support it */
 #if GLIB_CHECK_VERSION(2, 31, 0)

(#defines here and elsewhere has added bonus - when debugging, debugger
does not step into the inline functions, -- such stepping is quite annoying).

But somehow many developers prefer inline functions (sometimes it is better
indeed, especially in a commonly used header files, and when the functions
has complex or many parameters; in this case we have much simpler situation.

For fun, this #ifdeffery is 5 times larger than the actual users of the
functions being defined :)

Thanks,

/mjt