[PATCH 0/2] util/log: Make the per-thread flag immutable

[PATCH 0/2] util/log: Make the per-thread flag immutable

[Greg Kurz]

While working on the "util/log: Always send errors to logfile when daemonized"
series [1], I've encountered some issues with the per-thread flag. They stem
from the code not being designed to allow the per-thread flag to be enabled
or disabled more than once, but nothing is done to prevent that from
happening. This results in unexpected results like the creation of a log
file with a `%d` in its name or confusing errors when using the `log`
command in the monitor.

I'm posting fixes separately now in case it makes sense to merge them during
soft freeze. If so, I'll open an issue as explained in this recent mail [2].

[1] https://patchew.org/QEMU/20221019151651.334334-1-groug@kaod.org/
[2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg00137.html

Date: Wed, 19 Oct 2022 17:16:49 +0200
Message-ID: <20221019151651.334334-1-groug@kaod.org>

Greg Kurz (2):
  util/log: Make the per-thread flag immutable
  util/log: Ignore per-thread flag if global file already there

 util/log.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.38.1

[PATCH 1/2] util/log: Make the per-thread flag immutable

[Greg Kurz]

Per-thread logging was implemented under the assumption that once
enabled, it is not possible to switch back to single file logging.
This isn't enforced though and it is possible to go through the
global file opening sequence in per-thread mode. The code isn't
ready for this and produces unexpected results as detailed below.

Start QEMU in system emulation mode with `-D ./qemu.log.%d -d tid`
and then change the log level from the monitor to something that
doesn't have tid, e.g. `log cpu_reset`. The value of log_flags
is zero and per_thread is set to false : the rest of the code
then assumes it is running in the global log case and opens a
file named `qemu.log.%d`, which is obviously not an expected
behavior.

Enforce the immutability of the flag early in qemu_set_log_internal()
so that its value is correct for all subsequent users.

Fixes: 4e51069d6793 ("util/log: Support per-thread log files")
Cc: richard.henderson@linaro.org
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 util/log.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/util/log.c b/util/log.c
index 39866bdaf2fa..b7d2b6e09cfe 100644
--- a/util/log.c
+++ b/util/log.c
@@ -206,6 +206,11 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name,
     QEMU_LOCK_GUARD(&global_mutex);
     logfile = global_file;
 
+    /* The per-thread flag is immutable. */
+    if (log_per_thread) {
+        log_flags |= LOG_PER_THREAD;
+    }
+
     per_thread = log_flags & LOG_PER_THREAD;
 
     if (changed_name) {
-- 
2.38.1

[PATCH 2/2] util/log: Ignore per-thread flag if global file already there

[Greg Kurz]

If QEMU is started with `-D qemu.log.%d` without any `-d` option,
doing `log all` in the monitor fails with:

Filename template with '%d' required for 'tid'

It is confusing since '%d' was actually passed.

This happens because QEMU caches the log file name with %d converted
to getpid() since `tid` wasn't required. This name isn't suitable
for a subsequent enablement of per-thread logs. There's little cause
to change the behavior as `-d tid` is mostly used at user-only startup.

Drop the per-thread from the requested flags in this case : `log all`
will thus enable everything except `tid` instead of failing. This is
preferable over forcing the user to enable each log item individually.

With this patch, `tid` is now truely immutable : it can only be set
or unset from the command line and never changed afterwards.

Fixes: 4e51069d6793 ("util/log: Support per-thread log files")
Cc: richard.henderson@linaro.org
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 util/log.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/util/log.c b/util/log.c
index b7d2b6e09cfe..c2198badf240 100644
--- a/util/log.c
+++ b/util/log.c
@@ -209,6 +209,10 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name,
     /* The per-thread flag is immutable. */
     if (log_per_thread) {
         log_flags |= LOG_PER_THREAD;
+    } else {
+        if (global_filename) {
+            log_flags &= ~LOG_PER_THREAD;
+        }
     }
 
     per_thread = log_flags & LOG_PER_THREAD;
-- 
2.38.1

Re: [PATCH 0/2] util/log: Make the per-thread flag immutable

[Richard Henderson]

On 11/4/22 23:00, Greg Kurz wrote:
> While working on the "util/log: Always send errors to logfile when daemonized"
> series [1], I've encountered some issues with the per-thread flag. They stem
> from the code not being designed to allow the per-thread flag to be enabled
> or disabled more than once, but nothing is done to prevent that from
> happening. This results in unexpected results like the creation of a log
> file with a `%d` in its name or confusing errors when using the `log`
> command in the monitor.
> 
> I'm posting fixes separately now in case it makes sense to merge them during
> soft freeze. If so, I'll open an issue as explained in this recent mail [2].
> 
> [1] https://patchew.org/QEMU/20221019151651.334334-1-groug@kaod.org/
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg00137.html
> 
> Date: Wed, 19 Oct 2022 17:16:49 +0200
> Message-ID: <20221019151651.334334-1-groug@kaod.org>
> 
> Greg Kurz (2):
>    util/log: Make the per-thread flag immutable
>    util/log: Ignore per-thread flag if global file already there
> 
>   util/log.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 

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


r~

Re: [PATCH 0/2] util/log: Make the per-thread flag immutable

[Greg Kurz]

On Sat, 5 Nov 2022 09:37:26 +1100
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 11/4/22 23:00, Greg Kurz wrote:
> > While working on the "util/log: Always send errors to logfile when daemonized"
> > series [1], I've encountered some issues with the per-thread flag. They stem
> > from the code not being designed to allow the per-thread flag to be enabled
> > or disabled more than once, but nothing is done to prevent that from
> > happening. This results in unexpected results like the creation of a log
> > file with a `%d` in its name or confusing errors when using the `log`
> > command in the monitor.
> > 
> > I'm posting fixes separately now in case it makes sense to merge them during
> > soft freeze. If so, I'll open an issue as explained in this recent mail [2].
> > 
> > [1] https://patchew.org/QEMU/20221019151651.334334-1-groug@kaod.org/
> > [2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg00137.html
> > 
> > Date: Wed, 19 Oct 2022 17:16:49 +0200
> > Message-ID: <20221019151651.334334-1-groug@kaod.org>
> > 
> > Greg Kurz (2):
> >    util/log: Make the per-thread flag immutable
> >    util/log: Ignore per-thread flag if global file already there
> > 
> >   util/log.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> 
> Series:
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

Thanks for the quick review Richard !

I've created https://gitlab.com/qemu-project/qemu/-/issues/1302 with
a 7.2 milestone.

Paolo,

Can you queue this ?

Cheers,

--
Greg

> 
> r~

Re: [PATCH 0/2] util/log: Make the per-thread flag immutable

[Stefan Hajnoczi]

On Mon, 7 Nov 2022 at 07:36, Greg Kurz <groug@kaod.org> wrote:
>
> On Sat, 5 Nov 2022 09:37:26 +1100
> Richard Henderson <richard.henderson@linaro.org> wrote:
>
> > On 11/4/22 23:00, Greg Kurz wrote:
> > > While working on the "util/log: Always send errors to logfile when daemonized"
> > > series [1], I've encountered some issues with the per-thread flag. They stem
> > > from the code not being designed to allow the per-thread flag to be enabled
> > > or disabled more than once, but nothing is done to prevent that from
> > > happening. This results in unexpected results like the creation of a log
> > > file with a `%d` in its name or confusing errors when using the `log`
> > > command in the monitor.
> > >
> > > I'm posting fixes separately now in case it makes sense to merge them during
> > > soft freeze. If so, I'll open an issue as explained in this recent mail [2].
> > >
> > > [1] https://patchew.org/QEMU/20221019151651.334334-1-groug@kaod.org/
> > > [2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg00137.html
> > >
> > > Date: Wed, 19 Oct 2022 17:16:49 +0200
> > > Message-ID: <20221019151651.334334-1-groug@kaod.org>
> > >
> > > Greg Kurz (2):
> > >    util/log: Make the per-thread flag immutable
> > >    util/log: Ignore per-thread flag if global file already there
> > >
> > >   util/log.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > >
> >
> > Series:
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >
>
> Thanks for the quick review Richard !
>
> I've created https://gitlab.com/qemu-project/qemu/-/issues/1302 with
> a 7.2 milestone.
>
> Paolo,
>
> Can you queue this ?

I've applied it to the staging branch.

Stefan

Re: [PATCH 0/2] util/log: Make the per-thread flag immutable

[Stefan Hajnoczi]

Merged, thanks!

Stefan