Re: [Qemu-devel] [PATCH for-2.7 v3 04/36] qga: free remaining leaking state

[Markus Armbruster <armbru@redhat.com>]

Copying the QGA maintainer.

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/guest-agent-command-state.c | 7 +++++++
>  qga/guest-agent-core.h          | 1 +
>  qga/main.c                      | 6 ++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> index 4de229c..31c6368 100644
> --- a/qga/guest-agent-command-state.c
> +++ b/qga/guest-agent-command-state.c
> @@ -71,3 +71,10 @@ GACommandState *ga_command_state_new(void)
>      cs->groups = NULL;
>      return cs;
>  }
> +
> +void ga_command_state_free(GACommandState *cs)
> +{
> +    g_slist_foreach(cs->groups, qemu_g_func_free, NULL);
> +    g_slist_free(cs->groups);

Remarks on g_list_free_full() in PATCH 03 apply to g_slist_free_full()
here.

> +    g_free(cs);
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 0a49516..63e9d39 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -28,6 +28,7 @@ void ga_command_state_add(GACommandState *cs,
>  void ga_command_state_init_all(GACommandState *cs);
>  void ga_command_state_cleanup_all(GACommandState *cs);
>  GACommandState *ga_command_state_new(void);
> +void ga_command_state_free(GACommandState *cs);
>  bool ga_logging_enabled(GAState *s);
>  void ga_disable_logging(GAState *s);
>  void ga_enable_logging(GAState *s);
> diff --git a/qga/main.c b/qga/main.c
> index 868508b..0038702 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1372,6 +1372,8 @@ int main(int argc, char **argv)
>  end:
>      if (s->command_state) {
>          ga_command_state_cleanup_all(s->command_state);
> +        ga_command_state_free(s->command_state);
> +        json_message_parser_destroy(&s->parser);
>      }
>      if (s->channel) {
>          ga_channel_free(s->channel);
> @@ -1384,6 +1386,10 @@ end:
>      }
>  
>      config_free(config);
> +    if (s->main_loop) {
> +        g_main_loop_unref(s->main_loop);
> +    }
> +    g_free(s);
>  
>      return ret;
>  }

Not bothering to free memory immediately before terminating the process
is not a leak.  The commit message shouldn't claim it is.

Personally, I wouldn't bother with freeing memory there.  Applies to
PATCH 03, too.  But it looks like the maintainer likes having it done.
If that's true, then doing it completely is probably better.

Leaving actual review to the maintainer.