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

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Dec 12, 2013 at 07:36:18PM +0400, Michael Tokarev wrote:
> 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 :)

Yes, I think you are right.  In general I avoid using macros but here it
does make things nicer.

Stefan