Daniel P. Berrangé <[email protected]> writes:
> 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 <[email protected]>
> 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 <[email protected]>
> Reviewed-by: Richard Henderson <[email protected]>
> Signed-off-by: Daniel P. Berrangé <[email protected]>
> ---
> docs/about/deprecated.rst | 7 +++++++
> include/qemu/thread.h | 1 -
> system/vl.c | 11 ++++++-----
> util/qemu-thread-posix.c | 18 +-----------------
> util/qemu-thread-win32.c | 27 ++++++---------------------
> 5 files changed, 20 insertions(+), 44 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index b2420732e1..7187ea15fa 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -81,6 +81,13 @@ kernel since 2001. None of the board types QEMU supports
> need
> ``param_struct`` support, so this option has been deprecated and will
> be removed in a future QEMU version.
>
> +``debug-threads`` option for ``-name``
> +''''''''''''''''''''''''''''''''''''''
> +
> +Thread ``debug-threads`` option for the ``-name`` argument is now
Do you mean "The ``debug-threads`` option"?
"option for" or "option of"? Not a native speaker...
> +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 3b7057e6c6..a64fd90d4a 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -403,9 +403,7 @@ 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 = "DEPRECATED: thread names are always set where
> supported",
Please don't shout :)
We rarely note deprecation notes in command line help texts. I found
two.
block/qcow.c:
{
.name = BLOCK_OPT_ENCRYPT,
.type = QEMU_OPT_BOOL,
.help = "Encrypt the image with format 'aes'. (Deprecated "
"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
},
block/qcow2.c:
{ \
.name = BLOCK_OPT_ENCRYPT, \
.type = QEMU_OPT_BOOL, \
.help = "Encrypt the image with format 'aes'. (Deprecated " \
"in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)", \
}, \
{ \
Suggest something like
.help = "Enable thread names"
" (deprecated, always enabled where supported)",
> },
> { /* End of list */ }
> },
> @@ -554,9 +552,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");
Not sure this is worth the trouble, but since you wrote and tested it
already...
> }
> +
> 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);
Modulo nits:
Reviewed-by: Markus Armbruster <[email protected]>
