Marc-André Lureau <mlur...@redhat.com> writes:
> 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 ;)
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.
