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