Re: [PATCH v6 19/27] util: fix interleaving of error & trace output

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The monitor_cur_hmp() function will acquire/release mutex locks, which
> will trigger trace probes, which can in turn trigger qemu_log() calls.
> vreport() calls monitor_cur() multiple times through its execution
> both directly and indirectly via error_vprintf().
>
> The result is that the prefix information printed by vreport() gets
> interleaved with qemu_log() output, when run outside the context of
> an HMP command dispatcher. This can be seen with:
>
>  $ qemu-system-x86_64 \
>      -msg timestamp=on,guest-name=on \
>      -display none \
>      -object tls-creds-x509,id=f,dir=fish \
>      -name fish \
>      -d trace:qemu_mutex*
>    2025-09-10T16:30:42.514374Z qemu_mutex_unlock released mutex 0x560b0339b4c0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:30:42.514400Z qemu_mutex_lock waiting on mutex 0x560b033983e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:30:42.514402Z qemu_mutex_locked taken mutex 0x560b033983e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:30:42.514404Z qemu_mutex_unlock released mutex 0x560b033983e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:30:42.516716Z qemu_mutex_lock waiting on mutex 0x560b03398560 (../monitor/monitor.c:91)
>    2025-09-10T16:30:42.516723Z qemu_mutex_locked taken mutex 0x560b03398560 (../monitor/monitor.c:91)
>    2025-09-10T16:30:42.516726Z qemu_mutex_unlock released mutex 0x560b03398560 (../monitor/monitor.c:96)
>    2025-09-10T16:30:42.516728Z qemu_mutex_lock waiting on mutex 0x560b03398560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842057Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842058Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    2025-09-10T16:31:04.842055Z 2025-09-10T16:31:04.842060Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842061Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842062Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    2025-09-10T16:31:04.842064Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842065Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842066Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    fish 2025-09-10T16:31:04.842068Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842069Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842070Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    2025-09-10T16:31:04.842072Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842097Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842099Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    qemu-system-x86_64:2025-09-10T16:31:04.842100Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842102Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842103Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>     2025-09-10T16:31:04.842105Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842106Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842107Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>    Unable to access credentials fish/ca-cert.pem: No such file or directory2025-09-10T16:31:04.842109Z qemu_mutex_lock waiting on mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842110Z qemu_mutex_locked taken mutex 0x564f5e401560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:04.842111Z qemu_mutex_unlock released mutex 0x564f5e401560 (../monitor/monitor.c:96)
>
> To avoid this interleaving (as well as reduce the huge number of
> mutex lock/unlock calls) we need to ensure that monitor_cur_is_hmp()
> is only called once at the start of vreport(), and if no HMP is
> present, no further monitor APIs can be called.
>
> This implies error_[v]printf() cannot be called from vreport().
> Instead we must introduce error_[v]printf_mon() which accept a
> pre-acquired Monitor object. In some cases, however, fprintf
> can be called directly as output will never be directed to the
> monitor.
>
>  $ qemu-system-x86_64 \
>      -msg timestamp=on,guest-name=on \
>      -display none \
>      -object tls-creds-x509,id=f,dir=fish \
>      -name fish \
>      -d trace:qemu_mutex*
>    2025-09-10T16:31:22.701691Z qemu_mutex_unlock released mutex 0x5626fd3b84c0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:31:22.701728Z qemu_mutex_lock waiting on mutex 0x5626fd3b53e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:31:22.701730Z qemu_mutex_locked taken mutex 0x5626fd3b53e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:31:22.701732Z qemu_mutex_unlock released mutex 0x5626fd3b53e0 (/var/home/berrange/src/virt/qemu/include/qemu/lockable.h:56)
>    2025-09-10T16:31:22.703989Z qemu_mutex_lock waiting on mutex 0x5626fd3b5560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:22.703996Z qemu_mutex_locked taken mutex 0x5626fd3b5560 (../monitor/monitor.c:91)
>    2025-09-10T16:31:22.703999Z qemu_mutex_unlock released mutex 0x5626fd3b5560 (../monitor/monitor.c:96)
>    2025-09-10T16:31:22.704000Z fish qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/error-report.c | 63 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/util/error-report.c b/util/error-report.c
> index f3aa24ec22..b34a2bc7bc 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -32,9 +32,9 @@ const char *error_guest_name;
>  /*
>   * Print to the current human monitor if we have one, else to stderr.

This is now misleading.  Let's use

    * Print to @cur_mon if non-null, else to stderr.

>   */
> -int error_vprintf(const char *fmt, va_list ap)
> +static int G_GNUC_PRINTF(2, 0)
> +error_vprintf_mon(Monitor *cur_mon, const char *fmt, va_list ap)
>  {
> -    Monitor *cur_mon = monitor_cur();
>      /*
>       * This will return -1 if 'cur_mon' is NULL, or is QMP.
>       * IOW this will only print if in HMP, otherwise we
> @@ -47,13 +47,33 @@ int error_vprintf(const char *fmt, va_list ap)
>      return ret;
>  }
>  
> +/*
> + * Print to the current human monitor if we have one, else to stderr.

Likewise.

> + */
> +static int G_GNUC_PRINTF(2, 3)
> +error_printf_mon(Monitor *cur_mon, const char *fmt, ...)
> +{
> +    va_list ap;
> +    int ret;
> +
> +    va_start(ap, fmt);
> +    ret = error_vprintf_mon(cur_mon, fmt, ap);
> +    va_end(ap);
> +    return ret;
> +}
> +

Keep its function comment, i.e.

   /*
    * Print to the current human monitor if we have one, else to stderr.
    */
> +int error_vprintf(const char *fmt, va_list ap)
> +{
> +    return error_vprintf_mon(monitor_cur(), fmt, ap);
> +}
> +

This one lost its function comment in commit 397d30e9401.  Let's correct
that accident:

   /*
    * Print to the current human monitor if we have one, else to stderr.
    */
>  int error_printf(const char *fmt, ...)
>  {
>      va_list ap;
>      int ret;
>  
>      va_start(ap, fmt);
> -    ret = error_vprintf(fmt, ap);
> +    ret = error_vprintf_mon(monitor_cur(), fmt, ap);
>      va_end(ap);
>      return ret;
>  }

With these trivial touch-ups
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]