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

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

[Vladimir Sementsov-Ogievskiy]

We leak global_filename, and do not close global_file. Let's fix that.

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

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Vladimir Sementsov-Ogievskiy]

On 22.04.24 22:29, Stefan Hajnoczi wrote:
> 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?

Correct. But a bit more interesting: Coverity think that filename is leaked inside function. But that's a false positive (and marked as false-positive), as it is stored into global_filename, which is freed before next assignment. Still, looking at this, I noticed that global_filename is finally leaked at the end of the program.. And decided to fix.

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

Agree, that's a risk.

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

OK, I tend to agree and don't care about)

> 
>>
>> 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
>>

-- 
Best regards,
Vladimir