[PATCH v6 00/27] util: sync error_report & qemu_log output more closely

[PATCH v6 00/27] util: sync error_report & qemu_log output more closely

[Daniel P. Berrangé]

This series is a tangent that came out of discussion in

   https://lists.nongnu.org/archive/html/qemu-devel/2025-08/msg00903.html

In thinking about adding thread info to error_report, I
came to realize we should likely make qemu_log behave
consistently with error_report & friends. We already
honour '-msg timestamp=on', but don't honour 'guest-name=on'
and also don't include the binary name.

As an example of the current state, consider mixing error and
log output today:

- Default context:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
                       -d 'trace:qcrypto*'
  qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55ac6d97f700 dir=fish
  qcrypto_tls_creds_get_path TLS creds path creds=0x55ac6d97f700 filename=ca-cert.pem path=<none>
  qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

- Full context:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
                       -d 'trace:qcrypto*' \
                       -msg guest-name=on,timestamp=on \
                       -name "fish food"
  2025-08-19T20:14:16.791413Z qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55e9a3458d10 dir=fish
  2025-08-19T20:14:16.791429Z qcrypto_tls_creds_get_path TLS creds path creds=0x55e9a3458d10 filename=ca-cert.pem path=<none>
  2025-08-19T20:14:16.791433Z fish food qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

And after this series is complete:

