On Tue, 25 Oct 2022 14:33:15 +0200
Greg Kurz <gr...@kaod.org> wrote:
> On Tue, 25 Oct 2022 11:21:19 +0200
> Paolo Bonzini <pbonz...@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.ben...@linaro.org>
> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > ---
>
> Heh I currently have a very similar patch in my tree :-)
>
> Reviewed-by: Greg Kurz <gr...@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;
>
>
