Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Markus Armbruster]

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.

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