- Default context:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
                      -d 'trace:qcrypto*'
  qemu-system-x86_64(1184284:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55a24ad5cb30 dir=fish
  qemu-system-x86_64(1184284:main): qcrypto_tls_creds_get_path TLS creds path creds=0x55a24ad5cb30 filename=ca-cert.pem path=<none>
  qemu-system-x86_64(1184284:main): Unable to access credentials fish/ca-cert.pem: No such file or directory

- Full context:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
                      -d 'trace:qcrypto*' \
                      -msg guest-name=on,timestamp=on \
                      -name "fish food"
  2025-08-19T20:12:50.211823Z [fish food] qemu-system-x86_64(1168876:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5582183d8760 dir=fish
  2025-08-19T20:12:50.211842Z [fish food] qemu-system-x86_64(1168876:main): qcrypto_tls_creds_get_path TLS creds path creds=0x5582183d8760 filename=ca-cert.pem
+path=<none>
  2025-08-19T20:12:50.211846Z [fish food] qemu-system-x86_64(1168876:main): Unable to access credentials fish/ca-cert.pem: No such file or directory

The main things to note:

 * error_report/warn_report/qemu_log share the same
   output format and -msg applies to both

 * -msg debug-threads=on is now unconditionally enabled
   and thus the param is deprecated & ignored

 * Thread ID and name are unconditionally enabled

 * Guest name is surrounded in [...] brackets

 * The default output lines are typically 15 chars
   wider given that we always include the thread
   ID + name now

 * This takes the liberty of assigning the new file
   to the existing error-report.c maintainer (Markus)
   Since splitting it off into message.c instead of
   putting it all in error-report.c felt slightly
   nicer.

One thing I didn't tackle is making the location
info get reported for qemu_log. This is used to
give context for error messages when parsing some
CLI args, and could be interesting for log messages
associated with those same CLI args.

Changes in v6:

 - Don't use pthread API to set main thread name, as this
   affects visibility in 'top' and 'ps'
 - Split off Win32 thread naming race fix intoseparate
   patch
 - Tweaks to wording in API docs for logging
 - Improve VNC auth error messages
 - Don't relpace monitor_cur_is_qmp with monitor_cur_is_hmp,
   change code approach instead

Changes in v5:

 - Use g_strlcpy instead of strlcpy
 - Remove redundant !! around a value that was already bool
 - Improve the qemu_lock_trylock() API docs to encourage
   use of fprintf() instead of further qemu_log() calls
 - Add a qemu_log_trylock_context() counterpart for
   qemu_log_trylock that includes the message context
 - Don't scan for \n to determine whether we can output
   a message context prefix or not. Instead track call
   depth on qemu_log_trylock() and only output it when
   the depth was initially zero. This fixes linux-user
   -strace output formatting which was mangled by v4
 - Use fprintf(stderr) instead of error_printf_mon
   in cases where mon would be NULL
 - Use fputs/fputc in simple cases where we don't need the
   full power of fprintf
 - Use qemu_f{un}lockfile instead of f{un}lockfile
 - Add  -msg thread-info=on|off,program-name=on|off args
   to allow default message prefixes to be optionally disabled

Changes in v4:

 - Re-architected the way error_vprintf() operates
   to simplify monitor dependency
 - Fixed the move of error_vprintf from monitor.c
   to error-report.c so that unit tests can still
   get a stub to capture messages to g_test_message
 - Simplify changes to vreport() to avoid passing
   around function pointers
 - Add locking of stderr to vreport() to serialize
   incremental output
 - Fix thread naming logic changes on Windows
 - Change deprecation warning messages

Changes in v3:

 - Stop formatting a string in qmessage_context, instead
   output directly to a "FILE *" stream
 - Fix pre-existing bug interleaving qemu_log and
   vreport when the trace 'log' backend has mutex probes
   enabled

Changes in v2:

 - Re-use existing qemu_get_thread_id rather than
   re-inventing it as qemu_thread_get_id.
 - Expose qemu_thread_set_name and use it from all
   locations needing to set thread names
 - Fix qemu_log() to skip context prefix when
   emitting a log message in fragments across
   multiple calls
 - Skip allocating memory for empty context messages
 - Fix leak in win32 impl of qemu_thread_get_name
 - Use g_strlcpy where possible

Daniel P. Berrangé (27):
  meson: don't access 'cxx' object without checking cpp lang
  qemu-options: remove extraneous [] around arg values
  include: define constant for early constructor priority
  monitor: initialize global data from a constructor
  system: unconditionally enable thread naming
  util: fix race setting thread name on Win32
  util: expose qemu_thread_set_name
  audio: make jackaudio use qemu_thread_set_name
  util: set the name for the 'main' thread on Windows
  util: add API to fetch the current thread name
  util: introduce some API docs for logging APIs
  util: avoid repeated prefix on incremental qemu_log calls
  util/log: add missing error reporting in qemu_log_trylock_with_err
  ui: add proper error reporting for password changes
  ui: remove redundant use of error_printf_unless_qmp()
  monitor: remove redundant error_[v]printf_unless_qmp
  monitor: refactor error_vprintf()
  monitor: move error_vprintf back to error-report.c
  util: fix interleaving of error & trace output
  util: don't skip error prefixes when QMP is active
  util: fix interleaving of error prefixes
  util: introduce common helper for error-report & log code
  util: convert error-report & log to message API for timestamp
  util: add support for formatting a workload name in messages
  util: add support for formatting a program name in messages
  util: add support for formatting thread info in messages
  util: add brackets around guest name in message context

 MAINTAINERS                          |   2 +
 audio/jackaudio.c                    |  30 +++++--
 docs/about/deprecated.rst            |   7 ++
 include/monitor/monitor.h            |   3 -
 include/qemu/compiler.h              |   8 ++
 include/qemu/error-report.h          |   4 -
 include/qemu/log-for-trace.h         |  17 +++-
 include/qemu/log.h                   |  39 +++++++++
 include/qemu/message.h               |  40 +++++++++
 include/qemu/thread.h                |   3 +-
 include/ui/console.h                 |   2 +-
 include/ui/qemu-spice-module.h       |   3 +-
 meson.build                          |  23 ++++-
 monitor/monitor.c                    |  51 ++---------
 qemu-options.hx                      | 121 ++++++++++++++-------------
 rust/util/src/log.rs                 |   6 ++
 storage-daemon/qemu-storage-daemon.c |   6 ++
 stubs/error-printf.c                 |  23 -----
 stubs/meson.build                    |   1 -
 stubs/monitor-core.c                 |  19 ++++-
 system/vl.c                          |  49 +++++++++--
 tests/functional/generic/test_vnc.py |   4 +-
 tests/qemu-iotests/041               |   2 +-
 tests/qemu-iotests/common.filter     |   2 +-
 tests/unit/test-error-report.c       |   6 +-
 tests/unit/test-util-sockets.c       |   1 +
 ui/spice-core.c                      |  25 ++++--
 ui/spice-module.c                    |   7 +-
 ui/ui-qmp-cmds.c                     |  19 ++---
 ui/vnc-stubs.c                       |   6 +-
 ui/vnc.c                             |  16 ++--
 util/error-report.c                  | 103 +++++++++++++++--------
 util/log.c                           |  48 ++++++-----
 util/meson.build                     |   1 +
 util/message.c                       |  51 +++++++++++
 util/qemu-thread-posix.c             |  89 ++++++++++++++------
 util/qemu-thread-win32.c             |  87 +++++++++++++------
 37 files changed, 628 insertions(+), 296 deletions(-)
 create mode 100644 include/qemu/message.h
 delete mode 100644 stubs/error-printf.c
 create mode 100644 util/message.c

-- 
2.53.0

[PATCH v6 01/27] meson: don't access 'cxx' object without checking cpp lang

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 2d114e9018..9ad16d1998 100644
--- a/meson.build
+++ b/meson.build
@@ -3194,7 +3194,7 @@ if host_os == 'windows'
 endif
 
 # Detect if ConvertStringToBSTR has been defined in _com_util namespace
-if host_os == 'windows'
+if host_os == 'windows' and 'cpp' in all_languages
   has_convert_string_to_bstr = cxx.links('''
     #include <comutil.h>
     int main() {
-- 
2.53.0

[PATCH v6 02/27] qemu-options: remove extraneous [] around arg values

[Daniel P. Berrangé]

There are quite a few inappropriate uses of [...] around argument
values. The [] are intended to indicate optionality, but in some
cases it is used to wrap a set of enum values. In other cases it
is being used to show the value is entirely optional, which was
common behaviour for boolean values in the past. QEMU has deprecated
short-form boolean options for quite a while though, and we should
thus not advertize this possibility in the docs.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx | 110 ++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 55 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index bd014a3244..cdb7fa20c2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -25,7 +25,7 @@ SRST
 ERST
 
 DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
-    "-machine [type=]name[,prop[=value][,...]]\n"
+    "-machine [type=]name[,prop=value[,...]]\n"
     "                selects emulated machine ('-machine help' for list)\n"
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
     "                supported accelerators are kvm, xen, hvf, nvmm, whpx, mshv or tcg (default: tcg)\n"
@@ -227,7 +227,7 @@ SRST
 ERST
 
 DEF("accel", HAS_ARG, QEMU_OPTION_accel,
-    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
+    "-accel [accel=]accelerator[,prop=value[,...]]\n"
     "                select accelerator (kvm, xen, hvf, nvmm, whpx, mshv or tcg; use 'help' for a list)\n"
     "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
     "                kernel-irqchip=on|off|split controls accelerated irqchip support (default=on)\n"
@@ -789,17 +789,17 @@ ERST
 
 
 DEF("audio", HAS_ARG, QEMU_OPTION_audio,
-    "-audio [driver=]driver[,prop[=value][,...]]\n"
+    "-audio [driver=]driver[,prop=value[,...]]\n"
     "                specifies default audio backend when `audiodev` is not\n"
     "                used to create a machine or sound device;"
     "                options are the same as for -audiodev\n"
-    "-audio [driver=]driver,model=value[,prop[=value][,...]]\n"
+    "-audio [driver=]driver,model=value[,prop=value[,...]]\n"
     "                specifies the audio backend and device to use;\n"
     "                apart from 'model', options are the same as for -audiodev.\n"
     "                use '-audio model=help' to show possible devices.\n",
     QEMU_ARCH_ALL)
 SRST
-``-audio [driver=]driver[,model=value][,prop[=value][,...]]``
+``-audio [driver=]driver[,model=value][,prop=value[,...]]``
     If the ``model`` option is specified, ``-audio`` is a shortcut
     for configuring both the guest audio hardware and the host audio
     backend in one go. The guest hardware model can be set with
@@ -827,7 +827,7 @@ SRST
 ERST
 
 DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
-    "-audiodev [driver=]driver,id=id[,prop[=value][,...]]\n"
+    "-audiodev [driver=]driver,id=id[,prop=value[,...]]\n"
     "                specifies the audio backend to use\n"
     "                Use ``-audiodev help`` to list the available drivers\n"
     "                id= identifier of the backend\n"
@@ -840,25 +840,25 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
     "                valid values: s8, s16, s32, u8, u16, u32, f32\n"
     "                in|out.voices= number of voices to use\n"
     "                in|out.buffer-length= length of buffer in microseconds\n"
-    "-audiodev none,id=id,[,prop[=value][,...]]\n"
+    "-audiodev none,id=id,[,prop=value[,...]]\n"
     "                dummy driver that discards all output\n"
 #ifdef CONFIG_AUDIO_ALSA
-    "-audiodev alsa,id=id[,prop[=value][,...]]\n"
+    "-audiodev alsa,id=id[,prop=value[,...]]\n"
     "                in|out.dev= name of the audio device to use\n"
     "                in|out.period-length= length of period in microseconds\n"
     "                in|out.try-poll= attempt to use poll mode\n"
     "                threshold= threshold (in microseconds) when playback starts\n"
 #endif
 #ifdef CONFIG_AUDIO_COREAUDIO
-    "-audiodev coreaudio,id=id[,prop[=value][,...]]\n"
+    "-audiodev coreaudio,id=id[,prop=value[,...]]\n"
     "                in|out.buffer-count= number of buffers\n"
 #endif
 #ifdef CONFIG_AUDIO_DSOUND
-    "-audiodev dsound,id=id[,prop[=value][,...]]\n"
+    "-audiodev dsound,id=id[,prop=value[,...]]\n"
     "                latency= add extra latency to playback in microseconds\n"
 #endif
 #ifdef CONFIG_AUDIO_OSS
-    "-audiodev oss,id=id[,prop[=value][,...]]\n"
+    "-audiodev oss,id=id[,prop=value[,...]]\n"
     "                in|out.dev= path of the audio device to use\n"
     "                in|out.buffer-count= number of buffers\n"
     "                in|out.try-poll= attempt to use poll mode\n"
@@ -867,35 +867,35 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
     "                dsp-policy= set timing policy (0..10), -1 to use fragment mode\n"
 #endif
 #ifdef CONFIG_AUDIO_PA
-    "-audiodev pa,id=id[,prop[=value][,...]]\n"
+    "-audiodev pa,id=id[,prop=value[,...]]\n"
     "                server= PulseAudio server address\n"
     "                in|out.name= source/sink device name\n"
     "                in|out.latency= desired latency in microseconds\n"
 #endif
 #ifdef CONFIG_AUDIO_PIPEWIRE
-    "-audiodev pipewire,id=id[,prop[=value][,...]]\n"
+    "-audiodev pipewire,id=id[,prop=value[,...]]\n"
     "                in|out.name= source/sink device name\n"
     "                in|out.stream-name= name of pipewire stream\n"
     "                in|out.latency= desired latency in microseconds\n"
 #endif
 #ifdef CONFIG_AUDIO_SDL
-    "-audiodev sdl,id=id[,prop[=value][,...]]\n"
+    "-audiodev sdl,id=id[,prop=value[,...]]\n"
     "                in|out.buffer-count= number of buffers\n"
 #endif
 #ifdef CONFIG_AUDIO_SNDIO
-    "-audiodev sndio,id=id[,prop[=value][,...]]\n"
+    "-audiodev sndio,id=id[,prop=value[,...]]\n"
 #endif
 #ifdef CONFIG_SPICE
-    "-audiodev spice,id=id[,prop[=value][,...]]\n"
+    "-audiodev spice,id=id[,prop=value[,...]]\n"
 #endif
 #ifdef CONFIG_DBUS_DISPLAY
-    "-audiodev dbus,id=id[,prop[=value][,...]]\n"
+    "-audiodev dbus,id=id[,prop=value[,...]]\n"
 #endif
-    "-audiodev wav,id=id[,prop[=value][,...]]\n"
+    "-audiodev wav,id=id[,prop=value[,...]]\n"
     "                path= path of wav file to record\n",
     QEMU_ARCH_ALL)
 SRST
-``-audiodev [driver=]driver,id=id[,prop[=value][,...]]``
+``-audiodev [driver=]driver,id=id[,prop=value[,...]]``
     Adds a new audio backend driver identified by id. There are global
     and driver specific properties. Some values can be set differently
     for input and output, they're marked with ``in|out.``. You can set
@@ -954,11 +954,11 @@ SRST
     ``in|out.buffer-length=usecs``
         Sets the size of the buffer in microseconds.
 
-``-audiodev none,id=id[,prop[=value][,...]]``
+``-audiodev none,id=id[,prop=value[,...]]``
     Creates a dummy backend that discards all outputs. This backend has
     no backend specific properties.
 
-``-audiodev alsa,id=id[,prop[=value][,...]]``
+``-audiodev alsa,id=id[,prop=value[,...]]``
     Creates backend using the ALSA. This backend is only available on
     Linux.
 
@@ -977,7 +977,7 @@ SRST
     ``threshold=threshold``
         Threshold (in microseconds) when playback starts. Default is 0.
 
-``-audiodev coreaudio,id=id[,prop[=value][,...]]``
+``-audiodev coreaudio,id=id[,prop=value[,...]]``
     Creates a backend using Apple's Core Audio. This backend is only
     available on Mac OS and only supports playback.
 
@@ -986,7 +986,7 @@ SRST
     ``in|out.buffer-count=count``
         Sets the count of the buffers.
 
-``-audiodev dsound,id=id[,prop[=value][,...]]``
+``-audiodev dsound,id=id[,prop=value[,...]]``
     Creates a backend using Microsoft's DirectSound. This backend is
     only available on Windows and only supports playback.
 
@@ -996,7 +996,7 @@ SRST
         Add extra usecs microseconds latency to playback. Default is
         10000 (10 ms).
 
-``-audiodev oss,id=id[,prop[=value][,...]]``
+``-audiodev oss,id=id[,prop=value[,...]]``
     Creates a backend using OSS. This backend is available on most
     Unix-like systems.
 
@@ -1025,7 +1025,7 @@ SRST
         buffer sizes specified by ``buffer`` and ``buffer-count``. This
         option is ignored if you do not have OSS 4. Default is 5.
 
-``-audiodev pa,id=id[,prop[=value][,...]]``
+``-audiodev pa,id=id[,prop=value[,...]]``
     Creates a backend using PulseAudio. This backend is available on
     most systems.
 
@@ -1041,7 +1041,7 @@ SRST
         Desired latency in microseconds. The PulseAudio server will try
         to honor this value but actual latencies may be lower or higher.
 
-``-audiodev pipewire,id=id[,prop[=value][,...]]``
+``-audiodev pipewire,id=id[,prop=value[,...]]``
     Creates a backend using PipeWire. This backend is available on
     most systems.
 
@@ -1056,7 +1056,7 @@ SRST
     ``in|out.stream-name``
         Specify the name of pipewire stream.
 
-``-audiodev sdl,id=id[,prop[=value][,...]]``
+``-audiodev sdl,id=id[,prop=value[,...]]``
     Creates a backend using SDL. This backend is available on most
     systems, but you should use your platform's native backend if
     possible.
@@ -1066,7 +1066,7 @@ SRST
     ``in|out.buffer-count=count``
         Sets the count of the buffers.
 
-``-audiodev sndio,id=id[,prop[=value][,...]]``
+``-audiodev sndio,id=id[,prop=value[,...]]``
     Creates a backend using SNDIO. This backend is available on
     OpenBSD and most other Unix-like systems.
 
@@ -1079,13 +1079,13 @@ SRST
     ``in|out.latency=usecs``
         Sets the desired period length in microseconds.
 
-``-audiodev spice,id=id[,prop[=value][,...]]``
+``-audiodev spice,id=id[,prop=value[,...]]``
     Creates a backend that sends audio through SPICE. This backend
     requires ``-spice`` and automatically selected in that case, so
     usually you can ignore this option. This backend has no backend
     specific properties.
 
-``-audiodev wav,id=id[,prop[=value][,...]]``
+``-audiodev wav,id=id[,prop=value[,...]]``
     Creates a backend that writes audio to a WAV file.
 
     Backend specific options are:
@@ -1096,21 +1096,21 @@ SRST
 ERST
 
 DEF("device", HAS_ARG, QEMU_OPTION_device,
-    "-device driver[,prop[=value][,...]]\n"
+    "-device driver[,prop=value[,...]]\n"
     "                add device (based on driver)\n"
     "                prop=value,... sets driver properties\n"
     "                use '-device help' to print all possible drivers\n"
     "                use '-device driver,help' to print all possible properties\n",
     QEMU_ARCH_ALL)
 SRST
-``-device driver[,prop[=value][,...]]``
+``-device driver[,prop=value[,...]]``
     Add device driver. prop=value sets driver properties. Valid
     properties depend on the driver. To get help on possible drivers and
     properties, use ``-device help`` and ``-device driver,help``.
 
     Some drivers are:
 
-``-device ipmi-bmc-sim,id=id[,prop[=value][,...]]``
+``-device ipmi-bmc-sim,id=id[,prop=value[,...]]``
     Add an IPMI BMC. This is a simulation of a hardware management
     interface processor that normally sits on a system. It provides a
     watchdog and the ability to reset and power control the system. You
@@ -2308,19 +2308,19 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,x509-dh-key-file=<file>][,addr=addr]\n"
     "       [,ipv4=on|off][,ipv6=on|off][,unix=on|off]\n"
     "       [,tls-ciphers=<list>]\n"
-    "       [,tls-channel=[main|display|cursor|inputs|record|playback]]\n"
-    "       [,plaintext-channel=[main|display|cursor|inputs|record|playback]]\n"
+    "       [,tls-channel=main|display|cursor|inputs|record|playback]\n"
+    "       [,plaintext-channel=main|display|cursor|inputs|record|playback]\n"
     "       [,sasl=on|off][,disable-ticketing=on|off]\n"
     "       [,password-secret=<secret-id>]\n"
-    "       [,image-compression=[auto_glz|auto_lz|quic|glz|lz|off]]\n"
-    "       [,jpeg-wan-compression=[auto|never|always]]\n"
-    "       [,zlib-glz-wan-compression=[auto|never|always]]\n"
-    "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
-    "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
-    "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
+    "       [,image-compression=auto_glz|auto_lz|quic|glz|lz|off]\n"
+    "       [,jpeg-wan-compression=auto|never|always]\n"
+    "       [,zlib-glz-wan-compression=auto|never|always]\n"
+    "       [,streaming-video=off|all|filter][,disable-copy-paste=on|off]\n"
+    "       [,disable-agent-file-xfer=on|off][,agent-mouse=on|off]\n"
+    "       [,playback-compression=on|off][,seamless-migration=on|off]\n"
     "       [,video-codec=<codec>\n"
     "       [,max-refresh-rate=rate\n"
-    "       [,gl=[on|off]][,rendernode=<file>]\n"
+    "       [,gl=on|off][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
     QEMU_ARCH_ALL)
@@ -2380,7 +2380,7 @@ SRST
     ``tls-ciphers=<list>``
         Specify which ciphers to use.
 
-    ``tls-channel=[main|display|cursor|inputs|record|playback]``; \ ``plaintext-channel=[main|display|cursor|inputs|record|playback]``
+    ``tls-channel=main|display|cursor|inputs|record|playback``; \ ``plaintext-channel=main|display|cursor|inputs|record|playback``
         Force specific channel to be used with or without TLS
         encryption. The options can be specified multiple times to
         configure multiple channels. The special name "default" can be
@@ -2388,24 +2388,24 @@ SRST
         explicitly forced into one mode the spice client is allowed to
         pick tls/plaintext as he pleases.
 
-    ``image-compression=[auto_glz|auto_lz|quic|glz|lz|off]``
+    ``image-compression=auto_glz|auto_lz|quic|glz|lz|off``
         Configure image compression (lossless). Default is auto\_glz.
 
-    ``jpeg-wan-compression=[auto|never|always]``; \ ``zlib-glz-wan-compression=[auto|never|always]``
+    ``jpeg-wan-compression=auto|never|always``; \ ``zlib-glz-wan-compression=auto|never|always``
         Configure wan image compression (lossy for slow links). Default
         is auto.
 
-    ``streaming-video=[off|all|filter]``
+    ``streaming-video=off|all|filter``
         Configure video stream detection. Default is off.
 
-    ``agent-mouse=[on|off]``
+    ``agent-mouse=on|off``
         Enable/disable passing mouse events via vdagent. Default is on.
 
-    ``playback-compression=[on|off]``
+    ``playback-compression=on|off``
         Enable/disable audio stream compression (using celt 0.5.1).
         Default is on.
 
-    ``seamless-migration=[on|off]``
+    ``seamless-migration=on|off``
         Enable/disable spice seamless migration. Default is off.
 
     ``video-codec=<codec>``
@@ -2419,7 +2419,7 @@ SRST
         Provide the maximum refresh rate (or FPS) at which the encoding
         requests should be sent to the Spice server. Default would be 30.
 
-    ``gl=[on|off]``
+    ``gl=on|off``
         Enable/disable OpenGL context. Default is off.
 
     ``rendernode=<file>``
@@ -2649,7 +2649,7 @@ SRST
         bandwidth when playing videos. Disabling adaptive encodings
         restores the original static behavior of encodings like Tight.
 
-    ``share=[allow-exclusive|force-shared|ignore]``
+    ``share=allow-exclusive|force-shared|ignore``
         Set display sharing policy. 'allow-exclusive' allows clients to
         ask for exclusive access. As suggested by the rfb spec this is
         implemented by dropping other connections. Connecting multiple
@@ -4823,9 +4823,9 @@ SRST
 ERST
 
 DEF("mon", HAS_ARG, QEMU_OPTION_mon, \
-    "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]\n", QEMU_ARCH_ALL)
+    "-mon [chardev=]name[,mode=readline|control][,pretty=on|off]\n", QEMU_ARCH_ALL)
 SRST
-``-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]``
+``-mon [chardev=]name[,mode=readline|control][,pretty=on|off]``
     Set up a monitor connected to the chardev ``name``.
     QEMU supports two monitors: the Human Monitor Protocol
     (HMP; for human interaction), and the QEMU Monitor Protocol
@@ -5514,14 +5514,14 @@ ERST
 #endif
 
 DEF("msg", HAS_ARG, QEMU_OPTION_msg,
-    "-msg [timestamp[=on|off]][,guest-name=[on|off]]\n"
+    "-msg [timestamp=on|off][,guest-name=on|off]\n"
     "                control error message format\n"
     "                timestamp=on enables timestamps (default: off)\n"
     "                guest-name=on enables guest name prefix but only if\n"
     "                              -name guest option is set (default: off)\n",
     QEMU_ARCH_ALL)
 SRST
-``-msg [timestamp[=on|off]][,guest-name[=on|off]]``
+``-msg [timestamp=on|off][,guest-name=on|off]``
     Control error message format.
 
     ``timestamp=on|off``
-- 
2.53.0

[PATCH v6 03/27] include: define constant for early constructor priority

[Daniel P. Berrangé]

Functions marked with __attribute__((__constructor__)) will be
invoked in linker order. In theory this is well defined, but
in practice, it is hard to determine what this order will be
with the layers of indirection through meson, ninja and the
static libraries QEMU builds.

Notably, the order currently appears different between Linux
and Windows (as tested with Wine on Linux). This can cause
problems when certain QEMU constructors have a dependancy on
other QEMU constructors.

To address this define a QEMU_CONSTRUCTOR_EARLY constant which
provides a priority value that will run before other default
constructors. This is to be used for QEMU constructors that
are themselves self-contained, but may be relied upon by other
constructors.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/compiler.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 1c2b673c05..4c49f52eb0 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -194,6 +194,14 @@
 # define QEMU_USED
 #endif
 
+/*
+ * A priority for __attribute__((constructor(...))) that
+ * will run earlier than the default constructors. Must
+ * only be used for functions that have no dependency
+ * on global initialization of other QEMU subsystems.
+ */
+#define QEMU_CONSTRUCTOR_EARLY 101
+
 /*
  * Disable -ftrivial-auto-var-init on a local variable.
  *
-- 
2.53.0

[PATCH v6 04/27] monitor: initialize global data from a constructor

[Daniel P. Berrangé]

Some monitor functions, most notably, monitor_cur() rely on global
data being initialized by 'monitor_init_globals()'. The latter is
called relatively late in startup. If code triggers error_report()
before monitor_init_globals() is called, QEMU will abort when
accessing the uninitialized monitor mutex.

The critical monitor global data must be initialized from a
constructor function, to improve the guarantee that it is done
before any possible calls to monitor_cur(). Not only that, but
the constructor must be marked to run before the default
constructor in case any of them trigger error reporting.

Note in particular that the RCU constructor will spawn a background
thread so we might even have non-constructor QEMU code running
concurrently with other constructors.

As a general note, constructors should be extrememly careful
about what QEMU code they invoke, as it cannot be guaranteed that
the process is fully initialized and so not all normal QEMU API
rules apply.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Fixes: e69ee454b5f9 (monitor: Make current monitor a per-coroutine property)
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/monitor.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 1273eb7260..dd2c45ded0 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -708,18 +708,22 @@ void monitor_cleanup(void)
     }
 }
 
-static void monitor_qapi_event_init(void)
+/*
+ * Initialize static vars that have no deps on external
+ * module initialization, and are required for external
+ * functions to call things like monitor_cur()
+ */
+static void __attribute__((__constructor__(QEMU_CONSTRUCTOR_EARLY)))
+monitor_init_static(void)
 {
+    qemu_mutex_init(&monitor_lock);
+    coroutine_mon = g_hash_table_new(NULL, NULL);
     monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash,
                                                 qapi_event_throttle_equal);
 }
 
 void monitor_init_globals(void)
 {
-    monitor_qapi_event_init();
-    qemu_mutex_init(&monitor_lock);
-    coroutine_mon = g_hash_table_new(NULL, NULL);
-
     /*
      * The dispatcher BH must run in the main loop thread, since we
      * have commands assuming that context.  It would be nice to get
-- 
2.53.0

[PATCH v6 05/27] system: unconditionally enable thread naming

[Daniel P. Berrangé]

When thread naming was introduced years ago, it was disabled by
default and put behind a command line flag:

  commit 8f480de0c91a18d550721f8d9af969ebfbda0793
  Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
  Date:   Thu Jan 30 10:20:31 2014 +0000

    Add 'debug-threads' suboption to --name

This was done based on a concern that something might depend
on the historical thread naming. Thread names, however, were
never promised to be part of QEMU's public API. The defaults
will vary across platforms, so no assumptions should ever be
made about naming.

An opt-in behaviour is also unfortunately incompatible with
RCU which creates its thread from an constructor function
which is run before command line args are parsed. Thus the
RCU thread lacks any name.

libvirt has unconditionally enabled debug-threads=yes on all
VMs it creates for 10 years. Interestingly this DID expose a
bug in libvirt, as it parsed /proc/$PID/stat and could not
cope with a space in the thread name. This was a latent
pre-existing bug in libvirt though, and not a part of QEMU's
API.

Having thread names always available, will allow thread names
to be included in error reports and log messags QEMU prints
by default, which will improve ability to triage QEMU bugs.

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/about/deprecated.rst |  7 +++++++
 include/qemu/thread.h     |  1 -
 system/vl.c               | 12 +++++++-----
 util/qemu-thread-posix.c  | 18 +-----------------
 util/qemu-thread-win32.c  | 27 ++++++---------------------
 5 files changed, 21 insertions(+), 44 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 1d5c4f3707..568a48f21c 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -54,6 +54,13 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
 However, short-form booleans are deprecated and full explicit ``arg_name=on``
 form is preferred.
 
+``debug-threads`` option for ``-name`` (since 11.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The ``debug-threads`` option of the ``-name`` argument is now
+ignored. Thread naming is unconditionally enabled for all platforms
+where it is supported.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index f0302ed01f..3a286bb3ef 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -215,7 +215,6 @@ void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 G_NORETURN void qemu_thread_exit(void *retval);
-void qemu_thread_naming(bool enable);
 
 struct Notifier;
 /**
diff --git a/system/vl.c b/system/vl.c
index aa9a155041..d8676b3adf 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -403,9 +403,8 @@ static QemuOptsList qemu_name_opts = {
         }, {
             .name = "debug-threads",
             .type = QEMU_OPT_BOOL,
-            .help = "When enabled, name the individual threads; defaults off.\n"
-                    "NOTE: The thread names are for debugging and not a\n"
-                    "stable API.",
+            .help = "Enable thread names"
+                    "(deprecated, always enabled where supported)",
         },
         { /* End of list */ }
     },
@@ -554,9 +553,12 @@ static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *proc_name;
 
-    if (qemu_opt_get(opts, "debug-threads")) {
-        qemu_thread_naming(qemu_opt_get_bool(opts, "debug-threads", false));
+    if (qemu_opt_get(opts, "debug-threads") &&
+        !qemu_opt_get_bool(opts, "debug-threads", false)) {
+        fprintf(stderr, "Ignoring deprecated 'debug-threads=no' option, " \
+                "thread naming is unconditionally enabled\n");
     }
+
     qemu_name = qemu_opt_get(opts, "guest");
 
     proc_name = qemu_opt_get(opts, "process");
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index ba725444ba..7c985b5d38 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -22,22 +22,6 @@
 #include <pthread_np.h>
 #endif
 
-static bool name_threads;
-
-void qemu_thread_naming(bool enable)
-{
-    name_threads = enable;
-
-#if !defined CONFIG_PTHREAD_SETNAME_NP_W_TID && \
-    !defined CONFIG_PTHREAD_SETNAME_NP_WO_TID && \
-    !defined CONFIG_PTHREAD_SET_NAME_NP
-    /* This is a debugging option, not fatal */
-    if (enable) {
-        fprintf(stderr, "qemu: thread naming not supported on this host\n");
-    }
-#endif
-}
-
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
@@ -361,7 +345,7 @@ static void *qemu_thread_start(void *args)
     /* Attempt to set the threads name; note that this is for debug, so
      * we're not going to fail if we can't set it.
      */
-    if (name_threads && qemu_thread_args->name) {
+    if (qemu_thread_args->name) {
 # if defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
         pthread_setname_np(pthread_self(), qemu_thread_args->name);
 # elif defined(CONFIG_PTHREAD_SETNAME_NP_WO_TID)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index ca2e0b512e..9595a5b090 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -17,8 +17,6 @@
 #include "qemu-thread-common.h"
 #include <process.h>
 
-static bool name_threads;
-
 typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
                                                  PCWSTR lpThreadDescription);
 static pSetThreadDescription SetThreadDescriptionFunc;
@@ -44,16 +42,6 @@ static bool load_set_thread_description(void)
     return !!SetThreadDescriptionFunc;
 }
 
-void qemu_thread_naming(bool enable)
-{
-    name_threads = enable;
-
-    if (enable && !load_set_thread_description()) {
-        fprintf(stderr, "qemu: thread naming not supported on this host\n");
-        name_threads = false;
-    }
-}
-
 static void error_exit(int err, const char *msg)
 {
     char *pstr;
@@ -328,23 +316,20 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-static bool set_thread_description(HANDLE h, const char *name)
+static void set_thread_description(HANDLE h, const char *name)
 {
-    HRESULT hr;
     g_autofree wchar_t *namew = NULL;
 
     if (!load_set_thread_description()) {
-        return false;
+        return;
     }
 
     namew = g_utf8_to_utf16(name, -1, NULL, NULL, NULL);
     if (!namew) {
-        return false;
+        return;
     }
 
-    hr = SetThreadDescriptionFunc(h, namew);
-
-    return SUCCEEDED(hr);
+    SetThreadDescriptionFunc(h, namew);
 }
 
 void qemu_thread_create(QemuThread *thread, const char *name,
@@ -370,8 +355,8 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     if (!hThread) {
         error_exit(GetLastError(), __func__);
     }
-    if (name_threads && name && !set_thread_description(hThread, name)) {
-        fprintf(stderr, "qemu: failed to set thread description: %s\n", name);
+    if (name) {
+        set_thread_description(hThread, name);
     }
     CloseHandle(hThread);
 
-- 
2.53.0

[PATCH v6 06/27] util: fix race setting thread name on Win32

[Daniel P. Berrangé]

The call to set the thread name on Win32 platforms is done by the parent
thread, after _beginthreadex() returns. At this point the new child
thread is potentially already executing its start method. To ensure the
thread name is guaranteed to be set before any "interesting" code starts
executing, it must be done in the start method of the child thread itself.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-thread-win32.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 9595a5b090..255c88571f 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -22,6 +22,8 @@ typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
 static pSetThreadDescription SetThreadDescriptionFunc;
 static HMODULE kernel32_module;
 
+static void set_thread_description(const char *name);
+
 static bool load_set_thread_description(void)
 {
     static gsize _init_once = 0;
@@ -225,6 +227,7 @@ struct QemuThreadData {
     void             *arg;
     short             mode;
     NotifierList      exit;
+    char             *name; /* Freed in win32_start_routine */
 
     /* Only used for joinable threads. */
     bool              exited;
@@ -266,6 +269,10 @@ static unsigned __stdcall win32_start_routine(void *arg)
     void *(*start_routine)(void *) = data->start_routine;
     void *thread_arg = data->arg;
 
+    if (data->name) {
+        set_thread_description(data->name);
+        g_clear_pointer(&data->name, g_free);
+    }
     qemu_thread_data = data;
     qemu_thread_exit(start_routine(thread_arg));
     abort();
@@ -316,7 +323,7 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-static void set_thread_description(HANDLE h, const char *name)
+static void set_thread_description(const char *name)
 {
     g_autofree wchar_t *namew = NULL;
 
@@ -329,7 +336,7 @@ static void set_thread_description(HANDLE h, const char *name)
         return;
     }
 
-    SetThreadDescriptionFunc(h, namew);
+    SetThreadDescriptionFunc(GetCurrentThread(), namew);
 }
 
 void qemu_thread_create(QemuThread *thread, const char *name,
@@ -344,6 +351,7 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     data->arg = arg;
     data->mode = mode;
     data->exited = false;
+    data->name = g_strdup(name);
     notifier_list_init(&data->exit);
 
     if (data->mode != QEMU_THREAD_DETACHED) {
@@ -355,9 +363,6 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     if (!hThread) {
         error_exit(GetLastError(), __func__);
     }
-    if (name) {
-        set_thread_description(hThread, name);
-    }
     CloseHandle(hThread);
 
     thread->data = data;
-- 
2.53.0

[PATCH v6 07/27] util: expose qemu_thread_set_name

[Daniel P. Berrangé]

The ability to set the thread name needs to be used in a number
of places, so expose the current impls as public methods.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/thread.h    |  1 +
 util/qemu-thread-posix.c | 26 ++++++++++++++++----------
 util/qemu-thread-win32.c |  6 ++----
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 3a286bb3ef..27b888ab0a 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -215,6 +215,7 @@ void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 G_NORETURN void qemu_thread_exit(void *retval);
+void qemu_thread_set_name(const char *name);
 
 struct Notifier;
 /**
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 7c985b5d38..b1c127dbe3 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -329,6 +329,21 @@ static void qemu_thread_atexit_notify(void *arg)
     notifier_list_notify(&thread_exit, NULL);
 }
 
+void qemu_thread_set_name(const char *name)
+{
+    /*
+     * Attempt to set the threads name; note that this is for debug, so
+     * we're not going to fail if we can't set it.
+     */
+# if defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
+    pthread_setname_np(pthread_self(), name);
+# elif defined(CONFIG_PTHREAD_SETNAME_NP_WO_TID)
+    pthread_setname_np(name);
+# elif defined(CONFIG_PTHREAD_SET_NAME_NP)
+    pthread_set_name_np(pthread_self(), name);
+# endif
+}
+
 typedef struct {
     void *(*start_routine)(void *);
     void *arg;
@@ -342,17 +357,8 @@ static void *qemu_thread_start(void *args)
     void *arg = qemu_thread_args->arg;
     void *r;
 
-    /* Attempt to set the threads name; note that this is for debug, so
-     * we're not going to fail if we can't set it.
-     */
     if (qemu_thread_args->name) {
-# if defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
-        pthread_setname_np(pthread_self(), qemu_thread_args->name);
-# elif defined(CONFIG_PTHREAD_SETNAME_NP_WO_TID)
-        pthread_setname_np(qemu_thread_args->name);
-# elif defined(CONFIG_PTHREAD_SET_NAME_NP)
-        pthread_set_name_np(pthread_self(), qemu_thread_args->name);
-# endif
+        qemu_thread_set_name(qemu_thread_args->name);
     }
     QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
     g_free(qemu_thread_args->name);
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 255c88571f..31ec305a73 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -22,8 +22,6 @@ typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
 static pSetThreadDescription SetThreadDescriptionFunc;
 static HMODULE kernel32_module;
 
-static void set_thread_description(const char *name);
-
 static bool load_set_thread_description(void)
 {
     static gsize _init_once = 0;
@@ -270,7 +268,7 @@ static unsigned __stdcall win32_start_routine(void *arg)
     void *thread_arg = data->arg;
 
     if (data->name) {
-        set_thread_description(data->name);
+        qemu_thread_set_name(data->name);
         g_clear_pointer(&data->name, g_free);
     }
     qemu_thread_data = data;
@@ -323,7 +321,7 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-static void set_thread_description(const char *name)
+void qemu_thread_set_name(const char *name)
 {
     g_autofree wchar_t *namew = NULL;
 
-- 
2.53.0

[PATCH v6 08/27] audio: make jackaudio use qemu_thread_set_name

[Daniel P. Berrangé]

This has greater portability than directly call pthread_setname_np,
which is only 1 out of 3 possible functions for pthreads that can
set the name.

The new API requires a trampoline function, since it can only set
the name of the current thread.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 audio/jackaudio.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 7a3fcaedba..c47a1a3781 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -629,18 +629,36 @@ static void qjack_enable_in(HWVoiceIn *hw, bool enable)
     ji->c.enabled = enable;
 }
 
-#if !defined(WIN32) && defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
+#if !defined(WIN32)
+struct QJackThreadData {
+    void *(*function)(void *);
+    void *arg;
+};
+
+static void *qjack_thread_trampoline(void *targ)
+{
+    struct QJackThreadData *data = targ;
+    void *(*function)(void *) = data->function;
+    void *arg = data->arg;
+
+    g_free(data);
+    qemu_thread_set_name("jack-client");
+
+    return function(arg);
+}
+
 static int qjack_thread_creator(jack_native_thread_t *thread,
     const pthread_attr_t *attr, void *(*function)(void *), void *arg)
 {
-    int ret = pthread_create(thread, attr, function, arg);
+    struct QJackThreadData *data = g_new0(struct QJackThreadData, 1);
+    data->function = function;
+    data->arg = arg;
+    int ret = pthread_create(thread, attr, qjack_thread_trampoline, data);
     if (ret != 0) {
+        g_free(data);
         return ret;
     }
 
-    /* set the name of the thread */
-    pthread_setname_np(*thread, "jack-client");
-
     return ret;
 }
 #endif
@@ -695,7 +713,7 @@ static void register_audio_jack(void)
 {
     qemu_mutex_init(&qjack_shutdown_lock);
     audio_driver_register(&jack_driver);
-#if !defined(WIN32) && defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
+#if !defined(WIN32)
     jack_set_thread_creator(qjack_thread_creator);
 #endif
     jack_set_error_function(qjack_error);
-- 
2.53.0

[PATCH v6 09/27] util: set the name for the 'main' thread on Windows

[Daniel P. Berrangé]

The default main thread name is undefined, so use a constructor to
explicitly set it to 'main'. This constructor is marked to run early
as the thread name is intended to be used in error reporting / logs
which may be triggered very early in QEMU execution.

This is only done on Windows platforms, because on Linux (and possibly
other POSIX) changing the main thread name has a side effect of changing
the process name reported by tools like 'ps' which fetch from the file
/proc/self/task/tid/comm, expecting it to be the binary name.

The following patch will address POSIX platforms in a different way.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-thread-win32.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 31ec305a73..c330d924af 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -22,6 +22,12 @@ typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
 static pSetThreadDescription SetThreadDescriptionFunc;
 static HMODULE kernel32_module;
 
+static void __attribute__((__constructor__(QEMU_CONSTRUCTOR_EARLY)))
+qemu_thread_init(void)
+{
+    qemu_thread_set_name("main");
+}
+
 static bool load_set_thread_description(void)
 {
     static gsize _init_once = 0;
-- 
2.53.0

[PATCH v6 10/27] util: add API to fetch the current thread name

[Daniel P. Berrangé]

This will be used to include the thread name in error reports
in a later patch. It returns a const string stored in a thread
local to avoid memory allocation when it is called repeatedly
in a single thread. The thread name should be set at the very
start of the thread execution, which is the case when using
qemu_thread_create.

This uses the official thread APIs for fetching thread names,
so that it captures  names of threads spawned by code in 3rd
party libraries, not merely QEMU spawned thrads.

This also addresses the gap from the previous patch for setting
the name of the main thread. A constructor is used to initialize
the 'namebuf' thread-local in the main thread only.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/thread.h    |  1 +
 meson.build              | 21 ++++++++++++++++
 util/qemu-thread-posix.c | 53 +++++++++++++++++++++++++++++++++++++++-
 util/qemu-thread-win32.c | 49 ++++++++++++++++++++++++++++++++++---
 4 files changed, 119 insertions(+), 5 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 27b888ab0a..98cc5c41ac 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -216,6 +216,7 @@ void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 G_NORETURN void qemu_thread_exit(void *retval);
 void qemu_thread_set_name(const char *name);
+const char *qemu_thread_get_name(void);
 
 struct Notifier;
 /**
diff --git a/meson.build b/meson.build
index 9ad16d1998..710aa2ed83 100644
--- a/meson.build
+++ b/meson.build
@@ -2845,6 +2845,27 @@ config_host_data.set('CONFIG_PTHREAD_SET_NAME_NP', cc.links(osdep_prefix + '''
     pthread_set_name_np(thread, "QEMU");
     return 0;
   }''', dependencies: threads))
+
+config_host_data.set('CONFIG_PTHREAD_GETNAME_NP', cc.links(osdep_prefix + '''
+  #include <pthread.h>
+
+  int main(void)
+  {
+    char buf[16];
+    pthread_getname_np(pthread_self(), buf, sizeof(buf));
+    return 0;
+  }''', dependencies: threads))
+config_host_data.set('CONFIG_PTHREAD_GET_NAME_NP', cc.links(osdep_prefix + '''
+  #include <pthread.h>
+  #include <pthread_np.h>
+
+  int main(void)
+  {
+    char buf[16];
+    pthread_get_name_np(pthread_self(), buf, sizeof(buf));
+    return 0;
+  }''', dependencies: threads))
+
 config_host_data.set('CONFIG_PTHREAD_CONDATTR_SETCLOCK', cc.links(osdep_prefix + '''
   #include <pthread.h>
 
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index b1c127dbe3..2b180d99ae 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -18,10 +18,41 @@
 #include "qemu/tsan.h"
 #include "qemu/bitmap.h"
 
-#ifdef CONFIG_PTHREAD_SET_NAME_NP
+#if defined(CONFIG_PTHREAD_SET_NAME_NP) || defined(CONFIG_PTHREAD_GET_NAME_NP)
 #include <pthread_np.h>
 #endif
 
+/*
+ * This is not defined on Linux, but the man page indicates
+ * the buffer must be at least 16 bytes, including the NUL
+ * terminator
+ */
+#ifndef PTHREAD_MAX_NAMELEN_NP
+#define PTHREAD_MAX_NAMELEN_NP 16
+#endif
+
+static __thread char namebuf[PTHREAD_MAX_NAMELEN_NP];
+
+static void __attribute__((__constructor__(QEMU_CONSTRUCTOR_EARLY)))
+qemu_thread_init(void)
+{
+    /*
+     * Initialize the main thread name. We must not use
+     * qemu_thread_setname(), since on some platforms (at least Linux)
+     * this can change the process name that is reported by tools like
+     * 'ps'.
+     *
+     * This workaround suffices to ensure QEMU log/error messages
+     * get the main thread name, but at the cost of external tools
+     * like GDB not seeing it.
+     *
+     * NB using a constructor instead of static initializing namebuf,
+     * to ensure it only initializes the thread-local in the main
+     * thread
+     */
+    g_strlcpy(namebuf, "main", sizeof(namebuf));
+}
+
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
@@ -526,3 +557,23 @@ void *qemu_thread_join(QemuThread *thread)
     }
     return ret;
 }
