Re: [PATCH for-9.1] util/log: add cleanup function
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(_mutex) to balance startup()?
What about debug_regions?
OK, I tend to agree and don't care about)
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
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
Re: [PATCH for-9.1] util/log: add cleanup function
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(_mutex) to balance startup()?
What about debug_regions?
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
> 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
>
signature.asc
Description: PGP signature
[PATCH for-9.1] util/log: add cleanup function
We leak global_filename, and do not close global_file. Let's fix that.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
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
