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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> 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 ;)

Lisp is ---> that way.

At least the stupid loop is hidden away in glib-compat.h.  The actual
uses become neater, e.g.

    g_list_free_full(config->blacklist, g_free);

instead of

    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
    g_list_free(config->blacklist);

I think that's worth a bit of extra work in glib-compat.h.

>> 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.