+
+const char *qemu_thread_get_name(void)
+{
+    int rv;
+    if (namebuf[0] != '\0') {
+        return namebuf;
+    }
+
+# if defined(CONFIG_PTHREAD_GETNAME_NP)
+    rv = pthread_getname_np(pthread_self(), namebuf, sizeof(namebuf));
+# elif defined(CONFIG_PTHREAD_GET_NAME_NP)
+    rv = pthread_get_name_np(pthread_self(), namebuf, sizeof(namebuf));
+# else
+    rv = -1;
+# endif
+    if (rv != 0) {
+        g_strlcpy(namebuf, "unnamed", G_N_ELEMENTS(namebuf));
+    }
+    return namebuf;
+}
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index c330d924af..272afc3385 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -19,7 +19,10 @@
 
 typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
                                                  PCWSTR lpThreadDescription);
+typedef HRESULT (WINAPI *pGetThreadDescription) (HANDLE hThread,
+                                                 PWSTR *lpThreadDescription);
 static pSetThreadDescription SetThreadDescriptionFunc;
+static pGetThreadDescription GetThreadDescriptionFunc;
 static HMODULE kernel32_module;
 
 static void __attribute__((__constructor__(QEMU_CONSTRUCTOR_EARLY)))
@@ -28,7 +31,7 @@ qemu_thread_init(void)
     qemu_thread_set_name("main");
 }
 
-static bool load_set_thread_description(void)
+static bool load_thread_description(void)
 {
     static gsize _init_once = 0;
 
@@ -38,14 +41,17 @@ static bool load_set_thread_description(void)
             SetThreadDescriptionFunc =
                 (pSetThreadDescription)GetProcAddress(kernel32_module,
                                                       "SetThreadDescription");
-            if (!SetThreadDescriptionFunc) {
+            GetThreadDescriptionFunc =
+                (pGetThreadDescription)GetProcAddress(kernel32_module,
+                                                      "GetThreadDescription");
+            if (!SetThreadDescriptionFunc || !GetThreadDescriptionFunc) {
                 FreeLibrary(kernel32_module);
             }
         }
         g_once_init_leave(&_init_once, 1);
     }
 
-    return !!SetThreadDescriptionFunc;
+    return (SetThreadDescriptionFunc && GetThreadDescriptionFunc);
 }
 
 static void error_exit(int err, const char *msg)
@@ -331,7 +337,7 @@ void qemu_thread_set_name(const char *name)
 {
     g_autofree wchar_t *namew = NULL;
 
-    if (!load_set_thread_description()) {
+    if (!load_thread_description()) {
         return;
     }
 
@@ -415,3 +421,38 @@ bool qemu_thread_is_self(QemuThread *thread)
 {
     return GetCurrentThreadId() == thread->tid;
 }
+
+static __thread char namebuf[64];
+
+const char *qemu_thread_get_name(void)
+{
+    HRESULT hr;
+    wchar_t *namew = NULL;
+    g_autofree char *name = NULL;
+
+    if (namebuf[0] != '\0') {
+        return namebuf;
+    }
+
+    if (!load_thread_description()) {
+        goto error;
+    }
+
+    hr = GetThreadDescriptionFunc(GetCurrentThread(), &namew);
+    if (!SUCCEEDED(hr)) {
+        goto error;
+    }
+
+    name = g_utf16_to_utf8(namew, -1, NULL, NULL, NULL);
+    LocalFree(namew);
+    if (!name) {
+        goto error;
+    }
+
+    g_strlcpy(namebuf, name, G_N_ELEMENTS(namebuf));
+    return namebuf;
+
+ error:
+    g_strlcpy(namebuf, "unnamed", G_N_ELEMENTS(namebuf));
+    return namebuf;
+}
-- 
2.53.0

[PATCH v6 11/27] util: introduce some API docs for logging APIs

[Daniel P. Berrangé]

There is a gotcha with qemu_log() usage in a threaded process.
If fragments of a log message are output via qemu_log() it is
possible for messages from two threads to get mixed up. To
prevent this qemu_log_trylock() should be used, along with
fprintf(f) calls.

This is a subtle problem that needs to be explained in the
API docs to ensure correct usage.

In the Rust code, the log_mask_ln method which is conceptually
equivalent to the C qemu_log() call will unconditionally append
a newline so must only ever be used for complete log messages.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/log-for-trace.h | 17 ++++++++++++++++-
 include/qemu/log.h           | 32 ++++++++++++++++++++++++++++++++
 rust/util/src/log.rs         |  6 ++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/include/qemu/log-for-trace.h b/include/qemu/log-for-trace.h
index f3a8791f1d..6861a1a4b7 100644
--- a/include/qemu/log-for-trace.h
+++ b/include/qemu/log-for-trace.h
@@ -29,7 +29,22 @@ static inline bool qemu_loglevel_mask(int mask)
     return (qemu_loglevel & mask) != 0;
 }
 
-/* main logging function */
+/**
+ * qemu_log: report a log message
+ * @fmt: the format string for the message
+ * @...: the format string arguments
+ *
+ * This will emit a log message to the current output stream.
+ *
+ * The @fmt string should normally represent a complete line
+ * of text, and thus end with a newline character.
+ *
+ * While it is possible to incrementally output fragments of
+ * a complete line using qemu_log, this is inefficient and
+ * races with other threads. For outputting fragments it is
+ * strongly preferred to use the qemu_log_trylock() method
+ * combined with fprintf().
+ */
 void G_GNUC_PRINTF(1, 2) qemu_log(const char *fmt, ...);
 
 #endif
diff --git a/include/qemu/log.h b/include/qemu/log.h
index 7effba4da4..b49a1a5fe3 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -41,7 +41,39 @@ bool qemu_log_separate(void);
 
 /* Lock/unlock output. */
 
+/**
+ * Acquires a lock on the current log output stream.
+ * The returned FILE object should be used with the
+ * fprintf() function to output the log message, and
+ * then qemu_log_unlock() called to release the lock.
+ *
+ * The primary use case is to be able to incrementally
+ * output fragments of a complete log message in an
+ * efficient and race free manner.
+ *
+ * The simpler qemu_log() method should normally only
+ * be used to output complete log messages, and not
+ * within scope of a qemu_log_trylock() call.
+ *
+ * A typical usage pattern would be
+ *
+ *    FILE *f = qemu_log_trylock()
+ *
+ *    fprintf(f, "Something ");
+ *    fprintf(f, "Something ");
+ *    fprintf(f, "Something ");
+ *    fprintf(f, "The end\n");
+ *
+ *    qemu_log_unlock(f);
+ *
+ * Returns: the current FILE if available, NULL on error
+ */
 FILE *qemu_log_trylock(void) G_GNUC_WARN_UNUSED_RESULT;
+
+/**
+ * Releases the lock on the log output, previously
+ * acquired by qemu_log_trylock().
+ */
 void qemu_log_unlock(FILE *fd);
 
 /* Logging functions: */
diff --git a/rust/util/src/log.rs b/rust/util/src/log.rs
index 0a4bc4249a..a85db804a7 100644
--- a/rust/util/src/log.rs
+++ b/rust/util/src/log.rs
@@ -134,6 +134,12 @@ fn drop(&mut self) {
 ///     "Address 0x{:x} out of range",
 ///     error_address,
 /// );
+///
+/// The `log_mask_ln` macro must only be used for emitting complete
+/// log messages. Where it is required to incrementally output string
+/// fragments to construct a complete message, `LogGuard::new()` must
+/// be directly used in combination with `writeln()` to avoid output
+/// races with other QEMU threads.
 /// ```
 #[macro_export]
 macro_rules! log_mask_ln {
-- 
2.53.0

[PATCH v6 12/27] util: avoid repeated prefix on incremental qemu_log calls

[Daniel P. Berrangé]

There are three general patterns to QEMU log output

 1. Single complete message calls

      qemu_log("Some message\n");

 2. Direct use of fprintf

      FILE *f = qemu_log_trylock()
      fprintf(f, "...");
      fprintf(f, "...");
      fprintf(f, "...\n");
      qemu_log_unlock(f)

 3. Mixed use of qemu_log_trylock/qemu_log()

      FILE *f = qemu_log_trylock()
      qemu_log("....");
      qemu_log("....");
      qemu_log("....\n");
      qemu_log_unlock(f)

When message prefixes are enabled, the timestamp will be
unconditionally emitted for all qemu_log() calls. This
works fine in the 1st case, and has no effect in the 2nd
case. In the 3rd case, however, we get the timestamp
printed over & over in each fragment.

One can suggest that pattern (3) is pointless as it is
functionally identical to (2) but with extra indirection
and overhead. None the less we have a fair bit of code
that does this.

The qemu_log() call itself is nothing more than a wrapper
which does pattern (2) with a single fprintf() call.

One might question whether (2) should include the message
prefix in the same way that (1), but there are scenarios
where this could be inappropriate / unhelpful such as the
CPU register dumps or linux-user strace output.

This patch fixes the problem in pattern (3) by keeping
track of the call depth of qemu_log_trylock() and then
only emitting the the prefix when the starting depth
was zero. In doing this qemu_log_trylock_context() is
also introduced as a variant of qemu_log_trylock()
that emits the prefix. Callers doing to batch output
can thus choose whether a prefix is appropriate or
not.

Fixes: 012842c07552 (log: make '-msg timestamp=on' apply to all qemu_log usage)
Reported-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/log.h |  7 +++++++
 util/log.c         | 49 ++++++++++++++++++++++++++--------------------
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index b49a1a5fe3..92956e5d0f 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -70,6 +70,13 @@ bool qemu_log_separate(void);
  */
 FILE *qemu_log_trylock(void) G_GNUC_WARN_UNUSED_RESULT;
 
+/**
+ * As qemu_log_trylock(), but will also print the message
+ * context, if any is configured and this caused the
+ * acquisition of the FILE lock
+ */
+FILE *qemu_log_trylock_with_context(void) G_GNUC_WARN_UNUSED_RESULT;
+
 /**
  * Releases the lock on the log output, previously
  * acquired by qemu_log_trylock().
diff --git a/util/log.c b/util/log.c
index c44d66b5ce..1644e6814b 100644
--- a/util/log.c
+++ b/util/log.c
@@ -127,13 +127,39 @@ static FILE *qemu_log_trylock_with_err(Error **errp)
     return logfile;
 }
 
+/*
+ * Zero if there's been no opening qemu_log_trylock call,
+ * indicating the need for message context to be emitted
+ *
+ * Non-zero if we're in the middle of printing a message,
+ * possibly over multiple lines and must skip further
+ * message context
+ */
+static __thread uint log_depth;
+
 FILE *qemu_log_trylock(void)
 {
-    return qemu_log_trylock_with_err(NULL);
+    FILE *f = qemu_log_trylock_with_err(NULL);
+    log_depth++;
+    return f;
+}
+
+FILE *qemu_log_trylock_with_context(void)
+{
+    FILE *f = qemu_log_trylock();
+    if (f && log_depth == 1 && message_with_timestamp) {
+        g_autofree const char *timestr = NULL;
+        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
+        timestr = g_date_time_format_iso8601(dt);
+        fprintf(f, "%s ", timestr);
+    }
+    return f;
 }
 
 void qemu_log_unlock(FILE *logfile)
 {
+    assert(log_depth);
+    log_depth--;
     if (logfile) {
         fflush(logfile);
         qemu_funlockfile(logfile);
@@ -145,28 +171,9 @@ void qemu_log_unlock(FILE *logfile)
 
 void qemu_log(const char *fmt, ...)
 {
-    FILE *f;
-    g_autofree const char *timestr = NULL;
-
-    /*
-     * Prepare the timestamp *outside* the logging
-     * lock so it better reflects when the message
-     * was emitted if we are delayed acquiring the
-     * mutex
-     */
-    if (message_with_timestamp) {
-        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
-        timestr = g_date_time_format_iso8601(dt);
-    }
-
-    f = qemu_log_trylock();
+    FILE *f = qemu_log_trylock_with_context();
     if (f) {
         va_list ap;
-
-        if (timestr) {
-            fprintf(f, "%s ", timestr);
-        }
-
         va_start(ap, fmt);
         vfprintf(f, fmt, ap);
         va_end(ap);
-- 
2.53.0

[PATCH v6 13/27] util/log: add missing error reporting in qemu_log_trylock_with_err

[Daniel P. Berrangé]

One codepath that could return NULL failed to populate the errp
object.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/log.c b/util/log.c
index 1644e6814b..1c9c7adb2d 100644
--- a/util/log.c
+++ b/util/log.c
@@ -118,6 +118,7 @@ static FILE *qemu_log_trylock_with_err(Error **errp)
             logfile = qatomic_rcu_read((void **)&global_file);
             if (!logfile) {
                 rcu_read_unlock();
+                error_setg(errp, "Global log file output is not open");
                 return NULL;
             }
         }
-- 
2.53.0

[PATCH v6 14/27] ui: add proper error reporting for password changes

[Daniel P. Berrangé]

Neither the VNC or SPICE code for password changes provides error
reporting at source, leading the callers to report a largely useless
generic error message.

Fixing this removes one of the two remaining needs for the undesirable
error_printf_unless_qmp() method.

While fixing this the error message hint is improved to recommend the
'password-secret' option which allows securely passing a password at
startup.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/ui/console.h                 |  2 +-
 include/ui/qemu-spice-module.h       |  3 ++-
 tests/functional/generic/test_vnc.py |  4 ++--
 ui/spice-core.c                      | 25 ++++++++++++++++++-------
 ui/spice-module.c                    |  7 ++++---
 ui/ui-qmp-cmds.c                     | 19 ++++++-------------
 ui/vnc-stubs.c                       |  6 +++---
 ui/vnc.c                             | 10 +++++++---
 8 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 98feaa58bd..3677a9d334 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -457,7 +457,7 @@ void qemu_display_help(void);
 void vnc_display_init(const char *id, Error **errp);
 void vnc_display_open(const char *id, Error **errp);
 void vnc_display_add_client(const char *id, int csock, bool skipauth);
-int vnc_display_password(const char *id, const char *password);
+int vnc_display_password(const char *id, const char *password, Error **errp);
 int vnc_display_pw_expire(const char *id, time_t expires);
 void vnc_parse(const char *str);
 int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
diff --git a/include/ui/qemu-spice-module.h b/include/ui/qemu-spice-module.h
index 1f22d557ea..072efa0c83 100644
--- a/include/ui/qemu-spice-module.h
+++ b/include/ui/qemu-spice-module.h
@@ -29,7 +29,8 @@ struct QemuSpiceOps {
     void (*display_init)(void);
     int (*migrate_info)(const char *h, int p, int t, const char *s);
     int (*set_passwd)(const char *passwd,
-                      bool fail_if_connected, bool disconnect_if_connected);
+                      bool fail_if_connected, bool disconnect_if_connected,
+                      Error **errp);
     int (*set_pw_expire)(time_t expires);
     int (*display_add_client)(int csock, int skipauth, int tls);
 #ifdef CONFIG_SPICE
diff --git a/tests/functional/generic/test_vnc.py b/tests/functional/generic/test_vnc.py
index f1dd1597cf..097f858ca1 100755
--- a/tests/functional/generic/test_vnc.py
+++ b/tests/functional/generic/test_vnc.py
@@ -48,7 +48,7 @@ def test_no_vnc_change_password(self):
         self.assertEqual(set_password_response['error']['class'],
                          'GenericError')
         self.assertEqual(set_password_response['error']['desc'],
-                         'Could not set password')
+                         'No VNC display is present');
 
     def launch_guarded(self):
         try:
@@ -73,7 +73,7 @@ def test_change_password_requires_a_password(self):
         self.assertEqual(set_password_response['error']['class'],
                          'GenericError')
         self.assertEqual(set_password_response['error']['desc'],
-                         'Could not set password')
+                         'VNC password authentication is disabled')
 
     def test_change_password(self):
         self.set_machine('none')
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 8a6050f4ae..cdcec34f67 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -756,7 +756,7 @@ static void qemu_spice_init(void)
                              tls_ciphers);
     }
     if (password) {
-        qemu_spice.set_passwd(password, false, false);
+        qemu_spice.set_passwd(password, false, false, NULL);
     }
     if (qemu_opt_get_bool(opts, "sasl", 0)) {
         if (spice_server_set_sasl(spice_server, 1) == -1) {
@@ -919,8 +919,10 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
     return qemu_spice_add_interface(&qxlin->base);
 }
 
-static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
+static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn,
+                                 Error **errp)
 {
+    int ret;
     time_t lifetime, now = time(NULL);
     char *passwd;
 
@@ -934,26 +936,35 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
         passwd = NULL;
         lifetime = 1;
     }
-    return spice_server_set_ticket(spice_server, passwd, lifetime,
-                                   fail_if_conn, disconnect_if_conn);
+    ret = spice_server_set_ticket(spice_server, passwd, lifetime,
+                                  fail_if_conn, disconnect_if_conn);
+    if (ret < 0) {
+        error_setg(errp, "Unable to set SPICE server ticket");
+        return -1;
+    }
+    return 0;
 }
 
 static int qemu_spice_set_passwd(const char *passwd,
-                                 bool fail_if_conn, bool disconnect_if_conn)
+                                 bool fail_if_conn, bool disconnect_if_conn,
+                                 Error **errp)
 {
     if (strcmp(auth, "spice") != 0) {
+        error_setg(errp, "SPICE authentication is disabled");
+        error_append_hint(errp,
+                          "To enable it use '-spice ...,password-secret=ID'");
         return -1;
     }
 
     g_free(auth_passwd);
     auth_passwd = g_strdup(passwd);
-    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
+    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn, errp);
 }
 
 static int qemu_spice_set_pw_expire(time_t expires)
 {
     auth_expires = expires;
-    return qemu_spice_set_ticket(false, false);
+    return qemu_spice_set_ticket(false, false, NULL);
 }
 
 static int qemu_spice_display_add_client(int csock, int skipauth, int tls)
