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

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <address@hidden> writes:
> 
> > 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
> 
> Pointlessly dirty:
> 
>     g_list_foreach(list, (GFunc)g_free, NULL)
> 
> Trade dirty for cumbersome:
> 
>     void free_wrapper(gpointer data, gpointer unused)
>     {
>         g_free(data)
>     }
> 
>     g_list_foreach(list, free_wrapper, NULL);
> 
> But this is C.  In C, simple things are best done the simple way.  Even
> when that means we don't get to show off how amazingly well we've been
> educated on higher order functions:
> 
>     for (node = list; node; node = next) {
>         next = node->next;
>         g_free(node);
>     }
> 
> Simple, stupid, and not dirty.

If only we were paid to write more lines of code ;) Anyway, that's fine by me 
(it's work after all, I'll write elegant code for fun time ;)

> Quote: "Note that it is considered perfectly acceptable to access
> list->next directly."

Funny that quote is only in GList, but GSList also documents and exposes the 
struct.