[PULL v2 00/27] Misc patches queue
The following changes since commit 3fb456e9a0e9eef6a71d9b49bfff596a0f0046e9: Merge tag 'pull-request-2026-03-02' of https://gitlab.com/thuth/qemu into staging (2026-03-02 14:01:46 +0000) are available in the Git repository at: https://gitlab.com/berrange/qemu tags/next-pr-pull-request for you to fetch changes up to ca61f91ef9b0d10333881fd0070303ea33cbc72e: util/oslib-posix: increase memprealloc thread count to 32 (2026-03-05 17:40:24 +0000) ---------------------------------------------------------------- * Increase pre-alloc max thread count to 32 * Fix checkpatch warning for new/removed files with --terse * Detect more GPL boilerplate in checkpatch * Fix lean of data in TLS/websock GSource cancellation * Tweak CPU docs for DiamondRapids * Unconditionally enable thread naming * Fix race setting thread naming on Win32 * Add API docs for logging APIs * Fix interleaved error/trace output * Fix missing error prefixes with warn_report * Add detailed error reporting for VNC passworrd changes * Refactoring of error_vprintf & related funcs ---------------------------------------------------------------- Daniel P. Berrangé (25): scripts: detect another GPL license boilerplate variant io: separate freeing of tasks from marking them as complete io: fix cleanup for TLS I/O source data on cancellation io: fix cleanup for websock I/O source data on cancellation docs: simplify DiamondRapids CPU docs 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 Jon Kohler (1): util/oslib-posix: increase memprealloc thread count to 32 Markus Armbruster (1): scripts/checkpatch: Fix MAINTAINERS update warning with --terse audio/jackaudio.c | 28 +++++-- docs/about/deprecated.rst | 7 ++ docs/system/cpu-models-x86.rst.inc | 16 ++-- include/io/task.h | 29 +++++-- include/monitor/monitor.h | 3 - include/qemu/compiler.h | 8 ++ include/qemu/log-for-trace.h | 17 +++- include/qemu/log.h | 39 ++++++++++ include/qemu/thread.h | 3 +- include/ui/console.h | 2 +- include/ui/qemu-spice-module.h | 3 +- io/channel-tls.c | 64 ++++++++++----- io/channel-websock.c | 46 ++++++++--- io/task.c | 8 +- meson.build | 21 +++++ monitor/monitor.c | 51 +++--------- qemu-options.hx | 112 +++++++++++++-------------- rust/util/src/log.rs | 6 ++ scripts/checkpatch.pl | 11 +-- stubs/error-printf.c | 23 ------ stubs/meson.build | 1 - stubs/monitor-core.c | 19 ++++- system/vl.c | 12 +-- tests/functional/generic/test_vnc.py | 4 +- tests/unit/test-io-task.c | 26 +++++++ 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 | 92 +++++++++++++++++----- util/log.c | 50 +++++++----- util/oslib-posix.c | 2 +- util/qemu-thread-posix.c | 89 +++++++++++++++------ util/qemu-thread-win32.c | 87 ++++++++++++++------- 36 files changed, 631 insertions(+), 322 deletions(-) delete mode 100644 stubs/error-printf.c -- 2.53.0
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- scripts/checkpatch.pl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 3a9557417f..45b1153a9c 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -373,8 +373,9 @@ our @LICENSE_BOILERPLATE = ( "under the terms of the GNU Lesser General Public", "Permission is hereby granted, free of charge", "GNU GPL, version 2 or later", - "See the COPYING file" -); + "See the COPYING file", + "terms and conditions of the GNU General Public", + ); our $LICENSE_BOILERPLATE_RE = join("|", @LICENSE_BOILERPLATE); # Load common spelling mistakes and build regular expression list. -- 2.53.0
The original design of QIOTask was intended to simplify lifecycle management by automatically freeing it when the task was marked as complete. This overlooked the fact that when a QIOTask is used in combination with a GSource, there may be times when the source callback is never invoked. This is typically when a GSource is released before any I/O event arrives. In such cases it is not desirable to mark a QIOTask as complete, but it still needs to be freed. To satisfy this, the task must be released manually. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/io/task.h | 29 +++++++++++++++++++++-------- io/channel-tls.c | 4 ++++ io/channel-websock.c | 3 +++ io/task.c | 8 ++++++-- tests/unit/test-io-task.c | 26 ++++++++++++++++++++++++++ 5 files changed, 60 insertions(+), 10 deletions(-) diff --git a/include/io/task.h b/include/io/task.h index 0b5342ee84..98847f5994 100644 --- a/include/io/task.h +++ b/include/io/task.h @@ -96,7 +96,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task, * 1000, * myobject_operation_timer, * task, - * NULL); + * qio_task_free); * } * </programlisting> * </example> @@ -138,9 +138,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task, * the callback func 'myobject_operation_notify' shown * earlier to deal with the results. * - * Once this function returns false, object_unref will be called - * automatically on the task causing it to be released and the - * ref on QMyObject dropped too. + * Once this function returns FALSE, the task will be freed, + * causing it release the ref on QMyObject too. * * The QIOTask module can also be used to perform operations * in a background thread context, while still reporting the @@ -208,8 +207,8 @@ typedef void (*QIOTaskWorker)(QIOTask *task, * 'err' attribute in the task object to determine if * the operation was successful or not. * - * The returned task will be released when qio_task_complete() - * is invoked. + * The returned task must be released by calling + * qio_task_free() when no longer required. * * Returns: the task struct */ @@ -218,6 +217,19 @@ QIOTask *qio_task_new(Object *source, gpointer opaque, GDestroyNotify destroy); +/** + * qio_task_free: + * task: the task object to free + * + * Free the resources associated with the task. Typically + * the qio_task_complete() method will be called immediately + * before this to trigger the task callback, however, it is + * permissible to free the task in the case of cancellation. + * The destroy callback will be used to release the opaque + * data provided to qio_task_new(). + */ +void qio_task_free(QIOTask *task); + /** * qio_task_run_in_thread: * @task: the task struct @@ -268,8 +280,9 @@ void qio_task_wait_thread(QIOTask *task); * qio_task_complete: * @task: the task struct * - * Invoke the completion callback for @task and - * then free its memory. + * Invoke the completion callback for @task. This should typically + * only be invoked once on a task, and then qio_task_free() used + * to free it. */ void qio_task_complete(QIOTask *task); diff --git a/io/channel-tls.c b/io/channel-tls.c index b0cec27cb9..07274c12df 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -170,6 +170,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, trace_qio_channel_tls_handshake_fail(ioc); qio_task_set_error(task, err); qio_task_complete(task); + qio_task_free(task); return; } @@ -183,6 +184,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, trace_qio_channel_tls_credentials_allow(ioc); } qio_task_complete(task); + qio_task_free(task); } else { GIOCondition condition; QIOChannelTLSData *data = g_new0(typeof(*data), 1); @@ -270,11 +272,13 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, trace_qio_channel_tls_bye_fail(ioc); qio_task_set_error(task, err); qio_task_complete(task); + qio_task_free(task); return; } if (status == QCRYPTO_TLS_BYE_COMPLETE) { qio_task_complete(task); + qio_task_free(task); return; } diff --git a/io/channel-websock.c b/io/channel-websock.c index d0929ba232..100faba6b3 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -545,6 +545,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); + qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } @@ -561,6 +562,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_complete(ioc); qio_task_complete(task); } + qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } @@ -588,6 +590,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); + qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } diff --git a/io/task.c b/io/task.c index da79d31782..c3615d2d74 100644 --- a/io/task.c +++ b/io/task.c @@ -70,8 +70,12 @@ QIOTask *qio_task_new(Object *source, return task; } -static void qio_task_free(QIOTask *task) +void qio_task_free(QIOTask *task) { + if (!task) { + return; + } + qemu_mutex_lock(&task->thread_lock); if (task->thread) { if (task->thread->destroy) { @@ -108,6 +112,7 @@ static gboolean qio_task_thread_result(gpointer opaque) trace_qio_task_thread_result(task); qio_task_complete(task); + qio_task_free(task); return FALSE; } @@ -194,7 +199,6 @@ void qio_task_complete(QIOTask *task) { task->func(task, task->opaque); trace_qio_task_complete(task); - qio_task_free(task); } diff --git a/tests/unit/test-io-task.c b/tests/unit/test-io-task.c index 115dba8970..b1c8ecb7ab 100644 --- a/tests/unit/test-io-task.c +++ b/tests/unit/test-io-task.c @@ -73,6 +73,7 @@ static void test_task_complete(void) src = qio_task_get_source(task); qio_task_complete(task); + qio_task_free(task); g_assert(obj == src); @@ -84,6 +85,28 @@ static void test_task_complete(void) } +static void test_task_cancel(void) +{ + QIOTask *task; + Object *obj = object_new(TYPE_DUMMY); + Object *src; + struct TestTaskData data = { NULL, NULL, false }; + + task = qio_task_new(obj, task_callback, &data, NULL); + src = qio_task_get_source(task); + + qio_task_free(task); + + g_assert(obj == src); + + object_unref(obj); + + g_assert(data.source == NULL); + g_assert(data.err == NULL); + g_assert(data.freed == false); +} + + static void task_data_free(gpointer opaque) { struct TestTaskData *data = opaque; @@ -101,6 +124,7 @@ static void test_task_data_free(void) task = qio_task_new(obj, task_callback, &data, task_data_free); qio_task_complete(task); + qio_task_free(task); object_unref(obj); @@ -123,6 +147,7 @@ static void test_task_failure(void) qio_task_set_error(task, err); qio_task_complete(task); + qio_task_free(task); object_unref(obj); @@ -260,6 +285,7 @@ int main(int argc, char **argv) module_call_init(MODULE_INIT_QOM); type_register_static(&dummy_info); g_test_add_func("/crypto/task/complete", test_task_complete); + g_test_add_func("/crypto/task/cancel", test_task_cancel); g_test_add_func("/crypto/task/datafree", test_task_data_free); g_test_add_func("/crypto/task/failure", test_task_failure); g_test_add_func("/crypto/task/thread_complete", test_task_thread_complete); -- 2.53.0
The TLS code will create a GSource for tracking completion of the handshake process, passing a QIOChannelTLSData struct that contains various data items. The data struct is freed by the callback when it completes, which means when a source is cancelled, nothing is free'ing the data struct or its contents. Switch to provide a data free callback to the GSource, which ensures the QIOChannelTLSData struct is always freed even when the main event callback never fires. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3114 Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- io/channel-tls.c | 68 ++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/io/channel-tls.c b/io/channel-tls.c index 07274c12df..940fc3c6d1 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -153,13 +153,32 @@ struct QIOChannelTLSData { }; typedef struct QIOChannelTLSData QIOChannelTLSData; +static void qio_channel_tls_io_data_free(gpointer user_data) +{ + QIOChannelTLSData *data = user_data; + /* + * Usually 'task' will be NULL since the GSource + * callback will either complete the task or pass + * it on to a new GSource. We'll see a non-NULL + * task here only if the GSource was released before + * its callback triggers + */ + if (data->task) { + qio_task_free(data->task); + } + if (data->context) { + g_main_context_unref(data->context); + } + g_free(data); +} + static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc, GIOCondition condition, gpointer user_data); -static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, - QIOTask *task, - GMainContext *context) +static gboolean qio_channel_tls_handshake_task(QIOChannelTLS *ioc, + QIOTask *task, + GMainContext *context) { Error *err = NULL; int status; @@ -170,8 +189,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, trace_qio_channel_tls_handshake_fail(ioc); qio_task_set_error(task, err); qio_task_complete(task); - qio_task_free(task); - return; + return TRUE; } if (status == QCRYPTO_TLS_HANDSHAKE_COMPLETE) { @@ -184,7 +202,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, trace_qio_channel_tls_credentials_allow(ioc); } qio_task_complete(task); - qio_task_free(task); + return TRUE; } else { GIOCondition condition; QIOChannelTLSData *data = g_new0(typeof(*data), 1); @@ -208,8 +226,9 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, condition, qio_channel_tls_handshake_io, data, - NULL, + qio_channel_tls_io_data_free, context); + return FALSE; } } @@ -225,11 +244,9 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc, qio_task_get_source(task)); tioc->hs_ioc_tag = 0; - g_free(data); - qio_channel_tls_handshake_task(tioc, task, context); - - if (context) { - g_main_context_unref(context); + if (!qio_channel_tls_handshake_task(tioc, task, context)) { + /* task is kept by new GSource so must not be released yet */ + data->task = NULL; } return FALSE; @@ -252,14 +269,16 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc, func, opaque, destroy); trace_qio_channel_tls_handshake_start(ioc); - qio_channel_tls_handshake_task(ioc, task, context); + if (qio_channel_tls_handshake_task(ioc, task, context)) { + qio_task_free(task); + } } static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition, gpointer user_data); -static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, - GMainContext *context) +static gboolean qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, + GMainContext *context) { GIOCondition condition; QIOChannelTLSData *data; @@ -272,14 +291,12 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, trace_qio_channel_tls_bye_fail(ioc); qio_task_set_error(task, err); qio_task_complete(task); - qio_task_free(task); - return; + return TRUE; } if (status == QCRYPTO_TLS_BYE_COMPLETE) { qio_task_complete(task); - qio_task_free(task); - return; + return TRUE; } data = g_new0(typeof(*data), 1); @@ -299,7 +316,10 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, trace_qio_channel_tls_bye_pending(ioc, status); ioc->bye_ioc_tag = qio_channel_add_watch_full(ioc->master, condition, qio_channel_tls_bye_io, - data, NULL, context); + data, + qio_channel_tls_io_data_free, + context); + return FALSE; } @@ -312,11 +332,9 @@ static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition, QIOChannelTLS *tioc = QIO_CHANNEL_TLS(qio_task_get_source(task)); tioc->bye_ioc_tag = 0; - g_free(data); - qio_channel_tls_bye_task(tioc, task, context); - - if (context) { - g_main_context_unref(context); + if (!qio_channel_tls_bye_task(tioc, task, context)) { + /* task is kept by new GSource so must not be released yet */ + data->task = NULL; } return FALSE; -- 2.53.0
The websock code will create a GSource for tracking completion of the handshake process, passing a QIOTask which is freed by the callback when it completes, which means when a source is cancelled, nothing is free'ing the task. Switch to provide a data free callback to the GSource, which ensures the QIOTask is always freed even when the main event callback never fires. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3114 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- io/channel-websock.c | 49 +++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/io/channel-websock.c b/io/channel-websock.c index 100faba6b3..9902b014f7 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -526,11 +526,32 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, return 1; } +typedef struct QIOChannelWebsockData { + QIOTask *task; +} QIOChannelWebsockData; + +static void qio_channel_websock_data_free(gpointer user_data) +{ + QIOChannelWebsockData *data = user_data; + /* + * Usually 'task' will be NULL since the GSource + * callback will either complete the task or pass + * it on to a new GSource. We'll see a non-NULL + * task here only if the GSource was released before + * its callback triggers + */ + if (data->task) { + qio_task_free(data->task); + } + g_free(data); +} + static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, GIOCondition condition, gpointer user_data) { - QIOTask *task = user_data; + QIOChannelWebsockData *data = user_data; + QIOTask *task = data->task; QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK( qio_task_get_source(task)); Error *err = NULL; @@ -545,7 +566,6 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); - qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } @@ -562,7 +582,6 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_complete(ioc); qio_task_complete(task); } - qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } @@ -574,7 +593,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, GIOCondition condition, gpointer user_data) { - QIOTask *task = user_data; + QIOChannelWebsockData *data = user_data, *newdata = NULL; + QIOTask *task = data->task; QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK( qio_task_get_source(task)); Error *err = NULL; @@ -590,7 +610,6 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); - qio_task_free(task); wioc->hs_io_tag = 0; return FALSE; } @@ -603,12 +622,14 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, error_propagate(&wioc->io_err, err); trace_qio_channel_websock_handshake_reply(ioc); + newdata = g_new0(QIOChannelWebsockData, 1); + newdata->task = g_steal_pointer(&data->task); wioc->hs_io_tag = qio_channel_add_watch( wioc->master, G_IO_OUT, qio_channel_websock_handshake_send, - task, - NULL); + newdata, + qio_channel_websock_data_free); return FALSE; } @@ -904,12 +925,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc, gpointer opaque, GDestroyNotify destroy) { - QIOTask *task; + QIOChannelWebsockData *data = g_new0(QIOChannelWebsockData, 1); - task = qio_task_new(OBJECT(ioc), - func, - opaque, - destroy); + data->task = qio_task_new(OBJECT(ioc), + func, + opaque, + destroy); trace_qio_channel_websock_handshake_start(ioc); trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN); @@ -917,8 +938,8 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc, ioc->master, G_IO_IN, qio_channel_websock_handshake_io, - task, - NULL); + data, + qio_channel_websock_data_free); } -- 2.53.0
This aligns the first line of the docs with the style used for previous CPU models, and simplifies the text in the remaining docs. Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/system/cpu-models-x86.rst.inc | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/docs/system/cpu-models-x86.rst.inc b/docs/system/cpu-models-x86.rst.inc index 3605d05a8c..126c5a1972 100644 --- a/docs/system/cpu-models-x86.rst.inc +++ b/docs/system/cpu-models-x86.rst.inc @@ -72,17 +72,13 @@ compatibility is required, use the newest CPU model that is compatible across all desired hosts. ``DiamondRapids`` - Intel Xeon Processor. + Intel Xeon Processor (DiamondRapids, 2026) - Diamond Rapids product has a topology which differs from previous Xeon - products. It does not support SMT, but instead features a dual core - module (DCM) architecture. It also has core building blocks (CBB - die - level in CPU topology). The cache hierarchy is organized as follows: - L1 i/d cache is per thread, L2 cache is per DCM, and L3 cache is per - CBB. This cache topology can be emulated for DiamondRapids CPU model - using the smp-cache configuration as shown below: - - Example: + This does not include SMT but allows the module (dual core module + - DCM) and die (core building block - CBB) topology levels. The + cache hierarchy is L1 i/d cache per thread, L2 cache per module, + and L3 cache per die, which can be emulated using the smp-cache + option: :: -- 2.53.0
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 | 112 ++++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index 0da2b4d034..890c4f1d23 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, nitro, 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, nitro, 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. If no audio backend is specified, QEMU will attempt to select a @@ -959,11 +959,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. @@ -982,7 +982,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. @@ -991,7 +991,7 @@ SRST ``in|out.buffer-count=count`` Sets the count of the buffers. -``-audiodev dbus,id=id[,prop[=value][,...]]`` +``-audiodev dbus,id=id[,prop=value[,...]]`` Creates a D-Bus backend. It must be associated with the display (as ``-display dbus,audiodev=id``). (Since 7.0) @@ -1001,7 +1001,7 @@ SRST Number of samples per read/write (default to 480, 10ms at 48kHz) (Since 10.0) -``-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. @@ -1011,7 +1011,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. @@ -1040,7 +1040,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. @@ -1056,7 +1056,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. @@ -1071,7 +1071,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. @@ -1081,7 +1081,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. @@ -1094,13 +1094,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: @@ -1111,21 +1111,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 @@ -2355,19 +2355,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) @@ -2427,7 +2427,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 @@ -2435,24 +2435,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>`` @@ -2466,7 +2466,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>`` @@ -2696,7 +2696,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 @@ -4870,9 +4870,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 @@ -5561,14 +5561,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
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
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 d0ce914d85..576f82a256 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 3e341142a0..38d7b849e0 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
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
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..3be495b8e3 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 thread 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
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 | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/audio/jackaudio.c b/audio/jackaudio.c index f49fd82c45..be6fb378f7 100644 --- a/audio/jackaudio.c +++ b/audio/jackaudio.c @@ -633,18 +633,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 -- 2.53.0
Hi, On 5/3/26 18:47, Daniel P. Berrangé wrote:
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 | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/audio/jackaudio.c b/audio/jackaudio.c index f49fd82c45..be6fb378f7 100644 --- a/audio/jackaudio.c +++ b/audio/jackaudio.c @@ -633,18 +633,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)
I'm getting on macOS: ../../audio/jackaudio.c:654:12: error: unused function 'qjack_thread_creator' [-Werror,-Wunused-function] 654 | static int qjack_thread_creator(jack_native_thread_t *thread, | ^~~~~~~~~~~~~~~~~~~~ No clue why this isn't covered on CI. My local build: Audio backends CoreAudio support : YES PipeWire support : NO JACK support : YES 1.9.22
+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
Fixed adding: -- >8 -- diff --git a/audio/jackaudio.c b/audio/jackaudio.c index be6fb378f72..589aecede9a 100644 --- a/audio/jackaudio.c +++ b/audio/jackaudio.c @@ -712,7 +712,7 @@ static const TypeInfo audio_types[] = { static void __attribute__((__constructor__)) audio_jack_init(void) { qemu_mutex_init(&qjack_shutdown_lock); -#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); ---
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 platforms) 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 subsequent 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
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 df841e12e2..51f04d5008 100644 --- a/meson.build +++ b/meson.build @@ -2856,6 +2856,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 3be495b8e3..bd1c2ad2a5 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
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..1116570790 100644 --- a/rust/util/src/log.rs +++ b/rust/util/src/log.rs @@ -135,6 +135,12 @@ fn drop(&mut self) { /// 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 { ($mask:expr, $fmt:tt $($args:tt)*) => {{ -- 2.53.0
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..66f6dd5eec 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 unsigned int 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
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 66f6dd5eec..7cffbc1bf8 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> Reviewed-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 ee13ecc4a5..ef1c00134f 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -757,7 +757,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) { @@ -920,8 +920,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; @@ -935,26 +937,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 daf5b01d34..8bf14cf9a7 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 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. Reviewed-by: Markus Armbruster <armbru@redhat.com> 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 8bf14cf9a7..952976e964 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
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
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 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 6ae478bacc..fad796a49d 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 d6b7448183..c7f70bd95b 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
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 c7f70bd95b..f666ca4149 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); } } @@ -203,34 +223,35 @@ char *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 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 f666ca4149..6b9e592bc1 100644 --- a/util/error-report.c +++ b/util/error-report.c @@ -226,6 +226,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
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 6b9e592bc1..bbea49a55d 100644 --- a/util/error-report.c +++ b/util/error-report.c @@ -233,6 +233,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(); @@ -260,6 +263,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
From: Markus Armbruster <armbru@redhat.com> We recently improved the MAINTAINERS update warning to show the files that trigger it. Example: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #105: deleted file mode 100644 improved to WARNING: added, moved or deleted file(s): migration/threadinfo.h migration/threadinfo.c Does MAINTAINERS need updating? Unfortunately, this made things worse with --terse, as only the first line of each warning is shown then. WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? became WARNING: added, moved or deleted file(s): Adjust the warning text to WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? migration/threadinfo.h migration/threadinfo.c so we get the exact same warning as we used to with --terse. Fixes: 1d745e6d9635 (scripts/checkpatch: use new hook for MAINTAINERS update check) Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> [DB: fix typo with missing string concat operator] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- scripts/checkpatch.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 45b1153a9c..b77bd69328 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1474,9 +1474,9 @@ sub process_file_list { # If we don't see a MAINTAINERS update, prod the user to check if (int(@maybemaintainers) > 0 && !$sawmaintainers) { - WARN("added, moved or deleted file(s):\n\n " . - join("\n ", @maybemaintainers) . - "\n\nDoes MAINTAINERS need updating?\n"); + WARN("added, moved or deleted file(s)," + . " does MAINTAINERS need updating?\n " + . join("\n ", @maybemaintainers)); } } -- 2.53.0
From: Jon Kohler <jon@nutanix.com> Increase MAX_MEM_PREALLOC_THREAD_COUNT from 16 to 32. This was last touched in 2017 [1] and, since then, physical machine sizes and VMs therein have continue to get even bigger, both on average and on the extremes. For very large VMs, using 16 threads to preallocate memory can be a non-trivial bottleneck during VM start-up and migration. Increasing this limit to 32 threads reduces the time taken for these operations. Test results from quad socket Intel 8490H (4x 60 cores) show a fairly linear gain of 50% with the 2x thread count increase. --------------------------------------------- Idle Guest w/ 2M HugePages | Start-up time --------------------------------------------- 240 vCPU, 7.5TB (16 threads) | 2m41.955s --------------------------------------------- 240 vCPU, 7.5TB (32 threads) | 1m19.404s --------------------------------------------- Note: Going above 32 threads appears to have diminishing returns at the point where the memory bandwidth and context switching costs appear to be a limiting factor to linear scaling. For posterity, on the same system as above: - 32 threads: 1m19s - 48 threads: 1m4s - 64 threads: 59s - 240 threads: 50s Additional thread counts also get less interesting as the amount of memory is to be preallocated is smaller. Putting that all together, 32 threads appears to be a sane number with a solid speedup on fairly modern hardware. To go faster, we'd either need to improve the hardware (CPU/memory) itself or improve clear_pages_*() on the kernel side to be more efficient. [1] 1e356fc14bea ("mem-prealloc: reduce large guest start-up and migration time.") Signed-off-by: Jon Kohler <jon@nutanix.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- util/oslib-posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 3c14b72665..dc001da66d 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -61,7 +61,7 @@ #include "qemu/memalign.h" #include "qemu/mmap-alloc.h" -#define MAX_MEM_PREALLOC_THREAD_COUNT 16 +#define MAX_MEM_PREALLOC_THREAD_COUNT 32 struct MemsetThread; -- 2.53.0
On Thu, 5 Mar 2026 at 17:48, Daniel P. Berrangé <berrange@redhat.com> wrote:
The following changes since commit 3fb456e9a0e9eef6a71d9b49bfff596a0f0046e9:
Merge tag 'pull-request-2026-03-02' of https://gitlab.com/thuth/qemu into staging (2026-03-02 14:01:46 +0000)
are available in the Git repository at:
https://gitlab.com/berrange/qemu tags/next-pr-pull-request
for you to fetch changes up to ca61f91ef9b0d10333881fd0070303ea33cbc72e:
util/oslib-posix: increase memprealloc thread count to 32 (2026-03-05 17:40:24 +0000)
---------------------------------------------------------------- * Increase pre-alloc max thread count to 32 * Fix checkpatch warning for new/removed files with --terse * Detect more GPL boilerplate in checkpatch * Fix lean of data in TLS/websock GSource cancellation * Tweak CPU docs for DiamondRapids * Unconditionally enable thread naming * Fix race setting thread naming on Win32 * Add API docs for logging APIs * Fix interleaved error/trace output * Fix missing error prefixes with warn_report * Add detailed error reporting for VNC passworrd changes * Refactoring of error_vprintf & related funcs
----------------------------------------------------------------
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/11.0 for any user-visible changes. -- PMM
participants (3)
-
Daniel P. Berrangé -
Peter Maydell -
Philippe Mathieu-Daudé