diff --git a/ui/spice-module.c b/ui/spice-module.c
index 3222335872..7651c85885 100644
--- a/ui/spice-module.c
+++ b/ui/spice-module.c
@@ -45,14 +45,15 @@ static int qemu_spice_migrate_info_stub(const char *h, int p, int t,
 
 static int qemu_spice_set_passwd_stub(const char *passwd,
                                       bool fail_if_connected,
-                                      bool disconnect_if_connected)
+                                      bool disconnect_if_connected,
+                                      Error **errp)
 {
-    return -1;
+    g_assert_not_reached();
 }
 
 static int qemu_spice_set_pw_expire_stub(time_t expires)
 {
-    return -1;
+    g_assert_not_reached();
 }
 
 static int qemu_spice_display_add_client_stub(int csock, int skipauth,
diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
index b49b636152..1173c82cf7 100644
--- a/ui/ui-qmp-cmds.c
+++ b/ui/ui-qmp-cmds.c
@@ -31,15 +31,14 @@
 
 void qmp_set_password(SetPasswordOptions *opts, Error **errp)
 {
-    int rc;
-
     if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
         if (!qemu_using_spice(errp)) {
             return;
         }
-        rc = qemu_spice.set_passwd(opts->password,
-                opts->connected == SET_PASSWORD_ACTION_FAIL,
-                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
+        qemu_spice.set_passwd(opts->password,
+                              opts->connected == SET_PASSWORD_ACTION_FAIL,
+                              opts->connected == SET_PASSWORD_ACTION_DISCONNECT,
+                              errp);
     } else {
         assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
         if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
@@ -52,11 +51,7 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
          * Note that setting an empty password will not disable login
          * through this interface.
          */
-        rc = vnc_display_password(opts->u.vnc.display, opts->password);
-    }
-
-    if (rc != 0) {
-        error_setg(errp, "Could not set password");
+        vnc_display_password(opts->u.vnc.display, opts->password, errp);
     }
 }
 
@@ -107,9 +102,7 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
 #ifdef CONFIG_VNC
 void qmp_change_vnc_password(const char *password, Error **errp)
 {
-    if (vnc_display_password(NULL, password) < 0) {
-        error_setg(errp, "Could not set password");
-    }
+    vnc_display_password(NULL, password, errp);
 }
 #endif
 
diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
index a96bc86236..5de9bf9d70 100644
--- a/ui/vnc-stubs.c
+++ b/ui/vnc-stubs.c
@@ -2,11 +2,11 @@
 #include "ui/console.h"
 #include "qapi/error.h"
 
-int vnc_display_password(const char *id, const char *password)
+int vnc_display_password(const char *id, const char *password, Error **errp)
 {
-    return -ENODEV;
+    g_assert_not_reached();
 }
 int vnc_display_pw_expire(const char *id, time_t expires)
 {
-    return -ENODEV;
+    g_assert_not_reached();
 };
diff --git a/ui/vnc.c b/ui/vnc.c
index a61a4f937d..833e0e2e68 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3526,16 +3526,20 @@ static void vnc_display_close(VncDisplay *vd)
 #endif
 }
 
-int vnc_display_password(const char *id, const char *password)
+int vnc_display_password(const char *id, const char *password, Error **errp)
 {
     VncDisplay *vd = vnc_display_find(id);
 
     if (!vd) {
+        error_setg(errp, "No VNC display is present");
+        error_append_hint(errp,
+                          "To enable it, use '-vnc ...'");
         return -EINVAL;
     }
     if (vd->auth == VNC_AUTH_NONE) {
-        error_printf_unless_qmp("If you want use passwords please enable "
-                                "password auth using '-vnc ${dpy},password'.\n");
+        error_setg(errp, "VNC password authentication is disabled");
+        error_append_hint(errp,
+                          "To enable it, use '-vnc ...,password-secret=ID'");
         return -EINVAL;
     }
 
-- 
2.53.0

[PATCH v6 15/27] ui: remove redundant use of error_printf_unless_qmp()

[Daniel P. Berrangé]

The vnc_display_print_local_addr() method is intended to print the VNC
listening address on the console at startup, so the user can see the
auto-chosen port address when using the 'to=' flag. This is only called
by vnc_display_open() which is in the QEMU startup callpath. The check
for not being in QMP is thus redundant and can be removed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 833e0e2e68..fe020dcc4c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3578,9 +3578,9 @@ static void vnc_display_print_local_addr(VncDisplay *vd)
         qapi_free_SocketAddress(addr);
         return;
     }
-    error_printf_unless_qmp("VNC server running on %s:%s\n",
-                            addr->u.inet.host,
-                            addr->u.inet.port);
+    error_printf("VNC server running on %s:%s\n",
+                 addr->u.inet.host,
+                 addr->u.inet.port);
     qapi_free_SocketAddress(addr);
 }
 
-- 
2.53.0

[PATCH v6 16/27] monitor: remove redundant error_[v]printf_unless_qmp

[Daniel P. Berrangé]

The only callers of these functions have been removed. Adding any
new usage of them is highly undesirable, so they should be entirely
removed.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/monitor/monitor.h |  3 ---
 monitor/monitor.c         | 24 ------------------------
 stubs/error-printf.c      |  5 -----
 3 files changed, 32 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c3740ec616..296690e1f1 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -59,7 +59,4 @@ void monitor_register_hmp(const char *name, bool info,
 void monitor_register_hmp_info_hrt(const char *name,
                                    HumanReadableText *(*handler)(Error **errp));
 
-int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
-int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
-
 #endif /* MONITOR_H */
diff --git a/monitor/monitor.c b/monitor/monitor.c
index dd2c45ded0..4d26cd9496 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -281,30 +281,6 @@ int error_vprintf(const char *fmt, va_list ap)
     return vfprintf(stderr, fmt, ap);
 }
 
-int error_vprintf_unless_qmp(const char *fmt, va_list ap)
-{
-    Monitor *cur_mon = monitor_cur();
-
-    if (!cur_mon) {
-        return vfprintf(stderr, fmt, ap);
-    }
-    if (!monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
-    }
-    return -1;
-}
-
-int error_printf_unless_qmp(const char *fmt, ...)
-{
-    va_list ap;
-    int ret;
-
-    va_start(ap, fmt);
-    ret = error_vprintf_unless_qmp(fmt, ap);
-    va_end(ap);
-    return ret;
-}
-
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index 0e326d8010..1afa0f62ca 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
     }
     return vfprintf(stderr, fmt, ap);
 }
-
-int error_vprintf_unless_qmp(const char *fmt, va_list ap)
-{
-    return error_vprintf(fmt, ap);
-}
-- 
2.53.0

[PATCH v6 17/27] monitor: refactor error_vprintf()

[Daniel P. Berrangé]

The monitor_vprintf() code will return -1 if either the monitor
is NULL, or the monitor is QMP. The error_vprintf() code can
take advantage of this to avoid having to duplicate the same
checks, and instead simply look at the return value.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/monitor.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 4d26cd9496..64c552a047 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -269,16 +269,21 @@ void monitor_printc(Monitor *mon, int c)
 }
 
 /*
- * Print to current monitor if we have one, else to stderr.
+ * Print to the current human monitor if we have one, else to stderr.
  */
 int error_vprintf(const char *fmt, va_list ap)
 {
     Monitor *cur_mon = monitor_cur();
-
-    if (cur_mon && !monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
+    /*
+     * This will return -1 if 'cur_mon' is NULL, or is QMP.
+     * IOW this will only print if in HMP, otherwise we
+     * fallback to stderr for QMP / no-monitor scenarios.
+     */
+    int ret = monitor_vprintf(cur_mon, fmt, ap);
+    if (ret == -1) {
+        ret = vfprintf(stderr, fmt, ap);
     }
-    return vfprintf(stderr, fmt, ap);
+    return ret;
 }
 
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
-- 
2.53.0

[PATCH v6 18/27] monitor: move error_vprintf back to error-report.c

[Daniel P. Berrangé]

The current unit tests rely on monitor.o not being linked, such
that the monitor stubs get linked instead. Since error_vprintf
is in monitor.o this allows a stub error_vprintf impl to be used
that calls g_test_message.

This takes a different approach, with error_vprintf moving
back to error-report.c such that it is always linked into the
tests. The monitor_vprintf() stub is then changed to use
g_test_message if QTEST_SILENT_ERRORS is set, otherwise it will
return -1 and trigger error_vprintf to call vfprintf.

The end result is functionally equivalent for the purposes of
the unit tests.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/monitor.c    | 18 ------------------
 stubs/error-printf.c | 18 ------------------
 stubs/meson.build    |  1 -
 stubs/monitor-core.c | 14 +++++++++++++-
 util/error-report.c  | 18 ++++++++++++++++++
 5 files changed, 31 insertions(+), 38 deletions(-)
 delete mode 100644 stubs/error-printf.c

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 64c552a047..6dc5a7016d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -268,24 +268,6 @@ void monitor_printc(Monitor *mon, int c)
     monitor_printf(mon, "'");
 }
 
-/*
- * Print to the current human monitor if we have one, else to stderr.
- */
-int error_vprintf(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
-     * fallback to stderr for QMP / no-monitor scenarios.
-     */
-    int ret = monitor_vprintf(cur_mon, fmt, ap);
-    if (ret == -1) {
-        ret = vfprintf(stderr, fmt, ap);
-    }
-    return ret;
-}
-
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
deleted file mode 100644
index 1afa0f62ca..0000000000
--- a/stubs/error-printf.c
+++ /dev/null
@@ -1,18 +0,0 @@
-#include "qemu/osdep.h"
-#include "qemu/error-report.h"
-#include "monitor/monitor.h"
-
-int error_vprintf(const char *fmt, va_list ap)
-{
-    int ret;
-
-    if (g_test_initialized() && !g_test_subprocess() &&
-        getenv("QTEST_SILENT_ERRORS")) {
-        char *msg = g_strdup_vprintf(fmt, ap);
-        g_test_message("%s", msg);
-        ret = strlen(msg);
-        g_free(msg);
-        return ret;
-    }
-    return vfprintf(stderr, fmt, ap);
-}
diff --git a/stubs/meson.build b/stubs/meson.build
index 8a07059500..354bf97d74 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -3,7 +3,6 @@
 # below, so that it is clear who needs the stubbed functionality.
 
 stub_ss.add(files('cpu-get-clock.c'))
-stub_ss.add(files('error-printf.c'))
 stub_ss.add(files('fdset.c'))
 stub_ss.add(files('iothread-lock.c'))
 stub_ss.add(files('is-daemonized.c'))
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 1894cdfe1f..a7c32297c9 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -18,5 +18,17 @@ void qapi_event_emit(QAPIEvent event, QDict *qdict)
 
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
-    abort();
+    /*
+     * Pretend 'g_test_message' is our monitor console to
+     * stop the caller sending messages to stderr
+     */
+    if (g_test_initialized() && !g_test_subprocess() &&
+        getenv("QTEST_SILENT_ERRORS")) {
+        char *msg = g_strdup_vprintf(fmt, ap);
+        g_test_message("%s", msg);
+        size_t ret = strlen(msg);
+        g_free(msg);
+        return ret;
+    }
+    return -1;
 }
diff --git a/util/error-report.c b/util/error-report.c
index 1b17c11de1..f3aa24ec22 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -29,6 +29,24 @@ bool message_with_timestamp;
 bool error_with_guestname;
 const char *error_guest_name;
 
+/*
+ * Print to the current human monitor if we have one, else to stderr.
+ */
+int error_vprintf(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
+     * fallback to stderr for QMP / no-monitor scenarios.
+     */
+    int ret = monitor_vprintf(cur_mon, fmt, ap);
+    if (ret == -1) {
+        ret = vfprintf(stderr, fmt, ap);
+    }
+    return ret;
+}
+
 int error_printf(const char *fmt, ...)
 {
     va_list ap;
-- 
2.53.0

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

[Daniel P. Berrangé]

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.
  */
-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.
+ */
+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;
+}
+
+int error_vprintf(const char *fmt, va_list ap)
+{
+    return error_vprintf_mon(monitor_cur(), fmt, ap);
+}
+
 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;
 }
@@ -156,34 +176,34 @@ void loc_set_file(const char *fname, int lno)
 /*
  * Print current location to current monitor if we have one, else to stderr.
  */
-static void print_loc(void)
+static void print_loc(Monitor *cur)
 {
     const char *sep = "";
     int i;
     const char *const *argp;
 
-    if (!monitor_cur() && g_get_prgname()) {
-        error_printf("%s:", g_get_prgname());
+    if (!cur && g_get_prgname()) {
+        fprintf(stderr, "%s:", g_get_prgname());
         sep = " ";
     }
     switch (cur_loc->kind) {
     case LOC_CMDLINE:
         argp = cur_loc->ptr;
         for (i = 0; i < cur_loc->num; i++) {
-            error_printf("%s%s", sep, argp[i]);
+            error_printf_mon(cur, "%s%s", sep, argp[i]);
             sep = " ";
         }
-        error_printf(": ");
+        error_printf_mon(cur, ": ");
         break;
     case LOC_FILE:
-        error_printf("%s:", (const char *)cur_loc->ptr);
+        error_printf_mon(cur, "%s:", (const char *)cur_loc->ptr);
         if (cur_loc->num) {
-            error_printf("%d:", cur_loc->num);
+            error_printf_mon(cur, "%d:", cur_loc->num);
         }
-        error_printf(" ");
+        error_printf_mon(cur, " ");
         break;
     default:
-        error_printf("%s", sep);
+        error_printf_mon(cur, "%s", sep);
     }
 }
 
@@ -204,34 +224,35 @@ real_time_iso8601(void)
 G_GNUC_PRINTF(2, 0)
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
+    Monitor *cur = monitor_cur();
     gchar *timestr;
 
-    if (message_with_timestamp && !monitor_cur()) {
+    if (message_with_timestamp && !cur) {
         timestr = real_time_iso8601();
-        error_printf("%s ", timestr);
+        fprintf(stderr, "%s ", timestr);
         g_free(timestr);
     }
 
     /* Only prepend guest name if -msg guest-name and -name guest=... are set */
-    if (error_with_guestname && error_guest_name && !monitor_cur()) {
-        error_printf("%s ", error_guest_name);
+    if (error_with_guestname && error_guest_name && !cur) {
+        fprintf(stderr, "%s ", error_guest_name);
     }
 
-    print_loc();
+    print_loc(cur);
 
     switch (type) {
     case REPORT_TYPE_ERROR:
         break;
     case REPORT_TYPE_WARNING:
-        error_printf("warning: ");
+        error_printf_mon(cur, "warning: ");
         break;
     case REPORT_TYPE_INFO:
-        error_printf("info: ");
+        error_printf_mon(cur, "info: ");
         break;
     }
 
-    error_vprintf(fmt, ap);
-    error_printf("\n");
+    error_vprintf_mon(cur, fmt, ap);
+    error_printf_mon(cur, "\n");
 }
 
 /*
-- 
2.53.0

[PATCH v6 20/27] util: don't skip error prefixes when QMP is active

[Daniel P. Berrangé]

The vreport() function will print to HMP if available, otherwise
to stderr. In the event that vreport() is called during execution
of a QMP command, it will print to stderr, but mistakenly omit the
message prefixes (timestamp, guest name, program name).

This new usage of monitor_is_cur_qmp() from vreport() requires that
we add a stub to satisfy linking of non-system emulator binaries.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 stubs/monitor-core.c           | 5 +++++
 tests/unit/test-util-sockets.c | 1 +
 util/error-report.c            | 8 ++++++++
 3 files changed, 14 insertions(+)

diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index a7c32297c9..078a5012e9 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
     return NULL;
 }
 
+bool monitor_cur_is_qmp(void)
+{
+    return false;
+}
+
 Monitor *monitor_set_cur(Coroutine *co, Monitor *mon)
 {
     return NULL;
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index ee66d727c3..b9f2453e29 100644
--- a/tests/unit/test-util-sockets.c
+++ b/tests/unit/test-util-sockets.c
@@ -74,6 +74,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 Monitor *monitor_cur(void) { return cur_mon; }
 Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
+bool monitor_cur_is_qmp(void) { abort(); };
 
 #ifndef _WIN32
 static void test_socket_fd_pass_name_good(void)
diff --git a/util/error-report.c b/util/error-report.c
index b34a2bc7bc..052f0f68b8 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -227,6 +227,14 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     Monitor *cur = monitor_cur();
     gchar *timestr;
 
+    /*
+     * When current monitor is QMP, messages must go to stderr
+     * and have prefixes added
+     */
+    if (monitor_cur_is_qmp()) {
+        cur = NULL;
+    }
+
     if (message_with_timestamp && !cur) {
         timestr = real_time_iso8601();
         fprintf(stderr, "%s ", timestr);
-- 
2.53.0

[PATCH v6 21/27] util: fix interleaving of error prefixes

[Daniel P. Berrangé]

The vreport() function will optionally emit an prefix for error
messages which is output to stderr incrementally. In the event
that two vreport() calls execute concurrently, there is a risk
that the prefix output will interleave. To address this it is
required to take a lock on 'stderr' when outputting errors.

Reported-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/error-report.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/error-report.c b/util/error-report.c
index 052f0f68b8..516d323ffe 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -234,6 +234,9 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     if (monitor_cur_is_qmp()) {
         cur = NULL;
     }
+    if (!cur) {
+        qemu_flockfile(stderr);
+    }
 
     if (message_with_timestamp && !cur) {
         timestr = real_time_iso8601();
@@ -261,6 +264,10 @@ static void vreport(report_type type, const char *fmt, va_list ap)
 
     error_vprintf_mon(cur, fmt, ap);
     error_printf_mon(cur, "\n");
+
+    if (!cur) {
+        qemu_funlockfile(stderr);
+    }
 }
 
 /*
-- 
2.53.0

[PATCH v6 22/27] util: introduce common helper for error-report & log code

[Daniel P. Berrangé]

The error-report and log code both have a need to add prefixes
to messages they are printing, with the current example being
a timestamp.

The format and configuration they use should be consistent, so
providing a common helper will ensure this is always the case.
Initially the helper only emits a timestamp, but future patches
will expand this.

This takes the liberty of assigning the new file to the same
maintainer as the existing error-report.c file, given it will
be extracting some functionality from the latter.

While vreport() dynamically changes between reporting to the
monitor vs stderr, depending on whether HMP is active or not,
message prefixes are only ever used in the non-HMP case. Thus
the helper API can take a FILE * object and not have to deal
with the monitor at all.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 MAINTAINERS            |  2 ++
 include/qemu/message.h | 28 ++++++++++++++++++++++++++++
 util/meson.build       |  1 +
 util/message.c         | 23 +++++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 include/qemu/message.h
 create mode 100644 util/message.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 29f88d48f3..96a4ce438e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3261,9 +3261,11 @@ M: Markus Armbruster <armbru@redhat.com>
 S: Supported
 F: include/qapi/error.h
 F: include/qemu/error-report.h
+F: include/qemu/message.h
 F: qapi/error.json
 F: util/error.c
 F: util/error-report.c
+F: util/message.c
 F: scripts/coccinelle/err-bad-newline.cocci
 F: scripts/coccinelle/error-use-after-free.cocci
 F: scripts/coccinelle/error_propagate_null.cocci
diff --git a/include/qemu/message.h b/include/qemu/message.h
new file mode 100644
index 0000000000..0a06421f77
--- /dev/null
+++ b/include/qemu/message.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef QEMU_MESSAGE_H
+#define QEMU_MESSAGE_H
+
+enum QMessageFormatFlags {
+    QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
+};
+
+/**
+ * qmessage_set_format:
+ * @flags: the message information to emit
+ *
+ * Select which pieces of information to
+ * emit for messages
+ */
+void qmessage_set_format(int flags);
+
+/**
+ * qmessage_context_print:
+ * @fp: file to emit the prefix on
+ *
+ * Emit a message prefix with the information selected by
+ * an earlier call to qmessage_set_format.
+ */
+void qmessage_context_print(FILE *fp);
+
+#endif /* QEMU_MESSAGE_H */
diff --git a/util/meson.build b/util/meson.build
index 7c9445615d..3035bd4bee 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -38,6 +38,7 @@ util_ss.add(files('host-utils.c'))
 util_ss.add(files('bitmap.c', 'bitops.c'))
 util_ss.add(files('fifo8.c'))
 util_ss.add(files('cacheflush.c'))
+util_ss.add(files('message.c'))
 util_ss.add(files('error.c', 'error-report.c'))
 util_ss.add(files('qemu-print.c'))
 util_ss.add(files('id.c'))
diff --git a/util/message.c b/util/message.c
new file mode 100644
index 0000000000..99a403f9d0
--- /dev/null
+++ b/util/message.c
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+
+#include "qemu/message.h"
+#include "monitor/monitor.h"
+
+static int message_format;
+
+void qmessage_set_format(int flags)
+{
+    message_format = flags;
+}
+
+void qmessage_context_print(FILE *fp)
+{
+    if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
+        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
+        g_autofree char *timestr = g_date_time_format_iso8601(dt);
+        fputs(timestr, fp);
+        fputc(' ', fp);
+    }
+}
-- 
2.53.0

[PATCH v6 23/27] util: convert error-report & log to message API for timestamp

[Daniel P. Berrangé]

Both the error-report and log APIs can optional emit a timestamp
as a prefix on messages, with the '-msg timestamp=on' command
line flag is set.

Convert them to the new message API for formatting the context
prefix, guaranteeing they will have matching behaviour going
forward.

There is no change in output format for either logs or errors
with this conversion. There is a slight impact on logging code
in that recording the timestamp is now potentially delayed by
the need to acquire the logging mutex.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/error-report.h    |  1 -
 system/vl.c                    |  7 ++++++-
 tests/unit/test-error-report.c |  3 ++-
 util/error-report.c            | 18 ++----------------
 util/log.c                     |  8 +++-----
 5 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3ae2357fda..c8000778ec 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -70,7 +70,6 @@ void error_init(const char *argv0);
                               fmt, ##__VA_ARGS__);      \
     })
 
-extern bool message_with_timestamp;
 extern bool error_with_guestname;
 extern const char *error_guest_name;
 
