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 14:33:15 +0200
Greg Kurz <groug@kaod.org> wrote:

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

Hmm... wait, shouldn't this NULLifying be performed...

> > -        if (logfile != stderr) {
> > +    if (logfile) {
> > +        fflush(logfile);
> > +        if (changed_name && logfile != stderr) {
> >              RCUCloseFILE *r = g_new0(RCUCloseFILE, 1);
> >              r->fd = logfile;


... here since we the following closes the global_file ?

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