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

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

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

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

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

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

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

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

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

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

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

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

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

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

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