diff --git a/system/vl.c b/system/vl.c
index d8676b3adf..4ef41df747 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -26,6 +26,7 @@
 #include "qemu/help-texts.h"
 #include "qemu/datadir.h"
 #include "qemu/units.h"
+#include "qemu/message.h"
 #include "qemu/module.h"
 #include "qemu/target-info.h"
 #include "exec/cpu-common.h"
@@ -818,8 +819,12 @@ static void realtime_init(void)
 
 static void configure_msg(QemuOpts *opts)
 {
-    message_with_timestamp = qemu_opt_get_bool(opts, "timestamp", false);
+    int flags = 0;
+    if (qemu_opt_get_bool(opts, "timestamp", false)) {
+        flags |= QMESSAGE_FORMAT_TIMESTAMP;
+    }
     error_with_guestname = qemu_opt_get_bool(opts, "guest-name", false);
+    qmessage_set_format(flags);
 }
 
 
diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c
index a8532fc58f..9c63019694 100644
--- a/tests/unit/test-error-report.c
+++ b/tests/unit/test-error-report.c
@@ -12,6 +12,7 @@
 
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qemu/message.h"
 
 static void
 test_error_report_simple(void)
@@ -89,7 +90,7 @@ static void
 test_error_report_timestamp(void)
 {
     if (g_test_subprocess()) {
-        message_with_timestamp = true;
+        qmessage_set_format(QMESSAGE_FORMAT_TIMESTAMP);
         warn_report("warn");
         error_report("err");
         return;
diff --git a/util/error-report.c b/util/error-report.c
index 516d323ffe..1ef65a7475 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
+#include "qemu/message.h"
 
 /*
  * @report_type is the type of message: error, warning or
@@ -24,8 +25,6 @@ typedef enum {
     REPORT_TYPE_INFO,
 } report_type;
 
-/* Prepend timestamp to messages */
-bool message_with_timestamp;
 bool error_with_guestname;
 const char *error_guest_name;
 
@@ -207,13 +206,6 @@ static void print_loc(Monitor *cur)
     }
 }
 
-static char *
-real_time_iso8601(void)
-{
-    g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
-    return g_date_time_format_iso8601(dt);
-}
-
 /*
  * Print a message to current monitor if we have one, else to stderr.
  * @report_type is the type of message: error, warning or informational.
@@ -225,7 +217,6 @@ G_GNUC_PRINTF(2, 0)
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
     Monitor *cur = monitor_cur();
-    gchar *timestr;
 
     /*
      * When current monitor is QMP, messages must go to stderr
@@ -236,12 +227,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     }
     if (!cur) {
         qemu_flockfile(stderr);
-    }
-
-    if (message_with_timestamp && !cur) {
-        timestr = real_time_iso8601();
-        fprintf(stderr, "%s ", timestr);
-        g_free(timestr);
+        qmessage_context_print(stderr);
     }
 
     /* Only prepend guest name if -msg guest-name and -name guest=... are set */
diff --git a/util/log.c b/util/log.c
index 1c9c7adb2d..b5aae22c83 100644
--- a/util/log.c
+++ b/util/log.c
@@ -22,6 +22,7 @@
 #include "qemu/range.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qemu/message.h"
 #include "qemu/cutils.h"
 #include "trace/control.h"
 #include "qemu/thread.h"
@@ -148,11 +149,8 @@ FILE *qemu_log_trylock(void)
 FILE *qemu_log_trylock_with_context(void)
 {
     FILE *f = qemu_log_trylock();
-    if (f && log_depth == 1 && message_with_timestamp) {
-        g_autofree const char *timestr = NULL;
-        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
-        timestr = g_date_time_format_iso8601(dt);
-        fprintf(f, "%s ", timestr);
+    if (f && log_depth == 1) {
+        qmessage_context_print(f);
     }
     return f;
 }
-- 
2.53.0

[PATCH v6 24/27] util: add support for formatting a workload name in messages

[Daniel P. Berrangé]

The error_report function can include the guest name in any
messages it prints. The qemu_log function has no equivalent
behaviour.

This introduces support for a "workload name" in the new
messages API, which in the case of system emulators will
be the guest name. The possibility of defining a workload
name for other binaries is left as an exercise for the
future.

This change has no impact on the output of the error_report
function, but will change the qemu_log function. This can
be easily seen with the 'log' trace backend, and how it is
now more closely matching error_report output.

Before:

  # qemu-system-x86_64 -msg guest-name=on -name blah -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55b3af3fd870 dir=fish
  qcrypto_tls_creds_get_path TLS creds path creds=0x55b3af3fd870 filename=ca-cert.pem path=<none>
  blah qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

After:

  # qemu-system-x86_64 -msg guest-name=on -name blah -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  blah qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55b3af3fd870 dir=fish
  blah qcrypto_tls_creds_get_path TLS creds path creds=0x55b3af3fd870 filename=ca-cert.pem path=<none>
  blah 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>
---
 include/qemu/error-report.h |  3 ---
 include/qemu/message.h      | 10 ++++++++++
 system/vl.c                 |  6 ++++--
 util/error-report.c         |  7 -------
 util/message.c              | 12 ++++++++++++
 5 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c8000778ec..ffc305f828 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -70,7 +70,4 @@ void error_init(const char *argv0);
                               fmt, ##__VA_ARGS__);      \
     })
 
-extern bool error_with_guestname;
-extern const char *error_guest_name;
-
 #endif
diff --git a/include/qemu/message.h b/include/qemu/message.h
index 0a06421f77..cfc3c92648 100644
--- a/include/qemu/message.h
+++ b/include/qemu/message.h
@@ -5,6 +5,7 @@
 
 enum QMessageFormatFlags {
     QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
+    QMESSAGE_FORMAT_WORKLOAD_NAME = (1 << 1),
 };
 
 /**
@@ -16,6 +17,15 @@ enum QMessageFormatFlags {
  */
 void qmessage_set_format(int flags);
 
+/**
+ * qmessage_set_workload_name:
+ * @name: the name of the workload
+ *
+ * Set the workload name, which for a system emulator
+ * will be the guest VM name.
+ */
+void qmessage_set_workload_name(const char *name);
+
 /**
  * qmessage_context_print:
  * @fp: file to emit the prefix on
diff --git a/system/vl.c b/system/vl.c
index 4ef41df747..be5862e55e 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -823,7 +823,9 @@ static void configure_msg(QemuOpts *opts)
     if (qemu_opt_get_bool(opts, "timestamp", false)) {
         flags |= QMESSAGE_FORMAT_TIMESTAMP;
     }
-    error_with_guestname = qemu_opt_get_bool(opts, "guest-name", false);
+    if (qemu_opt_get_bool(opts, "guest-name", false)) {
+        flags |= QMESSAGE_FORMAT_WORKLOAD_NAME;
+    }
     qmessage_set_format(flags);
 }
 
@@ -3526,7 +3528,7 @@ void qemu_init(int argc, char **argv)
                     exit(1);
                 }
                 /* Capture guest name if -msg guest-name is used later */
-                error_guest_name = qemu_opt_get(opts, "guest");
+                qmessage_set_workload_name(qemu_opt_get(opts, "guest"));
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
diff --git a/util/error-report.c b/util/error-report.c
index 1ef65a7475..3009b93c23 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -25,8 +25,6 @@ typedef enum {
     REPORT_TYPE_INFO,
 } report_type;
 
-bool error_with_guestname;
-const char *error_guest_name;
 
 /*
  * Print to the current human monitor if we have one, else to stderr.
@@ -230,11 +228,6 @@ static void vreport(report_type type, const char *fmt, va_list ap)
         qmessage_context_print(stderr);
     }
 
-    /* Only prepend guest name if -msg guest-name and -name guest=... are set */
-    if (error_with_guestname && error_guest_name && !cur) {
-        fprintf(stderr, "%s ", error_guest_name);
-    }
-
     print_loc(cur);
 
     switch (type) {
diff --git a/util/message.c b/util/message.c
index 99a403f9d0..fb2c947153 100644
--- a/util/message.c
+++ b/util/message.c
@@ -6,12 +6,18 @@
 #include "monitor/monitor.h"
 
 static int message_format;
+static char *message_workloadname;
 
 void qmessage_set_format(int flags)
 {
     message_format = flags;
 }
 
+void qmessage_set_workload_name(const char *name)
+{
+    message_workloadname = g_strdup(name);
+}
+
 void qmessage_context_print(FILE *fp)
 {
     if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
@@ -20,4 +26,10 @@ void qmessage_context_print(FILE *fp)
         fputs(timestr, fp);
         fputc(' ', fp);
     }
+
+    if ((message_format & QMESSAGE_FORMAT_WORKLOAD_NAME) &&
+        message_workloadname) {
+        fputs(message_workloadname, fp);
+        fputc(' ', fp);
+    }
 }
-- 
2.53.0

[PATCH v6 25/27] util: add support for formatting a program name in messages

[Daniel P. Berrangé]

The error_report function can include the program name in any
messages it prints. The qemu_log function has no equivalent
behaviour.

This introduces support for a "program name" in the new
messages API, which will be included by default for all
binaries.

This change tweaks the output of the error_report function,
adding a space between the program name and the location
info. The qemu_log function will gain the program name. This
can be easily seen with the 'log' trace backend, and how it
is now more closely matching error_report output.

Before:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
  qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
  qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

After:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
  qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
  qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

When adding this the '-msg program-name=on|off' option is
introduced, so that the program name (which is enabled by
default) can be supressed if desired. This could be useful
if '-msg guest-name=on' is being used as a more informative
identifier.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/message.h         |  1 +
 qemu-options.hx                |  9 +++++++--
 system/vl.c                    | 11 ++++++++++-
 tests/unit/test-error-report.c |  5 +++--
 util/error-report.c            |  4 ----
 util/message.c                 | 10 +++++++++-
 6 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/qemu/message.h b/include/qemu/message.h
index cfc3c92648..b8518d06be 100644
--- a/include/qemu/message.h
+++ b/include/qemu/message.h
@@ -6,6 +6,7 @@
 enum QMessageFormatFlags {
     QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
     QMESSAGE_FORMAT_WORKLOAD_NAME = (1 << 1),
+    QMESSAGE_FORMAT_PROGRAM_NAME = (1 << 2),
 };
 
 /**
diff --git a/qemu-options.hx b/qemu-options.hx
index cdb7fa20c2..84dcbb0983 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5515,13 +5515,15 @@ ERST
 
 DEF("msg", HAS_ARG, QEMU_OPTION_msg,
     "-msg [timestamp=on|off][,guest-name=on|off]\n"
+    "     [,program-name=on|off]\n"
     "                control error message format\n"
     "                timestamp=on enables timestamps (default: off)\n"
     "                guest-name=on enables guest name prefix but only if\n"
-    "                              -name guest option is set (default: off)\n",
+    "                              -name guest option is set (default: off)\n"
+    "                program-name=off disables program name prefix (default: on)\n",
     QEMU_ARCH_ALL)
 SRST
-``-msg [timestamp=on|off][,guest-name=on|off]``
+``-msg [timestamp=on|off][,guest-name=on|off][,program-name=on|off]``
     Control error message format.
 
     ``timestamp=on|off``
@@ -5530,6 +5532,9 @@ SRST
     ``guest-name=on|off``
         Prefix messages with guest name but only if -name guest option is set
         otherwise the option is ignored. Default is off.
+
+    ``program-name=on|off``
+        Prefix messages with the program name. Default is on.
 ERST
 
 DEF("dump-vmstate", HAS_ARG, QEMU_OPTION_dump_vmstate,
diff --git a/system/vl.c b/system/vl.c
index be5862e55e..3e79bf47e0 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -381,6 +381,12 @@ static QemuOptsList qemu_msg_opts = {
             .help = "Prepends guest name for error messages but only if "
                     "-name guest is set otherwise option is ignored\n",
         },
+        {
+            .name = "program-name",
+            .type = QEMU_OPT_BOOL,
+            .help = "Prepends program name for error messages (enabled "
+                    "by default)\n",
+        },
         { /* end of list */ }
     },
 };
@@ -819,13 +825,16 @@ static void realtime_init(void)
 
 static void configure_msg(QemuOpts *opts)
 {
-    int flags = 0;
+    int flags = QMESSAGE_FORMAT_PROGRAM_NAME;
     if (qemu_opt_get_bool(opts, "timestamp", false)) {
         flags |= QMESSAGE_FORMAT_TIMESTAMP;
     }
     if (qemu_opt_get_bool(opts, "guest-name", false)) {
         flags |= QMESSAGE_FORMAT_WORKLOAD_NAME;
     }
+    if (!qemu_opt_get_bool(opts, "program-name", true)) {
+        flags &= ~QMESSAGE_FORMAT_PROGRAM_NAME;
+    }
     qmessage_set_format(flags);
 }
 
diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c
index 9c63019694..714eafc51e 100644
--- a/tests/unit/test-error-report.c
+++ b/tests/unit/test-error-report.c
@@ -47,7 +47,7 @@ test_error_report_loc(void)
     g_test_trap_subprocess(NULL, 0, 0);
     g_test_trap_assert_passed();
     g_test_trap_assert_stderr("\
-test-error-report:some-file.c:7717: test error1*\
+test-error-report: some-file.c:7717: test error1*\
 test-error-report: test error2*\
 ");
 }
@@ -90,7 +90,8 @@ static void
 test_error_report_timestamp(void)
 {
     if (g_test_subprocess()) {
-        qmessage_set_format(QMESSAGE_FORMAT_TIMESTAMP);
+        qmessage_set_format(QMESSAGE_FORMAT_TIMESTAMP |
+                            QMESSAGE_FORMAT_PROGRAM_NAME);
         warn_report("warn");
         error_report("err");
         return;
diff --git a/util/error-report.c b/util/error-report.c
index 3009b93c23..196d51618c 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -179,10 +179,6 @@ static void print_loc(Monitor *cur)
     int i;
     const char *const *argp;
 
-    if (!cur && g_get_prgname()) {
-        fprintf(stderr, "%s:", g_get_prgname());
-        sep = " ";
-    }
     switch (cur_loc->kind) {
     case LOC_CMDLINE:
         argp = cur_loc->ptr;
diff --git a/util/message.c b/util/message.c
index fb2c947153..dd01bf7462 100644
--- a/util/message.c
+++ b/util/message.c
@@ -5,7 +5,7 @@
 #include "qemu/message.h"
 #include "monitor/monitor.h"
 
-static int message_format;
+static int message_format = QMESSAGE_FORMAT_PROGRAM_NAME;
 static char *message_workloadname;
 
 void qmessage_set_format(int flags)
@@ -32,4 +32,12 @@ void qmessage_context_print(FILE *fp)
         fputs(message_workloadname, fp);
         fputc(' ', fp);
     }
+
+    if (message_format & QMESSAGE_FORMAT_PROGRAM_NAME) {
+        const char *pgnamestr = g_get_prgname();
+        if (pgnamestr) {
+            fputs(pgnamestr, fp);
+            fputs(": ", fp);
+        }
+    }
 }
-- 
2.53.0

[PATCH v6 26/27] util: add support for formatting thread info in messages

[Daniel P. Berrangé]

The message context is now extended to be able to include the
thread ID and thread name, after the program name. On Linux
the thread ID will match the process TID visible in /proc,
while on other platforms it will merely be an integer repr
of the system thread object address/ID.

This changes the output for both error_report and qemu_log,
when running under the system emulators or the QEMU storage
daemon. Other programs omit the thread information since
they are largely single threaded, though potentially it
would be useful to enable in all of them, given that the RCU
thread will always get spawned by a constructor function.

Before:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
  qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
  qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

After:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
  qemu-system-x86_64: (772366:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
  qemu-system-x86_64: (772366:main): qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
  qemu-system-x86_64: (772366:main): Unable to access credentials fish/ca-cert.pem: No such file or directory

The '-msg thread-info=on|off' argument is introduced to allow this
new default output to be supressed if desired.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/message.h               |  1 +
 qemu-options.hx                      | 10 +++++++---
 storage-daemon/qemu-storage-daemon.c |  6 ++++++
 system/vl.c                          | 17 +++++++++++++++--
 tests/qemu-iotests/041               |  2 +-
 tests/qemu-iotests/common.filter     |  2 +-
 util/message.c                       |  7 +++++++
 7 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/include/qemu/message.h b/include/qemu/message.h
index b8518d06be..2cc092c993 100644
--- a/include/qemu/message.h
+++ b/include/qemu/message.h
@@ -7,6 +7,7 @@ enum QMessageFormatFlags {
     QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
     QMESSAGE_FORMAT_WORKLOAD_NAME = (1 << 1),
     QMESSAGE_FORMAT_PROGRAM_NAME = (1 << 2),
+    QMESSAGE_FORMAT_THREAD_INFO = (1 << 3),
 };
 
 /**
diff --git a/qemu-options.hx b/qemu-options.hx
index 84dcbb0983..50cbecc7d7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5515,15 +5515,16 @@ ERST
 
 DEF("msg", HAS_ARG, QEMU_OPTION_msg,
     "-msg [timestamp=on|off][,guest-name=on|off]\n"
-    "     [,program-name=on|off]\n"
+    "     [,program-name=on|off][,thread-info=on|off]\n"
     "                control error message format\n"
     "                timestamp=on enables timestamps (default: off)\n"
     "                guest-name=on enables guest name prefix but only if\n"
     "                              -name guest option is set (default: off)\n"
-    "                program-name=off disables program name prefix (default: on)\n",
+    "                program-name=off disables program name prefix (default: on)\n"
+    "                thread-info=off disables thread ID and name prefix (default: on)\n",
     QEMU_ARCH_ALL)
 SRST
-``-msg [timestamp=on|off][,guest-name=on|off][,program-name=on|off]``
+``-msg [timestamp=on|off][,guest-name=on|off][,program-name=on|off][,thread-info=on|off]``
     Control error message format.
 
     ``timestamp=on|off``
@@ -5535,6 +5536,9 @@ SRST
 
     ``program-name=on|off``
         Prefix messages with the program name. Default is on.
+
+    ``guest-info=on|off``
+        Prefix messages with the thread ID and name. Default is on.
 ERST
 
 DEF("dump-vmstate", HAS_ARG, QEMU_OPTION_dump_vmstate,
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index eb72561358..cc44ed7848 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -47,6 +47,7 @@
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
+#include "qemu/message.h"
 #include "qemu/help_option.h"
 #include "qemu/job.h"
 #include "qemu/log.h"
@@ -65,6 +66,10 @@ static const char *pid_file;
 static char *pid_file_realpath;
 static volatile bool exit_requested = false;
 
+#define QMESSAGE_FORMAT_DEFAULT \
+    (QMESSAGE_FORMAT_PROGRAM_NAME | \
+     QMESSAGE_FORMAT_THREAD_INFO)
+
 void qemu_system_killed(int signal, pid_t pid)
 {
     exit_requested = true;
@@ -399,6 +404,7 @@ int main(int argc, char *argv[])
 #endif
 
     error_init(argv[0]);
+    qmessage_set_format(QMESSAGE_FORMAT_DEFAULT);
     qemu_init_exec_dir(argv[0]);
     os_setup_signal_handling();
 
diff --git a/system/vl.c b/system/vl.c
index 3e79bf47e0..bedda94103 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -145,6 +145,10 @@
 
 #define MAX_VIRTIO_CONSOLES 1
 
+#define QMESSAGE_FORMAT_DEFAULT \
+    (QMESSAGE_FORMAT_PROGRAM_NAME | \
+     QMESSAGE_FORMAT_THREAD_INFO)
+
 typedef struct BlockdevOptionsQueueEntry {
     BlockdevOptions *bdo;
     Location loc;
@@ -387,6 +391,12 @@ static QemuOptsList qemu_msg_opts = {
             .help = "Prepends program name for error messages (enabled "
                     "by default)\n",
         },
+        {
+            .name = "program-name",
+            .type = QEMU_OPT_BOOL,
+            .help = "Prepends current thread ID and name for error messages "
+                    "(enabled by default)\n",
+        },
         { /* end of list */ }
     },
 };
@@ -822,10 +832,9 @@ static void realtime_init(void)
     }
 }
 
-
 static void configure_msg(QemuOpts *opts)
 {
-    int flags = QMESSAGE_FORMAT_PROGRAM_NAME;
+    int flags = QMESSAGE_FORMAT_DEFAULT;
     if (qemu_opt_get_bool(opts, "timestamp", false)) {
         flags |= QMESSAGE_FORMAT_TIMESTAMP;
     }
@@ -835,6 +844,9 @@ static void configure_msg(QemuOpts *opts)
     if (!qemu_opt_get_bool(opts, "program-name", true)) {
         flags &= ~QMESSAGE_FORMAT_PROGRAM_NAME;
     }
+    if (!qemu_opt_get_bool(opts, "thread-info", true)) {
+        flags &= ~QMESSAGE_FORMAT_THREAD_INFO;
+    }
     qmessage_set_format(flags);
 }
 
@@ -2903,6 +2915,7 @@ void qemu_init(int argc, char **argv)
     module_call_init(MODULE_INIT_OPTS);
 
     error_init(argv[0]);
+    qmessage_set_format(QMESSAGE_FORMAT_DEFAULT);
     qemu_init_exec_dir(argv[0]);
 
     os_setup_limits();
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8452845f44..dc2666afe7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1102,7 +1102,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.vm.shutdown()
         log = iotests.filter_qtest(self.vm.get_log())
         log = re.sub(r'^Formatting.*\n', '', log)
-        log = re.sub(r'^%s: ' % os.path.basename(iotests.qemu_prog), '', log)
+        log = re.sub(r'^%s: \(\d+:\w+\): ' % os.path.basename(iotests.qemu_prog), '', log)
 
         self.assertEqual(log,
                          "Can no longer replace 'img1' by 'repair0', because " +
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 26e6b45b04..c8cb2e860c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -81,7 +81,7 @@ _filter_qemu_io()
 # replace occurrences of QEMU_PROG with "qemu"
 _filter_qemu()
 {
-    gsed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG):#\1QEMU_PROG:#" \
+    gsed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG): ([0-9]\+:[-_a-zA-Z]\+):#\1QEMU_PROG:#" \
         -e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#' \
         -e $'s#\r##' # QEMU monitor uses \r\n line endings
 }
