Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Marc-André Lureau]

Hi

----- Original Message -----
> Copying the QGA maintainer.
> 
> address@hidden writes:
> 
> > From: Marc-André Lureau <address@hidden>
> >
> > Free the list, not just the elements.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  include/glib-compat.h | 9 +++++++++
> >  qga/main.c            | 8 ++------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > index 01aa7b3..6d643b2 100644
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable
> > *hash_table, gpointer key)
> >      } while (0)
> >  #endif
> >  
> > +/*
> > + * A GFunc function helper freeing the first argument (not part of glib)
> 
> Well, it's not part of GLib, because non-obsolete GLib has no use for
> it!  You'd simply pass g_free directly to a _free_full() function
> instead of passing a silly wrapper to a _foreach() function.
> 
> The commit does a bit more than just plug a leak, it also provides a new
> helper for general use.  Mention in the commit message?
> 
> I wonder how many more of these silly g_free() wrappers we have.  A
> quick grep reports hits in string-input-visitor.c and
> qobject/json-streamer.c.  If you add one to a header, you get to hunt
> them down :)
> 
> > + */
> > +static inline void qemu_g_func_free(gpointer data,
> > +                                    gpointer user_data)
> > +{
> > +    g_free(data);
> > +}
> > +
> >  #endif
> > diff --git a/qga/main.c b/qga/main.c
> > index 4c3b2c7..868508b 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
> >  #ifdef CONFIG_FSFREEZE
> >      g_free(config->fsfreeze_hook);
> >  #endif
> > +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
> > +    g_list_free(config->blacklist);
> 
> Modern GLib code doesn't need silly wrappers:
> 
>     g_list_free_full(config->blacklist, g_free);
> 
> Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
> Please explain this in the commit message.
> 
> Even better, provide a replacement in glib-compat.h #if
> !GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
> when we upgrade to 2.28.

ok

> 
> >      g_free(config);
> >  }
> >  
> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config)
> >      return EXIT_SUCCESS;
> >  }
> >  
> > -static void free_blacklist_entry(gpointer entry, gpointer unused)
> > -{
> > -    g_free(entry);
> > -}
> > -
> >  int main(int argc, char **argv)
> >  {
> >      int ret = EXIT_SUCCESS;
> > @@ -1379,7 +1376,6 @@ end:
> >      if (s->channel) {
> >          ga_channel_free(s->channel);
> >      }
> > -    g_list_foreach(config->blacklist, free_blacklist_entry, NULL);
> >      g_free(s->pstate_filepath);
> >      g_free(s->state_filepath_isfrozen);
> 
> If you at least explain why not g_list_free_full() in the commit
> message, you can add
> Reviewed-by: Markus Armbruster <address@hidden>
> 
> But I'd like to encourage you to explore providing a replacement for
> g_list_free_full().

Such replacement would make use of a (GFunc) cast, glib implementation is 
calling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't want to 
have such cast in qemu code. He suggested to have the static inline helper in 
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html