| Message ID | 20240417183333.39256-1-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [for-9.1] util/log: add cleanup function | expand |
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 >
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 >>
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)
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(+)