diff --git a/util/message.c b/util/message.c
index dd01bf7462..9bf640c8c4 100644
--- a/util/message.c
+++ b/util/message.c
@@ -40,4 +40,11 @@ void qmessage_context_print(FILE *fp)
             fputs(": ", fp);
         }
     }
+
+    if (message_format & QMESSAGE_FORMAT_THREAD_INFO) {
+        int thid = qemu_get_thread_id();
+        const char *thname = qemu_thread_get_name();
+
+        fprintf(fp, "(%d:%s): ", thid, thname);
+    }
 }
-- 
2.53.0

[PATCH v6 27/27] util: add brackets around guest name in message context

[Daniel P. Berrangé]

The message context can optionally include the guest name if the
argument '-msg guest-name=on' is given. The formatting, however,
does not look good if the guest name contains whitespace. Change
the output to include square brackets to demarcate the name.

Before:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*' -msg guest-name=on -name "fish food"
  fish food qemu-system-x86_64: (1146846:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55657e94e690 dir=fish
  fish food qemu-system-x86_64: (1146846:main): qcrypto_tls_creds_get_path TLS creds path creds=0x55657e94e690 filename=ca-cert.pem path=<none>
  fish food qemu-system-x86_64: (1146846:main): Unable to access credentials fish/ca-cert.pem: No such file or directory

After:

  # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*' -msg guest-name=on -name "fish food"
  [fish food] qemu-system-x86_64: (1144713:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5604ada2c950 dir=fish
  [fish food] qemu-system-x86_64: (1144713:main): qcrypto_tls_creds_get_path TLS creds path creds=0x5604ada2c950 filename=ca-cert.pem path=<none>
  [fish food] qemu-system-x86_64: (1144713:main): 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/message.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/message.c b/util/message.c
index 9bf640c8c4..73af8e8ca4 100644
--- a/util/message.c
+++ b/util/message.c
@@ -29,8 +29,9 @@ void qmessage_context_print(FILE *fp)
 
     if ((message_format & QMESSAGE_FORMAT_WORKLOAD_NAME) &&
         message_workloadname) {
+        fputc('[', fp);
         fputs(message_workloadname, fp);
-        fputc(' ', fp);
+        fputs("] ", fp);
     }
 
     if (message_format & QMESSAGE_FORMAT_PROGRAM_NAME) {
-- 
2.53.0

Re: [PATCH v6 03/27] include: define constant for early constructor priority

[Markus Armbruster]

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

> Functions marked with __attribute__((__constructor__)) will be
> invoked in linker order. In theory this is well defined, but
> in practice, it is hard to determine what this order will be
> with the layers of indirection through meson, ninja and the
> static libraries QEMU builds.
>
> Notably, the order currently appears different between Linux
> and Windows (as tested with Wine on Linux). This can cause
> problems when certain QEMU constructors have a dependancy on
> other QEMU constructors.
>
> To address this define a QEMU_CONSTRUCTOR_EARLY constant which
> provides a priority value that will run before other default
> constructors. This is to be used for QEMU constructors that
> are themselves self-contained, but may be relied upon by other
> constructors.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I'm no fan of doing non-trivial work in constructors.  We discussed
possible alternatives in review of v5.  This is an opinion, not a
demand.  And Paolo's opinion counts a lot more here.

> ---
>  include/qemu/compiler.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 1c2b673c05..4c49f52eb0 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -194,6 +194,14 @@
>  # define QEMU_USED
>  #endif
>  
> +/*
> + * A priority for __attribute__((constructor(...))) that
> + * will run earlier than the default constructors. Must
> + * only be used for functions that have no dependency
> + * on global initialization of other QEMU subsystems.
> + */
> +#define QEMU_CONSTRUCTOR_EARLY 101
> +
>  /*
>   * Disable -ftrivial-auto-var-init on a local variable.
>   *

Re: [PATCH v6 11/27] util: introduce some API docs for logging APIs

[Markus Armbruster]

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

> There is a gotcha with qemu_log() usage in a threaded process.
> If fragments of a log message are output via qemu_log() it is
> possible for messages from two threads to get mixed up. To
> prevent this qemu_log_trylock() should be used, along with
> fprintf(f) calls.
>
> This is a subtle problem that needs to be explained in the
> API docs to ensure correct usage.
>
> In the Rust code, the log_mask_ln method which is conceptually
> equivalent to the C qemu_log() call will unconditionally append
> a newline so must only ever be used for complete log messages.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v6 12/27] util: avoid repeated prefix on incremental qemu_log calls

[Markus Armbruster]

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

> There are three general patterns to QEMU log output
>
>  1. Single complete message calls
>
>       qemu_log("Some message\n");
>
>  2. Direct use of fprintf
>
>       FILE *f = qemu_log_trylock()
>       fprintf(f, "...");
>       fprintf(f, "...");
>       fprintf(f, "...\n");
>       qemu_log_unlock(f)

Real code needs to check @f or risk crashing.  Shouldn't we show that
here?

>  3. Mixed use of qemu_log_trylock/qemu_log()
>
>       FILE *f = qemu_log_trylock()
>       qemu_log("....");
>       qemu_log("....");
>       qemu_log("....\n");
>       qemu_log_unlock(f)
>
> When message prefixes are enabled, the timestamp will be
> unconditionally emitted for all qemu_log() calls. This
> works fine in the 1st case, and has no effect in the 2nd
> case. In the 3rd case, however, we get the timestamp
> printed over & over in each fragment.
>
> One can suggest that pattern (3) is pointless as it is
> functionally identical to (2) but with extra indirection
> and overhead. None the less we have a fair bit of code
> that does this.
>
> The qemu_log() call itself is nothing more than a wrapper
> which does pattern (2) with a single fprintf() call.

qemu_log_trylock()'s lock is recursive.  Worth a comment?  Not sure.

> One might question whether (2) should include the message
> prefix in the same way that (1), but there are scenarios
> where this could be inappropriate / unhelpful such as the
> CPU register dumps or linux-user strace output.
>
> This patch fixes the problem in pattern (3) by keeping
> track of the call depth of qemu_log_trylock() and then
> only emitting the the prefix when the starting depth
> was zero. In doing this qemu_log_trylock_context() is
> also introduced as a variant of qemu_log_trylock()
> that emits the prefix. Callers doing to batch output
> can thus choose whether a prefix is appropriate or
> not.
>
> Fixes: 012842c07552 (log: make '-msg timestamp=on' apply to all qemu_log usage)
> Reported-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v6 12/27] util: avoid repeated prefix on incremental qemu_log calls

[Daniel P. Berrangé]

On Wed, Feb 18, 2026 at 10:52:07AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > There are three general patterns to QEMU log output
> >
> >  1. Single complete message calls
> >
> >       qemu_log("Some message\n");
> >
> >  2. Direct use of fprintf
> >
> >       FILE *f = qemu_log_trylock()
> >       fprintf(f, "...");
> >       fprintf(f, "...");
> >       fprintf(f, "...\n");
> >       qemu_log_unlock(f)
> 
> Real code needs to check @f or risk crashing.  Shouldn't we show that
> here?

Yes, we should really.

> >  3. Mixed use of qemu_log_trylock/qemu_log()
> >
> >       FILE *f = qemu_log_trylock()
> >       qemu_log("....");
> >       qemu_log("....");
> >       qemu_log("....\n");
> >       qemu_log_unlock(f)
> >
> > When message prefixes are enabled, the timestamp will be
> > unconditionally emitted for all qemu_log() calls. This
> > works fine in the 1st case, and has no effect in the 2nd
> > case. In the 3rd case, however, we get the timestamp
> > printed over & over in each fragment.
> >
> > One can suggest that pattern (3) is pointless as it is
> > functionally identical to (2) but with extra indirection
> > and overhead. None the less we have a fair bit of code
> > that does this.
> >
> > The qemu_log() call itself is nothing more than a wrapper
> > which does pattern (2) with a single fprintf() call.
> 
> qemu_log_trylock()'s lock is recursive.  Worth a comment?  Not sure.

Sure.

> > Fixes: 012842c07552 (log: make '-msg timestamp=on' apply to all qemu_log usage)
> > Reported-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 13/27] util/log: add missing error reporting in qemu_log_trylock_with_err

[Markus Armbruster]

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

> One codepath that could return NULL failed to populate the errp
> object.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/log.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/log.c b/util/log.c
> index 1644e6814b..1c9c7adb2d 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -118,6 +118,7 @@ static FILE *qemu_log_trylock_with_err(Error **errp)
   static FILE *qemu_log_trylock_with_err(Error **errp)
   {
       FILE *logfile;

       logfile = thread_file;
       if (!logfile) {

Slow path: no log file cached for this thread.

           if (log_per_thread) {

We're logging to per-thread log files.  @global_filename is a file name
pattern; derive this thread's log file from it.

               g_autofree char *filename
                   = g_strdup_printf(global_filename, log_thread_id());
               logfile = fopen(filename, "w");
               if (!logfile) {
                   error_setg_errno(errp, errno,
                                    "Error opening logfile %s for thread %d",
                                    filename, log_thread_id());
                   return NULL;
               }
               thread_file = logfile;
               qemu_log_thread_cleanup_notifier.notify = qemu_log_thread_cleanup;
               qemu_thread_atexit_add(&qemu_log_thread_cleanup_notifier);
           } else {

Logging to a single log file.  Fetch it from @global_file.

               rcu_read_lock();
               /*
                * FIXME: typeof_strip_qual, as used by qatomic_rcu_read,
                * does not work with pointers to undefined structures,
                * such as we have with struct _IO_FILE and musl libc.
                * Since all we want is a read of a pointer, cast to void**,
                * which does work with typeof_strip_qual.
                */
>              logfile = qatomic_rcu_read((void **)&global_file);
>              if (!logfile) {

@global_file was null when we fetched it.

It can be null when @daemonized and the user didn't set a log file.

>                  rcu_read_unlock();
> +                error_setg(errp, "Global log file output is not open");

Impact?  The only callers passing non-null @errp is
qemu_set_log_internal(), and it can call only when @log_per_thread.  We
can't reach the new error then.

So this is just a latent bug.  Worth mentioning in the commit message?

>                  return NULL;
>              }

We do not set thread_file.  We'll take the slow path again.  Harmless
enough, but its a bit at odds with how the code is structured here.
Observation, not a demand.

>          }
       }

       qemu_flockfile(logfile);
       return logfile;
   }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v6 03/27] include: define constant for early constructor priority

[Daniel P. Berrangé]

On Wed, Feb 18, 2026 at 10:22:27AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Functions marked with __attribute__((__constructor__)) will be
> > invoked in linker order. In theory this is well defined, but
> > in practice, it is hard to determine what this order will be
> > with the layers of indirection through meson, ninja and the
> > static libraries QEMU builds.
> >
> > Notably, the order currently appears different between Linux
> > and Windows (as tested with Wine on Linux). This can cause
> > problems when certain QEMU constructors have a dependancy on
> > other QEMU constructors.
> >
> > To address this define a QEMU_CONSTRUCTOR_EARLY constant which
> > provides a priority value that will run before other default
> > constructors. This is to be used for QEMU constructors that
> > are themselves self-contained, but may be relied upon by other
> > constructors.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> I'm no fan of doing non-trivial work in constructors.  We discussed
> possible alternatives in review of v5.  This is an opinion, not a
> demand.  And Paolo's opinion counts a lot more here.

I think that it is worth exploring the alternative idea, but not
as part of this series which is already getting way longer than
I would like. This constructor usage is trivially removed later
if we get there.

> 
> > ---
> >  include/qemu/compiler.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > index 1c2b673c05..4c49f52eb0 100644
> > --- a/include/qemu/compiler.h
> > +++ b/include/qemu/compiler.h
> > @@ -194,6 +194,14 @@
> >  # define QEMU_USED
> >  #endif
> >  
> > +/*
> > + * A priority for __attribute__((constructor(...))) that
> > + * will run earlier than the default constructors. Must
> > + * only be used for functions that have no dependency
> > + * on global initialization of other QEMU subsystems.
> > + */
> > +#define QEMU_CONSTRUCTOR_EARLY 101
> > +
> >  /*
> >   * Disable -ftrivial-auto-var-init on a local variable.
> >   *
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 14/27] ui: add proper error reporting for password changes

[Markus Armbruster]

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

> Neither the VNC or SPICE code for password changes provides error
> reporting at source, leading the callers to report a largely useless
> generic error message.
>
> Fixing this removes one of the two remaining needs for the undesirable
> error_printf_unless_qmp() method.
>
> While fixing this the error message hint is improved to recommend the
> 'password-secret' option which allows securely passing a password at
> startup.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/ui/console.h                 |  2 +-
>  include/ui/qemu-spice-module.h       |  3 ++-
>  tests/functional/generic/test_vnc.py |  4 ++--
>  ui/spice-core.c                      | 25 ++++++++++++++++++-------
>  ui/spice-module.c                    |  7 ++++---
>  ui/ui-qmp-cmds.c                     | 19 ++++++-------------
>  ui/vnc-stubs.c                       |  6 +++---
>  ui/vnc.c                             | 10 +++++++---
>  8 files changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 98feaa58bd..3677a9d334 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -457,7 +457,7 @@ void qemu_display_help(void);
>  void vnc_display_init(const char *id, Error **errp);
>  void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
> -int vnc_display_password(const char *id, const char *password);
> +int vnc_display_password(const char *id, const char *password, Error **errp);
>  int vnc_display_pw_expire(const char *id, time_t expires);
>  void vnc_parse(const char *str);
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> diff --git a/include/ui/qemu-spice-module.h b/include/ui/qemu-spice-module.h
> index 1f22d557ea..072efa0c83 100644
> --- a/include/ui/qemu-spice-module.h
> +++ b/include/ui/qemu-spice-module.h
> @@ -29,7 +29,8 @@ struct QemuSpiceOps {
>      void (*display_init)(void);
>      int (*migrate_info)(const char *h, int p, int t, const char *s);
>      int (*set_passwd)(const char *passwd,
> -                      bool fail_if_connected, bool disconnect_if_connected);
> +                      bool fail_if_connected, bool disconnect_if_connected,
> +                      Error **errp);
>      int (*set_pw_expire)(time_t expires);
>      int (*display_add_client)(int csock, int skipauth, int tls);
>  #ifdef CONFIG_SPICE
> diff --git a/tests/functional/generic/test_vnc.py b/tests/functional/generic/test_vnc.py
> index f1dd1597cf..097f858ca1 100755
> --- a/tests/functional/generic/test_vnc.py
> +++ b/tests/functional/generic/test_vnc.py
> @@ -48,7 +48,7 @@ def test_no_vnc_change_password(self):
>          self.assertEqual(set_password_response['error']['class'],
>                           'GenericError')
>          self.assertEqual(set_password_response['error']['desc'],
> -                         'Could not set password')
> +                         'No VNC display is present');
>  
>      def launch_guarded(self):
>          try:
> @@ -73,7 +73,7 @@ def test_change_password_requires_a_password(self):
>          self.assertEqual(set_password_response['error']['class'],
>                           'GenericError')
>          self.assertEqual(set_password_response['error']['desc'],
> -                         'Could not set password')
> +                         'VNC password authentication is disabled')
>  
>      def test_change_password(self):
>          self.set_machine('none')
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 8a6050f4ae..cdcec34f67 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -756,7 +756,7 @@ static void qemu_spice_init(void)
>                               tls_ciphers);
>      }
>      if (password) {
> -        qemu_spice.set_passwd(password, false, false);
> +        qemu_spice.set_passwd(password, false, false, NULL);

qemu_spice.set_passwd is qemu_spice_set_passwd().  It's converted to
Error below.  That conversion doesn't replace error reporting, it only
adds.  Therefore, passing NULL does not lose error reporting here.
Good.

However, why is ignoring errors okay here?  Not this patch's fault, of
course.

>      }
>      if (qemu_opt_get_bool(opts, "sasl", 0)) {
>          if (spice_server_set_sasl(spice_server, 1) == -1) {
               error_report("spice: failed to enable sasl");
               exit(1);

For what it's worth, we treat this error as fatal.

           }
           auth = "sasl";
       }
> @@ -919,8 +919,10 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
>      return qemu_spice_add_interface(&qxlin->base);
>  }
>  
> -static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
> +static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn,
> +                                 Error **errp)
>  {
> +    int ret;
>      time_t lifetime, now = time(NULL);
>      char *passwd;
>  
> @@ -934,26 +936,35 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
>          passwd = NULL;
>          lifetime = 1;
>      }
> -    return spice_server_set_ticket(spice_server, passwd, lifetime,
> -                                   fail_if_conn, disconnect_if_conn);
> +    ret = spice_server_set_ticket(spice_server, passwd, lifetime,
> +                                  fail_if_conn, disconnect_if_conn);
> +    if (ret < 0) {
> +        error_setg(errp, "Unable to set SPICE server ticket");
> +        return -1;
> +    }
> +    return 0;
>  }
>  
>  static int qemu_spice_set_passwd(const char *passwd,
> -                                 bool fail_if_conn, bool disconnect_if_conn)
> +                                 bool fail_if_conn, bool disconnect_if_conn,
> +                                 Error **errp)
>  {
>      if (strcmp(auth, "spice") != 0) {
> +        error_setg(errp, "SPICE authentication is disabled");
> +        error_append_hint(errp,
> +                          "To enable it use '-spice ...,password-secret=ID'");
>          return -1;
>      }
>  
>      g_free(auth_passwd);
>      auth_passwd = g_strdup(passwd);
> -    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
> +    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn, errp);
>  }
>  
>  static int qemu_spice_set_pw_expire(time_t expires)
>  {
>      auth_expires = expires;
> -    return qemu_spice_set_ticket(false, false);
> +    return qemu_spice_set_ticket(false, false, NULL);
>  }
>  
>  static int qemu_spice_display_add_client(int csock, int skipauth, int tls)
> diff --git a/ui/spice-module.c b/ui/spice-module.c
> index 3222335872..7651c85885 100644
> --- a/ui/spice-module.c
> +++ b/ui/spice-module.c
> @@ -45,14 +45,15 @@ static int qemu_spice_migrate_info_stub(const char *h, int p, int t,
>  
>  static int qemu_spice_set_passwd_stub(const char *passwd,
>                                        bool fail_if_connected,
> -                                      bool disconnect_if_connected)
> +                                      bool disconnect_if_connected,
> +                                      Error **errp)
>  {
> -    return -1;
> +    g_assert_not_reached();
>  }
>  
>  static int qemu_spice_set_pw_expire_stub(time_t expires)
>  {
> -    return -1;
> +    g_assert_not_reached();
>  }
>  

Makes the stubs' "just to keep the linker happy" nature obvious.  I like
it.

> static int qemu_spice_display_add_client_stub(int csock, int skipauth,
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index b49b636152..1173c82cf7 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -31,15 +31,14 @@
>  
>  void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>  {
> -    int rc;
> -
>      if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
> -        rc = qemu_spice.set_passwd(opts->password,
> -                opts->connected == SET_PASSWORD_ACTION_FAIL,
> -                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
> +        qemu_spice.set_passwd(opts->password,
> +                              opts->connected == SET_PASSWORD_ACTION_FAIL,
> +                              opts->connected == SET_PASSWORD_ACTION_DISCONNECT,
> +                              errp);
>      } else {
>          assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
>          if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
> @@ -52,11 +51,7 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>           * Note that setting an empty password will not disable login
>           * through this interface.
>           */
> -        rc = vnc_display_password(opts->u.vnc.display, opts->password);
> -    }
> -
> -    if (rc != 0) {
> -        error_setg(errp, "Could not set password");
> +        vnc_display_password(opts->u.vnc.display, opts->password, errp);
>      }
>  }
>  
> @@ -107,9 +102,7 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>  #ifdef CONFIG_VNC
>  void qmp_change_vnc_password(const char *password, Error **errp)
>  {
> -    if (vnc_display_password(NULL, password) < 0) {
> -        error_setg(errp, "Could not set password");
> -    }
> +    vnc_display_password(NULL, password, errp);
>  }
>  #endif
>  

The QMP commands become simpler.  Nice!

> diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
> index a96bc86236..5de9bf9d70 100644
> --- a/ui/vnc-stubs.c
> +++ b/ui/vnc-stubs.c
> @@ -2,11 +2,11 @@
>  #include "ui/console.h"
>  #include "qapi/error.h"
>  
> -int vnc_display_password(const char *id, const char *password)
> +int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
> -    return -ENODEV;
> +    g_assert_not_reached();
>  }
>  int vnc_display_pw_expire(const char *id, time_t expires)
>  {
> -    return -ENODEV;
> +    g_assert_not_reached();
>  };

Like it.

> diff --git a/ui/vnc.c b/ui/vnc.c
> index a61a4f937d..833e0e2e68 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3526,16 +3526,20 @@ static void vnc_display_close(VncDisplay *vd)
>  #endif
>  }
>  
> -int vnc_display_password(const char *id, const char *password)
> +int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
>      VncDisplay *vd = vnc_display_find(id);
>  
>      if (!vd) {
> +        error_setg(errp, "No VNC display is present");
> +        error_append_hint(errp,
> +                          "To enable it, use '-vnc ...'");
>          return -EINVAL;
>      }
>      if (vd->auth == VNC_AUTH_NONE) {
> -        error_printf_unless_qmp("If you want use passwords please enable "
> -                                "password auth using '-vnc ${dpy},password'.\n");
> +        error_setg(errp, "VNC password authentication is disabled");
> +        error_append_hint(errp,
> +                          "To enable it, use '-vnc ...,password-secret=ID'");
>          return -EINVAL;
>      }

The only issue I found is not this patch's problem, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v6 15/27] ui: remove redundant use of error_printf_unless_qmp()

[Markus Armbruster]

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

> The vnc_display_print_local_addr() method is intended to print the VNC
> listening address on the console at startup, so the user can see the
> auto-chosen port address when using the 'to=' flag. This is only called
> by vnc_display_open() which is in the QEMU startup callpath. The check
> for not being in QMP is thus redundant and can be removed.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/vnc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 833e0e2e68..fe020dcc4c 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3578,9 +3578,9 @@ static void vnc_display_print_local_addr(VncDisplay *vd)
>          qapi_free_SocketAddress(addr);
>          return;
>      }
> -    error_printf_unless_qmp("VNC server running on %s:%s\n",
> -                            addr->u.inet.host,
> -                            addr->u.inet.port);
> +    error_printf("VNC server running on %s:%s\n",
> +                 addr->u.inet.host,
> +                 addr->u.inet.port);
>      qapi_free_SocketAddress(addr);
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Spotted in review: vnc_display_open() could be static.  Observation, not
a demand.

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

