Re: [PATCH] util/log: do not close and reopen log files when flags are turned off

[Greg Kurz <groug@kaod.org>]

On Tue, 25 Oct 2022 11:21:19 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> log_append makes sure that if you turn off the logging (which clears
> log_flags and makes need_to_open_file false) the old log is not
> overwritten.  The usecase is that if you remove or move the file
> QEMU will not keep writing to the old file.  However, this is
> not always the desited behavior, in particular having log_append==1
> after changing the file name makes little sense.
> 
> When qemu_set_log_internal is called from the logfile monitor
> command, filename must be non-NULL and therefore changed_name must
> be true.  Therefore, the only case where the file is closed and
> need_to_open_file == false is indeed when log_flags becomes
> zero.  In this case, just flush the file and do not bother
> closing it, thus faking the same append behavior as previously.
> 
> The behavioral change is that changing the logfile twice, for
> example log1 -> log2 -> log1, will cause log1 to be overwritten.
> This can simply be documented, since it is not a particularly
> surprising behavior.
> 
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Heh I currently have a very similar patch in my tree :-)

Reviewed-by: Greg Kurz <groug@kaod.org>

I'll include this and other bug fixes as prerequisites for my
on-going work on logging when daemonized.

>  util/log.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/util/log.c b/util/log.c
> index d6eb0378c3a3..06d0173788dc 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -44,7 +44,6 @@ static FILE *global_file;
>  static __thread FILE *thread_file;
>  
>  int qemu_loglevel;
> -static bool log_append;
>  static bool log_per_thread;
>  static GArray *debug_regions;
>  
> @@ -259,19 +258,19 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name,
>      daemonized = is_daemonized();
>      need_to_open_file = log_flags && !per_thread && (!daemonized || filename);
>  
> -    if (logfile && (!need_to_open_file || changed_name)) {
> -        qatomic_rcu_set(&global_file, NULL);
> -        if (logfile != stderr) {
> +    if (logfile) {
> +        fflush(logfile);
> +        if (changed_name && logfile != stderr) {
>              RCUCloseFILE *r = g_new0(RCUCloseFILE, 1);
>              r->fd = logfile;
>              call_rcu(r, rcu_close_file, rcu);
> +            logfile = NULL;
>          }
> -        logfile = NULL;
>      }
>  
>      if (!logfile && need_to_open_file) {
>          if (filename) {
> -            logfile = fopen(filename, log_append ? "a" : "w");
> +            logfile = fopen(filename, "w");
>              if (!logfile) {
>                  error_setg_errno(errp, errno, "Error opening logfile %s",
>                                   filename);
> @@ -290,8 +289,6 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name,
>              logfile = stderr;
>          }
>  
> -        log_append = 1;
> -
>          qatomic_rcu_set(&global_file, logfile);
>      }
>      return true;