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

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