[Markus Armbruster]

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>

[...]

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

[Markus Armbruster]

One more...

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:

[...]

> 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

[...]

> @@ -156,34 +176,34 @@ void loc_set_file(const char *fname, int lno)
>  /*
>   * Print current location to current 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.

>   */
> -static void print_loc(void)
> +static void print_loc(Monitor *cur)
>  {
>      const char *sep = "";
>      int i;

[...]

Re: [PATCH v6 20/27] util: don't skip error prefixes when QMP is active

[Markus Armbruster]

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

> The vreport() function will print to HMP if available, otherwise
> to stderr. In the event that vreport() is called during execution
> of a QMP command, it will print to stderr, but mistakenly omit the
> message prefixes (timestamp, guest name, program name).
>
> This new usage of monitor_is_cur_qmp() from vreport() requires that
> we add a stub to satisfy linking of non-system emulator binaries.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  stubs/monitor-core.c           | 5 +++++
>  tests/unit/test-util-sockets.c | 1 +
>  util/error-report.c            | 8 ++++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index a7c32297c9..078a5012e9 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
>      return NULL;
>  }
>  
> +bool monitor_cur_is_qmp(void)
> +{
> +    return false;
> +}
> +
>  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon)
>  {
>      return NULL;
> diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> index ee66d727c3..b9f2453e29 100644
> --- a/tests/unit/test-util-sockets.c
> +++ b/tests/unit/test-util-sockets.c
> @@ -74,6 +74,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>  Monitor *monitor_cur(void) { return cur_mon; }
>  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
> +bool monitor_cur_is_qmp(void) { abort(); };
>  
>  #ifndef _WIN32
>  static void test_socket_fd_pass_name_good(void)
> diff --git a/util/error-report.c b/util/error-report.c
> index b34a2bc7bc..052f0f68b8 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -227,6 +227,14 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>      Monitor *cur = monitor_cur();
>      gchar *timestr;
>  
> +    /*
> +     * When current monitor is QMP, messages must go to stderr
> +     * and have prefixes added

Let's end the sentence with a period.

> +     */
> +    if (monitor_cur_is_qmp()) {
> +        cur = NULL;
> +    }
> +
>      if (message_with_timestamp && !cur) {
>          timestr = real_time_iso8601();
>          fprintf(stderr, "%s ", timestr);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v6 03/27] include: define constant for early constructor priority

[Markus Armbruster]

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

> On Wed, Feb 18, 2026 at 10:22:27AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > Functions marked with __attribute__((__constructor__)) will be
>> > invoked in linker order. In theory this is well defined, but
>> > in practice, it is hard to determine what this order will be
>> > with the layers of indirection through meson, ninja and the
>> > static libraries QEMU builds.
>> >
>> > Notably, the order currently appears different between Linux
>> > and Windows (as tested with Wine on Linux). This can cause
>> > problems when certain QEMU constructors have a dependancy on
>> > other QEMU constructors.
>> >
>> > To address this define a QEMU_CONSTRUCTOR_EARLY constant which
>> > provides a priority value that will run before other default
>> > constructors. This is to be used for QEMU constructors that
>> > are themselves self-contained, but may be relied upon by other
>> > constructors.
>> >
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> 
>> I'm no fan of doing non-trivial work in constructors.  We discussed
>> possible alternatives in review of v5.  This is an opinion, not a
>> demand.  And Paolo's opinion counts a lot more here.
>
> I think that it is worth exploring the alternative idea, but not
> as part of this series which is already getting way longer than
> I would like. This constructor usage is trivially removed later
> if we get there.

Fair!

Re: [PATCH v6 22/27] util: introduce common helper for error-report & log code

[Markus Armbruster]

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

> The error-report and log code both have a need to add prefixes
> to messages they are printing, with the current example being
> a timestamp.
>
> The format and configuration they use should be consistent, so
> providing a common helper will ensure this is always the case.
> Initially the helper only emits a timestamp, but future patches
> will expand this.
>
> This takes the liberty of assigning the new file to the same
> maintainer as the existing error-report.c file, given it will
> be extracting some functionality from the latter.

Fair.

> While vreport() dynamically changes between reporting to the
> monitor vs stderr, depending on whether HMP is active or not,
> message prefixes are only ever used in the non-HMP case. Thus
> the helper API can take a FILE * object and not have to deal
> with the monitor at all.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  MAINTAINERS            |  2 ++
>  include/qemu/message.h | 28 ++++++++++++++++++++++++++++
>  util/meson.build       |  1 +
>  util/message.c         | 23 +++++++++++++++++++++++
>  4 files changed, 54 insertions(+)
>  create mode 100644 include/qemu/message.h
>  create mode 100644 util/message.c

No uses, yet.  Commit message could mention it's put to use in the next
few commits.

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29f88d48f3..96a4ce438e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3261,9 +3261,11 @@ M: Markus Armbruster <armbru@redhat.com>
>  S: Supported
>  F: include/qapi/error.h
>  F: include/qemu/error-report.h
> +F: include/qemu/message.h
>  F: qapi/error.json
>  F: util/error.c
>  F: util/error-report.c
> +F: util/message.c
>  F: scripts/coccinelle/err-bad-newline.cocci
>  F: scripts/coccinelle/error-use-after-free.cocci
>  F: scripts/coccinelle/error_propagate_null.cocci
> diff --git a/include/qemu/message.h b/include/qemu/message.h
> new file mode 100644
> index 0000000000..0a06421f77
> --- /dev/null
> +++ b/include/qemu/message.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef QEMU_MESSAGE_H
> +#define QEMU_MESSAGE_H
> +
> +enum QMessageFormatFlags {
> +    QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
> +};
> +
> +/**
> + * qmessage_set_format:
> + * @flags: the message information to emit
> + *
> + * Select which pieces of information to
> + * emit for messages
> + */
> +void qmessage_set_format(int flags);
> +
> +/**
> + * qmessage_context_print:
> + * @fp: file to emit the prefix on
> + *
> + * Emit a message prefix with the information selected by
> + * an earlier call to qmessage_set_format.
> + */
> +void qmessage_context_print(FILE *fp);
> +
> +#endif /* QEMU_MESSAGE_H */
> diff --git a/util/meson.build b/util/meson.build
> index 7c9445615d..3035bd4bee 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -38,6 +38,7 @@ util_ss.add(files('host-utils.c'))
>  util_ss.add(files('bitmap.c', 'bitops.c'))
>  util_ss.add(files('fifo8.c'))
>  util_ss.add(files('cacheflush.c'))
> +util_ss.add(files('message.c'))
>  util_ss.add(files('error.c', 'error-report.c'))
>  util_ss.add(files('qemu-print.c'))
>  util_ss.add(files('id.c'))
> diff --git a/util/message.c b/util/message.c
> new file mode 100644
> index 0000000000..99a403f9d0
> --- /dev/null
> +++ b/util/message.c
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +
> +#include "qemu/message.h"
> +#include "monitor/monitor.h"

Superfluous #include.

It'll become used in PATCH 26, for qemu_thread_get_name().  Should
include qemu/thread.h there instead.

> +
> +static int message_format;
> +
> +void qmessage_set_format(int flags)
> +{
> +    message_format = flags;
> +}
> +
> +void qmessage_context_print(FILE *fp)
> +{
> +    if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
> +        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
> +        g_autofree char *timestr = g_date_time_format_iso8601(dt);
> +        fputs(timestr, fp);
> +        fputc(' ', fp);

The context string is either empty, or it ends with a space, for ease of
use.  Okay.

I'd go for

           fprintf(fp, "%s ", timestr);

> +    }
> +}

Alright, everybody's favorite topic: naming.

message.[ch] aren't about messages, but message *prefixes*.  You call
them "context" in qmessage_context_print().  I'm fine with "context".

External symbols are prefixed with qmessage_.  I prefer such prefixes to
match the filename.

Prefix in util/ overwhelmingly start with qemu_.

Somewhat long prefixes feel okay here, as these symbols are used only a
couple of times.  qemu_message_context_ might be too long, though.

Could use the classic technique of murdering vowels: to qemu_msg_ctxt_.

Thoughts?

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Markus Armbruster]

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

> The error_report function can include the program name in any
> messages it prints. The qemu_log function has no equivalent
> behaviour.
>
> This introduces support for a "program name" in the new
> messages API, which will be included by default for all
> binaries.
>
> This change tweaks the output of the error_report function,
> adding a space between the program name and the location
> info.

Why?

>       The qemu_log function will gain the program name. This
> can be easily seen with the 'log' trace backend, and how it
> is now more closely matching error_report output.
>
> Before:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>   qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> After:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> When adding this the '-msg program-name=on|off' option is
> introduced, so that the program name (which is enabled by
> default) can be supressed if desired. This could be useful
> if '-msg guest-name=on' is being used as a more informative
> identifier.

Separate patch?

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Re: [PATCH v6 26/27] util: add support for formatting thread info in messages

[Markus Armbruster]

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

> The message context is now extended to be able to include the
> thread ID and thread name, after the program name. On Linux
> the thread ID will match the process TID visible in /proc,
> while on other platforms it will merely be an integer repr
> of the system thread object address/ID.
>
> This changes the output for both error_report and qemu_log,
> when running under the system emulators or the QEMU storage
> daemon.

Yes:

    $ qemu-system-x86_64 -object tls-creds-x509,id=t0
    qemu-system-x86_64: (1116063:main): Missing 'dir' property value

I do not like this change.  The part in (parenthesis) is gobbledygook
for non-developers.  I feel it should be off by default.

Making it on by default for log message (as shown below) is fine.

>         Other programs omit the thread information since
> they are largely single threaded, though potentially it
> would be useful to enable in all of them, given that the RCU
> thread will always get spawned by a constructor function.
>
> Before:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
>   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> After:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
>   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: (772366:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> The '-msg thread-info=on|off' argument is introduced to allow this
> new default output to be supressed if desired.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Re: [PATCH v6 27/27] util: add brackets around guest name in message context

[Markus Armbruster]

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

> The message context can optionally include the guest name if the
> argument '-msg guest-name=on' is given. The formatting, however,
> does not look good if the guest name contains whitespace. Change
> the output to include square brackets to demarcate the name.
>
> Before:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*' -msg guest-name=on -name "fish food"
>   fish food qemu-system-x86_64: (1146846:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55657e94e690 dir=fish
>   fish food qemu-system-x86_64: (1146846:main): qcrypto_tls_creds_get_path TLS creds path creds=0x55657e94e690 filename=ca-cert.pem path=<none>
>   fish food qemu-system-x86_64: (1146846:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> After:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*' -msg guest-name=on -name "fish food"
>   [fish food] qemu-system-x86_64: (1144713:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5604ada2c950 dir=fish
>   [fish food] qemu-system-x86_64: (1144713:main): qcrypto_tls_creds_get_path TLS creds path creds=0x5604ada2c950 filename=ca-cert.pem path=<none>
>   [fish food] qemu-system-x86_64: (1144713:main): 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/message.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/util/message.c b/util/message.c
> index 9bf640c8c4..73af8e8ca4 100644
> --- a/util/message.c
> +++ b/util/message.c
> @@ -29,8 +29,9 @@ void qmessage_context_print(FILE *fp)
>  
>      if ((message_format & QMESSAGE_FORMAT_WORKLOAD_NAME) &&
>          message_workloadname) {
> +        fputc('[', fp);
>          fputs(message_workloadname, fp);
> -        fputc(' ', fp);
> +        fputs("] ", fp);

Again, I'd go for

           fprintf(fp, "[%s] ", message_workloadname);

>      }
>  
>      if (message_format & QMESSAGE_FORMAT_PROGRAM_NAME) {

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Peter Maydell]

On Wed, 11 Feb 2026 at 15:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The error_report function can include the program name in any
> messages it prints. The qemu_log function has no equivalent
> behaviour.
>
> This introduces support for a "program name" in the new
> messages API, which will be included by default for all
> binaries.
>
> This change tweaks the output of the error_report function,
> adding a space between the program name and the location
> info. The qemu_log function will gain the program name. This
> can be easily seen with the 'log' trace backend, and how it
> is now more closely matching error_report output.
>
> Before:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>   qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> After:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory

On the other hand if you're using the logs for debugging
then you now have an extra big pointless prefix on them that
you have to turn off. Especially if you're logging to a file
that's a lot of extra garbage in the output. I'm not really
looking forward to now having to give QEMU an extra long
and unwieldy command line argument every time I want to
do debug or tracepoint logging.

Why do we care about the qemu_log output matching the
error-report output? The logs are expected to be
quite frequent, to only be there if you've explicitly
turned them on, and to be usually directed to a log file.
The error reporting is more likely to be infrequent, to
be on stdout, and to be important/fatal things.

-- PMM

Re: [PATCH v6 26/27] util: add support for formatting thread info in messages

[Peter Maydell]

On Wed, 11 Feb 2026 at 15:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
> After:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
>   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: (772366:main): Unable to access credentials fish/ca-cert.pem: No such file or directory

Even more output enabled by default that is pretty useless for most
uses of tracepoints and debug logging :-(

Please can we have this stuff all off-by-default? People
who want this extra info can enable it.

-- PMM

Re: [PATCH v6 14/27] ui: add proper error reporting for password changes

[Daniel P. Berrangé]

On Wed, Feb 18, 2026 at 01:10:09PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Neither the VNC or SPICE code for password changes provides error
> > reporting at source, leading the callers to report a largely useless
> > generic error message.
> >
> > Fixing this removes one of the two remaining needs for the undesirable
> > error_printf_unless_qmp() method.
> >
> > While fixing this the error message hint is improved to recommend the
> > 'password-secret' option which allows securely passing a password at
> > startup.
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/ui/console.h                 |  2 +-
> >  include/ui/qemu-spice-module.h       |  3 ++-
> >  tests/functional/generic/test_vnc.py |  4 ++--
> >  ui/spice-core.c                      | 25 ++++++++++++++++++-------
> >  ui/spice-module.c                    |  7 ++++---
> >  ui/ui-qmp-cmds.c                     | 19 ++++++-------------
> >  ui/vnc-stubs.c                       |  6 +++---
> >  ui/vnc.c                             | 10 +++++++---
> >  8 files changed, 43 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 98feaa58bd..3677a9d334 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -457,7 +457,7 @@ void qemu_display_help(void);
> >  void vnc_display_init(const char *id, Error **errp);
> >  void vnc_display_open(const char *id, Error **errp);
> >  void vnc_display_add_client(const char *id, int csock, bool skipauth);
> > -int vnc_display_password(const char *id, const char *password);
> > +int vnc_display_password(const char *id, const char *password, Error **errp);
> >  int vnc_display_pw_expire(const char *id, time_t expires);
> >  void vnc_parse(const char *str);
> >  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> > diff --git a/include/ui/qemu-spice-module.h b/include/ui/qemu-spice-module.h
> > index 1f22d557ea..072efa0c83 100644
> > --- a/include/ui/qemu-spice-module.h
> > +++ b/include/ui/qemu-spice-module.h
> > @@ -29,7 +29,8 @@ struct QemuSpiceOps {
> >      void (*display_init)(void);
> >      int (*migrate_info)(const char *h, int p, int t, const char *s);
> >      int (*set_passwd)(const char *passwd,
> > -                      bool fail_if_connected, bool disconnect_if_connected);
> > +                      bool fail_if_connected, bool disconnect_if_connected,
> > +                      Error **errp);
> >      int (*set_pw_expire)(time_t expires);
> >      int (*display_add_client)(int csock, int skipauth, int tls);
> >  #ifdef CONFIG_SPICE
> > diff --git a/tests/functional/generic/test_vnc.py b/tests/functional/generic/test_vnc.py
> > index f1dd1597cf..097f858ca1 100755
> > --- a/tests/functional/generic/test_vnc.py
> > +++ b/tests/functional/generic/test_vnc.py
> > @@ -48,7 +48,7 @@ def test_no_vnc_change_password(self):
> >          self.assertEqual(set_password_response['error']['class'],
> >                           'GenericError')
> >          self.assertEqual(set_password_response['error']['desc'],
> > -                         'Could not set password')
> > +                         'No VNC display is present');
> >  
> >      def launch_guarded(self):
> >          try:
> > @@ -73,7 +73,7 @@ def test_change_password_requires_a_password(self):
> >          self.assertEqual(set_password_response['error']['class'],
> >                           'GenericError')
> >          self.assertEqual(set_password_response['error']['desc'],
> > -                         'Could not set password')
> > +                         'VNC password authentication is disabled')
> >  
> >      def test_change_password(self):
> >          self.set_machine('none')
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 8a6050f4ae..cdcec34f67 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -756,7 +756,7 @@ static void qemu_spice_init(void)
> >                               tls_ciphers);
> >      }
> >      if (password) {
> > -        qemu_spice.set_passwd(password, false, false);
> > +        qemu_spice.set_passwd(password, false, false, NULL);
> 
> qemu_spice.set_passwd is qemu_spice_set_passwd().  It's converted to
> Error below.  That conversion doesn't replace error reporting, it only
> adds.  Therefore, passing NULL does not lose error reporting here.
> Good.
> 
> However, why is ignoring errors okay here?  Not this patch's fault, of
> course.

IIUC There are two scenarios qemu_spice_set_passwd can fail

 * The password auth method is not enabled
     -> minor config bug, but we should report that error
 * The fail_if_conn is true and a client is connected
     -> impossible scenario during startup, fine to ignore



With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 22/27] util: introduce common helper for error-report & log code

[Daniel P. Berrangé]

On Wed, Feb 18, 2026 at 03:04:23PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The error-report and log code both have a need to add prefixes
> > to messages they are printing, with the current example being
> > a timestamp.
> >
> > The format and configuration they use should be consistent, so
> > providing a common helper will ensure this is always the case.
> > Initially the helper only emits a timestamp, but future patches
> > will expand this.
> >
> > This takes the liberty of assigning the new file to the same
> > maintainer as the existing error-report.c file, given it will
> > be extracting some functionality from the latter.
> 
> Fair.
> 
> > While vreport() dynamically changes between reporting to the
> > monitor vs stderr, depending on whether HMP is active or not,
> > message prefixes are only ever used in the non-HMP case. Thus
> > the helper API can take a FILE * object and not have to deal
> > with the monitor at all.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  MAINTAINERS            |  2 ++
> >  include/qemu/message.h | 28 ++++++++++++++++++++++++++++
> >  util/meson.build       |  1 +
> >  util/message.c         | 23 +++++++++++++++++++++++
> >  4 files changed, 54 insertions(+)
> >  create mode 100644 include/qemu/message.h
> >  create mode 100644 util/message.c

snip

> > diff --git a/util/message.c b/util/message.c
> > new file mode 100644
> > index 0000000000..99a403f9d0
> > --- /dev/null
> > +++ b/util/message.c
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "qemu/message.h"
> > +#include "monitor/monitor.h"
> 
> Superfluous #include.
> 
> It'll become used in PATCH 26, for qemu_thread_get_name().  Should
> include qemu/thread.h there instead.

opps, yes.

> > +
> > +static int message_format;
> > +
> > +void qmessage_set_format(int flags)
> > +{
> > +    message_format = flags;
> > +}
> > +
> > +void qmessage_context_print(FILE *fp)
> > +{
> > +    if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
> > +        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
> > +        g_autofree char *timestr = g_date_time_format_iso8601(dt);
> > +        fputs(timestr, fp);
> > +        fputc(' ', fp);
> 
> The context string is either empty, or it ends with a space, for ease of
> use.  Okay.
> 
> I'd go for
> 
>            fprintf(fp, "%s ", timestr);

Previous reviewer comments preferred fputs/c to avoid
redundant printf string interpolation since qemu_log
could be used in fairly hot code paths  at times.

> 
> > +    }
> > +}
> 
> Alright, everybody's favorite topic: naming.
> 
> message.[ch] aren't about messages, but message *prefixes*.  You call
> them "context" in qmessage_context_print().  I'm fine with "context".

The use of "message" was a somewhat forward looking thing, guessing
at possible other needs related to message ouput.

In particular I think there's scope for the Location handling APIs to
be in this file instead of error-report.c.

So one could imagine  qmessage_loc_push/pop  APis later.

> External symbols are prefixed with qmessage_.  I prefer such prefixes to
> match the filename.

My view is that they do match if you pretend the 'q' is implicit by
this living inside qemu.git ;-P

> Prefix in util/ overwhelmingly start with qemu_.

Naturally my choice was based on what I've previously done for
naming in io/, crypto/ and auth/, etc where all the C APIs have
a leading 'q' to scope them to QEMU, but this is omitted in the
directory/file names :-)

> Somewhat long prefixes feel okay here, as these symbols are used only a
> couple of times.  qemu_message_context_ might be too long, though.
> 
> Could use the classic technique of murdering vowels: to qemu_msg_ctxt_.
> 
> Thoughts?

IMHO, despite the existing usage in util/, "qemu_" is overkill as a
naming prefix.

A plain 'q' prefix is most liable to clash with "Qt" library functions,
but we don't consume that in QEMU so largely not neccessary to worry
about unless perhaps dragged in indirectly via a 3rd party dep

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Daniel P. Berrangé]

On Thu, Feb 19, 2026 at 11:08:45AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The error_report function can include the program name in any
> > messages it prints. The qemu_log function has no equivalent
> > behaviour.
> >
> > This introduces support for a "program name" in the new
> > messages API, which will be included by default for all
> > binaries.
> >
> > This change tweaks the output of the error_report function,
> > adding a space between the program name and the location
> > info.
> 
> Why?

Looking again, arguably that is fixing a pre-existing bug

