Hi
----- Original Message -----
> Copying the QGA maintainer.
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Free the list, not just the elements.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > 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 <arm...@redhat.com>
>
> 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
