Hi
----- Original Message -----
> Marc-André Lureau <mlur...@redhat.com> writes:
>
> > 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
>
> 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.