The current code is:

    if (!monitor_cur() && g_get_prgname()) {
        error_printf("%s:", g_get_prgname());
        sep = " ";
    }
    switch (cur_loc->kind) {
    case LOC_CMDLINE:
        argp = cur_loc->ptr;
        for (i = 0; i < cur_loc->num; i++) {
            error_printf("%s%s", sep, argp[i]);
            sep = " ";
        }
        error_printf(": ");
        break;
    case LOC_FILE:
        error_printf("%s:", (const char *)cur_loc->ptr);
        if (cur_loc->num) {
            error_printf("%d:", cur_loc->num);
        }
        error_printf(" ");
        break;
    default:
        error_printf("%s", sep);
    }


Notice how we honour "sep" for LOC_CMDLINE but ignore
it for LOC_FILE.

With the program name printing moved out into a separate
function, the space is always printed. No change for
LOC_CMDLINE, but new whitespace for LOC_FILE.

Should have changed this in a separate patch.

> >       The qemu_log function will gain the program name. This
> > can be easily seen with the 'log' trace backend, and how it
> > is now more closely matching error_report output.
> >
> > Before:
> >
> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> >   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
> >   qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
> >
> > After:
> >
> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> >   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
> >   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
> >
> > When adding this the '-msg program-name=on|off' option is
> > introduced, so that the program name (which is enabled by
> > default) can be supressed if desired. This could be useful
> > if '-msg guest-name=on' is being used as a more informative
> > identifier.
> 
> Separate patch?

It is a bit of a borderline decision, but I felt the knob to turn it
off should be in the same patch that force enables it.

> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 26/27] util: add support for formatting thread info in messages

[Daniel P. Berrangé]

On Thu, Feb 19, 2026 at 10:29:56AM +0000, Peter Maydell wrote:
> On Wed, 11 Feb 2026 at 15:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > After:
> >
> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> >   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
> >   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
> >   qemu-system-x86_64: (772366:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
> 
> Even more output enabled by default that is pretty useless for most
> uses of tracepoints and debug logging :-(

Getting the thread info in qemu log output was the original primary
motivating factor in this series. This short example doesn't show
it, but when debugging/tracing QEMU it is pretty important to
understand which messages are coming from different threads, so we
can correctly interpret the control flow. Pretty much every logging
framework these days will include some thread identifier by default,
and IMHO it is reasonable to do this by default in QEMU too.

There was a proposal a while back on the list to add stack trace
dumps to qemu_log output to allow threads to be distinguished, which
obviously could never be enabled by default. The short thread ident
info was something that could be unconditionally enabled so we get
more useful info from users reporting bugs without having to go
back and ask them to re-do the log collection with more options.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 26/27] util: add support for formatting thread info in messages

[Daniel P. Berrangé]

On Thu, Feb 19, 2026 at 11:14:54AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The message context is now extended to be able to include the
> > thread ID and thread name, after the program name. On Linux
> > the thread ID will match the process TID visible in /proc,
> > while on other platforms it will merely be an integer repr
> > of the system thread object address/ID.
> >
> > This changes the output for both error_report and qemu_log,
> > when running under the system emulators or the QEMU storage
> > daemon.
> 
> Yes:
> 
>     $ qemu-system-x86_64 -object tls-creds-x509,id=t0
>     qemu-system-x86_64: (1116063:main): Missing 'dir' property value
> 
> I do not like this change.  The part in (parenthesis) is gobbledygook
> for non-developers.  I feel it should be off by default.
> 
> Making it on by default for log message (as shown below) is fine.

Hmm, challenging as now we have to distiguish callers when formatting
the message prefixes, but I can see your point.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Daniel P. Berrangé]

On Thu, Feb 19, 2026 at 10:23:36AM +0000, Peter Maydell wrote:
> On Wed, 11 Feb 2026 at 15:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The error_report function can include the program name in any
> > messages it prints. The qemu_log function has no equivalent
> > behaviour.
> >
> > This introduces support for a "program name" in the new
> > messages API, which will be included by default for all
> > binaries.
> >
> > This change tweaks the output of the error_report function,
> > adding a space between the program name and the location
> > info. The qemu_log function will gain the program name. This
> > can be easily seen with the 'log' trace backend, and how it
> > is now more closely matching error_report output.
> >
> > Before:
> >
> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> >   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
> >   qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
> >
> > After:
> >
> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> >   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
> >   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
> 
> On the other hand if you're using the logs for debugging
> then you now have an extra big pointless prefix on them that
> you have to turn off. Especially if you're logging to a file
> that's a lot of extra garbage in the output. I'm not really
> looking forward to now having to give QEMU an extra long
> and unwieldy command line argument every time I want to
> do debug or tracepoint logging.
> 
> Why do we care about the qemu_log output matching the
> error-report output? The logs are expected to be
> quite frequent, to only be there if you've explicitly
> turned them on, and to be usually directed to a log file.

Personally I never use the log-to-file facility. Instead typical use
case is relying on the trace points 'log' facility as illustrated in
the commit message example to dump to stderr for simple debugging,
and having consistent formatting between the logs & errors is useful
there.

In effect this patch series is implicitly viewing the error report
output as a being a variant of log output with a level of "ERROR",
despite us having completely separate logic for logs vs errors
internally.

In this particular patch, I didn't especially have a use case for
the program name in the log output, it instead of fell out of the
general goal of using the same formatting logic for both subsystems.

I'll think about how to alter this.

> The error reporting is more likely to be infrequent, to
> be on stdout, and to be important/fatal things.
> 
> -- PMM
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 26/27] util: add support for formatting thread info in messages

[Peter Maydell]

On Wed, 25 Feb 2026 at 16:30, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Feb 19, 2026 at 10:29:56AM +0000, Peter Maydell wrote:
> > On Wed, 11 Feb 2026 at 15:29, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > After:
> > >
> > >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
> > >   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x560db818e080 dir=fish
> > >   qemu-system-x86_64: (772366:main): qcrypto_tls_creds_get_path TLS creds path creds=0x560db818e080 filename=ca-cert.pem path=<none>
> > >   qemu-system-x86_64: (772366:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
> >
> > Even more output enabled by default that is pretty useless for most
> > uses of tracepoints and debug logging :-(
>
> Getting the thread info in qemu log output was the original primary
> motivating factor in this series. This short example doesn't show
> it, but when debugging/tracing QEMU it is pretty important to
> understand which messages are coming from different threads, so we
> can correctly interpret the control flow. Pretty much every logging
> framework these days will include some thread identifier by default,
> and IMHO it is reasonable to do this by default in QEMU too.

I have basically never found that I wanted to know in QEMU
tracing which thread was involved, so I would dispute its
importance in the general case.

There's plenty of other things that would be more interesting to
me to log by default: for instance in pretty much all our
tracepoints in devices we don't have any indication of which
device of possibly several instantiated in the system is the
one producing this log output; having an easy way to have the
tracepoints include e.g. the QOM path of the device involved
would be nice.

I don't object to providing optional logging of the host
thread for people and cases where it's useful. But it's
pretty useless to me, and if we always enable by default
all the optional stuff that might be useful to somebody
then the trace lines end up being a mile long and trace log
files can become huge. Why can't we have this be something like
 -d thread-ids,trace:qcrypto*
so people who want it can turn it on and people who don't
want it don't get it?

thanks
-- PMM

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Peter Maydell]

On Wed, 25 Feb 2026 at 16:38, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Feb 19, 2026 at 10:23:36AM +0000, Peter Maydell wrote:
> > Why do we care about the qemu_log output matching the
> > error-report output? The logs are expected to be
> > quite frequent, to only be there if you've explicitly
> > turned them on, and to be usually directed to a log file.
>
> Personally I never use the log-to-file facility. Instead typical use
> case is relying on the trace points 'log' facility as illustrated in
> the commit message example to dump to stderr for simple debugging,
> and having consistent formatting between the logs & errors is useful
> there.

Do you not find that the tracepoint output is often just too big
to be manageable via stderr? (To take a random example, the last
thing I did with tracepoint logging produced a file 196MB large
for a not very long run of the guest.)

-- PMM

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Daniel P. Berrangé]

On Wed, Feb 25, 2026 at 05:43:45PM +0000, Peter Maydell wrote:
> On Wed, 25 Feb 2026 at 16:38, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Feb 19, 2026 at 10:23:36AM +0000, Peter Maydell wrote:
> > > Why do we care about the qemu_log output matching the
> > > error-report output? The logs are expected to be
> > > quite frequent, to only be there if you've explicitly
> > > turned them on, and to be usually directed to a log file.
> >
> > Personally I never use the log-to-file facility. Instead typical use
> > case is relying on the trace points 'log' facility as illustrated in
> > the commit message example to dump to stderr for simple debugging,
> > and having consistent formatting between the logs & errors is useful
> > there.
> 
> Do you not find that the tracepoint output is often just too big
> to be manageable via stderr? (To take a random example, the last
> thing I did with tracepoint logging produced a file 196MB large
> for a not very long run of the guest.)

The things I'm debugging tend to be more focused on interactions
around mgmt (QMP)  changes, rather than guest execution, so the
trace output is pretty manageable, but yeah I can see how you can
easily get huge output.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v6 22/27] util: introduce common helper for error-report & log code

[Markus Armbruster]

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

> On Wed, Feb 18, 2026 at 03:04:23PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > The error-report and log code both have a need to add prefixes
>> > to messages they are printing, with the current example being
>> > a timestamp.
>> >
>> > The format and configuration they use should be consistent, so
>> > providing a common helper will ensure this is always the case.
>> > Initially the helper only emits a timestamp, but future patches
>> > will expand this.
>> >
>> > This takes the liberty of assigning the new file to the same
>> > maintainer as the existing error-report.c file, given it will
>> > be extracting some functionality from the latter.
>> 
>> Fair.
>> 
>> > While vreport() dynamically changes between reporting to the
>> > monitor vs stderr, depending on whether HMP is active or not,
>> > message prefixes are only ever used in the non-HMP case. Thus
>> > the helper API can take a FILE * object and not have to deal
>> > with the monitor at all.
>> >
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  MAINTAINERS            |  2 ++
>> >  include/qemu/message.h | 28 ++++++++++++++++++++++++++++
>> >  util/meson.build       |  1 +
>> >  util/message.c         | 23 +++++++++++++++++++++++
>> >  4 files changed, 54 insertions(+)
>> >  create mode 100644 include/qemu/message.h
>> >  create mode 100644 util/message.c
>
> snip
>
>> > diff --git a/util/message.c b/util/message.c
>> > new file mode 100644
>> > index 0000000000..99a403f9d0
>> > --- /dev/null
>> > +++ b/util/message.c
>> > @@ -0,0 +1,23 @@
>> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> > +
>> > +#include "qemu/osdep.h"
>> > +
>> > +#include "qemu/message.h"
>> > +#include "monitor/monitor.h"
>> 
>> Superfluous #include.
>> 
>> It'll become used in PATCH 26, for qemu_thread_get_name().  Should
>> include qemu/thread.h there instead.
>
> opps, yes.
>
>> > +
>> > +static int message_format;
>> > +
>> > +void qmessage_set_format(int flags)
>> > +{
>> > +    message_format = flags;
>> > +}
>> > +
>> > +void qmessage_context_print(FILE *fp)
>> > +{
>> > +    if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
>> > +        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
>> > +        g_autofree char *timestr = g_date_time_format_iso8601(dt);
>> > +        fputs(timestr, fp);
>> > +        fputc(' ', fp);
>> 
>> The context string is either empty, or it ends with a space, for ease of
>> use.  Okay.
>> 
>> I'd go for
>> 
>>            fprintf(fp, "%s ", timestr);
>
> Previous reviewer comments preferred fputs/c to avoid
> redundant printf string interpolation since qemu_log
> could be used in fairly hot code paths  at times.

Looking at the bright side (for you): you get to pick the reviewer to
side with!

>> > +    }
>> > +}
>> 
>> Alright, everybody's favorite topic: naming.
>> 
>> message.[ch] aren't about messages, but message *prefixes*.  You call
>> them "context" in qmessage_context_print().  I'm fine with "context".
>
> The use of "message" was a somewhat forward looking thing, guessing
> at possible other needs related to message ouput.
>
> In particular I think there's scope for the Location handling APIs to
> be in this file instead of error-report.c.
>
> So one could imagine  qmessage_loc_push/pop  APis later.
>
>> External symbols are prefixed with qmessage_.  I prefer such prefixes to
>> match the filename.
>
> My view is that they do match if you pretend the 'q' is implicit by
> this living inside qemu.git ;-P
>
>> Prefix in util/ overwhelmingly start with qemu_.
>
> Naturally my choice was based on what I've previously done for
> naming in io/, crypto/ and auth/, etc where all the C APIs have
> a leading 'q' to scope them to QEMU, but this is omitted in the
> directory/file names :-)

I like local consistency.

Actually, I'd like global consistency, but that's a lost cause in QEMU.

>> Somewhat long prefixes feel okay here, as these symbols are used only a
>> couple of times.  qemu_message_context_ might be too long, though.
>> 
>> Could use the classic technique of murdering vowels: to qemu_msg_ctxt_.
>> 
>> Thoughts?
>
> IMHO, despite the existing usage in util/, "qemu_" is overkill as a
> naming prefix.

qemu_msg_ is exactly as long as qmessage_.  If one is overkill, so is
the other :)

> A plain 'q' prefix is most liable to clash with "Qt" library functions,
> but we don't consume that in QEMU so largely not neccessary to worry
> about unless perhaps dragged in indirectly via a 3rd party dep
>
> With regards,
> Daniel

Re: [PATCH v6 25/27] util: add support for formatting a program name in messages

[Markus Armbruster]

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

> On Thu, Feb 19, 2026 at 11:08:45AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > The error_report function can include the program name in any
>> > messages it prints. The qemu_log function has no equivalent
>> > behaviour.
>> >
>> > This introduces support for a "program name" in the new
>> > messages API, which will be included by default for all
>> > binaries.
>> >
>> > This change tweaks the output of the error_report function,
>> > adding a space between the program name and the location
>> > info.
>> 
>> Why?
>
> Looking again, arguably that is fixing a pre-existing bug
>
> The current code is:
>
>     if (!monitor_cur() && g_get_prgname()) {
>         error_printf("%s:", g_get_prgname());
>         sep = " ";
>     }
>     switch (cur_loc->kind) {
>     case LOC_CMDLINE:
>         argp = cur_loc->ptr;
>         for (i = 0; i < cur_loc->num; i++) {
>             error_printf("%s%s", sep, argp[i]);
>             sep = " ";
>         }
>         error_printf(": ");
>         break;
>     case LOC_FILE:
>         error_printf("%s:", (const char *)cur_loc->ptr);
>         if (cur_loc->num) {
>             error_printf("%d:", cur_loc->num);
>         }
>         error_printf(" ");
>         break;
>     default:
>         error_printf("%s", sep);
>     }
>
>
> Notice how we honour "sep" for LOC_CMDLINE but ignore
> it for LOC_FILE.

This works exactly as intended.  To illustrate, add

    error_report("test, ignore");

to qdev_device_add(), then:

    $ cat t.cfg
    [device]
      driver = "usb-kbd"
    $ qemu-system-x86_64 -S -display none -machine usb=on -device usb-kbd -readconfig t.cfg -monitor stdio
    QEMU 10.2.50 monitor - type 'help' for more information
    qemu-system-x86_64: -device usb-kbd: test, ignore
    qemu-system-x86_64:t.cfg:2: test, ignore
    (qemu) device_add usb-kbd
    test, ignore

The format

    program:sourcefile:lineno: message

conforms to the GNU Coding Standards at
<https://www.gnu.org/prep/standards/standards.html#Errors>, and matches
common practice.

> With the program name printing moved out into a separate
> function, the space is always printed. No change for
> LOC_CMDLINE, but new whitespace for LOC_FILE.
>
> Should have changed this in a separate patch.
>
>> >       The qemu_log function will gain the program name. This
>> > can be easily seen with the 'log' trace backend, and how it
>> > is now more closely matching error_report output.
>> >
>> > Before:
>> >
>> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>> >   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>> >   qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>> >
>> > After:
>> >
>> >   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish -d 'trace:qcrypto*'
>> >   qemu-system-x86_64: qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5584e13937f0 dir=fish
>> >   qemu-system-x86_64: qcrypto_tls_creds_get_path TLS creds path creds=0x5584e13937f0 filename=ca-cert.pem path=<none>
>> >   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>> >
>> > When adding this the '-msg program-name=on|off' option is
>> > introduced, so that the program name (which is enabled by
>> > default) can be supressed if desired. This could be useful
>> > if '-msg guest-name=on' is being used as a more informative
>> > identifier.
>> 
>> Separate patch?
>
> It is a bit of a borderline decision, but I felt the knob to turn it
> off should be in the same patch that force enables it.

Peter objected to making the logs more verbose by default, and I think
he has a point.  Perhaps having errors and logs share a general format
is useful anyway, and perhaps making it even more configurable is as
well.  I figure we'd need separate configuration for logs and for error
messages.

Let's get to rough consensus before we debate the finer points like this
one.

>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> With regards,
> Daniel

Re: [PATCH v6 00/27] util: sync error_report & qemu_log output more closely

[Markus Armbruster]

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

> This series is a tangent that came out of discussion in
>
>    https://lists.nongnu.org/archive/html/qemu-devel/2025-08/msg00903.html

This was about error messages providing sufficient information both for
users and for developers.

Manos proposed to print a stack backtrace on programming errors and
fatal errors (&error_abort and &error_fatal), enabled at configure time.
He found this useful for debugging a use-after-free.

Daniel pointed out that GDB shows better stack backtraces, and that we
could not enable this feature in distro builds.  He wondered whether
showing just thread names would do.

> In thinking about adding thread info to error_report,

I understand why that can be useful for developers in certain scenarios,
but I feel it results in error reporting that is overly vebose and
unwieldy for users.  I'd prefer optional, default off.

>                                                       I
> came to realize we should likely make qemu_log behave
> consistently with error_report & friends. We already
> honour '-msg timestamp=on', but don't honour 'guest-name=on'
> and also don't include the binary name.

I'm willing to accept a message format common to error reporting and log
if that's useful.  Backed by shared code, obviously.

Why could it be useful, and to whom?

Error messages are both for users and developers.  Both need them to be
clear and detailed enough to figure out what's gone wrong, and what to
do about it.

Developers can use additional detail.  Lots and lots of it at times.  No
excuse for dumping it all on users.

Carefully designed logs can also be for both.  Much of what QEMU can log
is just for developers, and that's *fine*.  Sometimes developers want to
log lots of detail per message, sometimes they don't, to keep the logs
managable, as Peter pointed out.

When the error message needs of users and developers conflict, I put
users first.

When the logging needs of users and developers conflict, I put
developers first.

> As an example of the current state, consider mixing error and
> log output today:

Having the errors in the log can clearly be useful.  The easiest way to
have them in the log is --- wait for it --- logging them.

We don't do that now.  Instead, you can log to stderr, or you can piece
together log file contents and error messages.  The format differences
can be inconvenient either way.

Could we log errors?

> - Default context:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
>                        -d 'trace:qcrypto*'
>   qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55ac6d97f700 dir=fish
>   qcrypto_tls_creds_get_path TLS creds path creds=0x55ac6d97f700 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> - Full context:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
>                        -d 'trace:qcrypto*' \
>                        -msg guest-name=on,timestamp=on \
>                        -name "fish food"
>   2025-08-19T20:14:16.791413Z qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55e9a3458d10 dir=fish
>   2025-08-19T20:14:16.791429Z qcrypto_tls_creds_get_path TLS creds path creds=0x55e9a3458d10 filename=ca-cert.pem path=<none>
>   2025-08-19T20:14:16.791433Z fish food qemu-system-x86_64: Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> And after this series is complete:
>
> - Default context:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
>                       -d 'trace:qcrypto*'
>   qemu-system-x86_64(1184284:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x55a24ad5cb30 dir=fish
>   qemu-system-x86_64(1184284:main): qcrypto_tls_creds_get_path TLS creds path creds=0x55a24ad5cb30 filename=ca-cert.pem path=<none>
>   qemu-system-x86_64(1184284:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> - Full context:
>
>   # qemu-system-x86_64 -object tls-creds-x509,id=t0,dir=fish \
>                       -d 'trace:qcrypto*' \
>                       -msg guest-name=on,timestamp=on \
>                       -name "fish food"
>   2025-08-19T20:12:50.211823Z [fish food] qemu-system-x86_64(1168876:main): qcrypto_tls_creds_x509_load TLS creds x509 load creds=0x5582183d8760 dir=fish
>   2025-08-19T20:12:50.211842Z [fish food] qemu-system-x86_64(1168876:main): qcrypto_tls_creds_get_path TLS creds path creds=0x5582183d8760 filename=ca-cert.pem
> +path=<none>
>   2025-08-19T20:12:50.211846Z [fish food] qemu-system-x86_64(1168876:main): Unable to access credentials fish/ca-cert.pem: No such file or directory
>
> The main things to note:
>
>  * error_report/warn_report/qemu_log share the same
>    output format and -msg applies to both

I fear this is a bad idea.  I apologize for being so late with this.  I
didn't think hard enough until now.

Unified format with separate configuration controls and defaults could
be okay.

>  * -msg debug-threads=on is now unconditionally enabled
>    and thus the param is deprecated & ignored

This part just makes sense.

>  * Thread ID and name are unconditionally enabled
>
>  * Guest name is surrounded in [...] brackets
>
>  * The default output lines are typically 15 chars
>    wider given that we always include the thread
>    ID + name now
>
>  * This takes the liberty of assigning the new file
>    to the existing error-report.c maintainer (Markus)
>    Since splitting it off into message.c instead of
>    putting it all in error-report.c felt slightly
>    nicer.
>
> One thing I didn't tackle is making the location
> info get reported for qemu_log. This is used to
> give context for error messages when parsing some
> CLI args, and could be interesting for log messages
> associated with those same CLI args.

Re: [PATCH v6 00/27] util: sync error_report & qemu_log output more closely

[Daniel P. Berrangé]

On Thu, Feb 26, 2026 at 10:51:01AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:

snip

> >  * -msg debug-threads=on is now unconditionally enabled
> >    and thus the param is deprecated & ignored
> 
> This part just makes sense.

The code for the new message/log output forms doesn't appear until
patch 22. The first 21 patches end up being refactoring & bug fixing,
so I'm inclined to send a pull request for those first 21, and so I
can more easily carry the remaining 6 while we re-consider the ideas.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|