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

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;

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

Il mar 25 ott 2022, 16:39 Greg Kurz <groug@kaod.org> ha scritto:

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

Yes it should.

Paolo

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

On Tue, 25 Oct 2022 22:27:36 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il mar 25 ott 2022, 16:39 Greg Kurz <groug@kaod.org> ha scritto:
> 
> > > > -    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);
> >
> 
> Yes it should.
> 

I'll fix this when I repost the full series.

Cheers,

--
Greg


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

On 10/25/22 19:21, Paolo Bonzini 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>
> ---
>   util/log.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~