Re: [PATCH for-9.1] util/log: add cleanup function

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On Wed, Apr 17, 2024 at 09:33:33PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We leak global_filename, and do not close global_file. Let's fix that.

What is the goal?

Leaking global_filename does not cause unbounded memory consumption. I
guess the goal in freeing global_filename is to keep leak checker
reports tidy?

Closing global_file doesn't improve anything AFAICT. It might cause
problems if another component still wants to log something from a
destructor function. I'm not sure if the order of destructors is
defined.

What about qemu_mutex_destroy(&global_mutex) to balance startup()?

What about debug_regions?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> Interesting: seems, nobody is maintainer of util/log.c
> 
>  util/log.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/util/log.c b/util/log.c
> index d36c98da0b..30de209210 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -85,6 +85,15 @@ static void qemu_log_thread_cleanup(Notifier *n, void *unused)
>      }
>  }
>  
> +static void __attribute__((__destructor__)) cleanup(void)
> +{
> +    g_free(global_filename);
> +    if (global_file && global_file != stderr) {
> +        fclose(global_file);
> +        global_file = NULL;
> +    }
> +}
> +
>  /* Lock/unlock output. */
>  
>  static FILE *qemu_log_trylock_with_err(Error **errp)
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]