Re: [PATCH 03/12] system: unconditionally enable thread naming

["Dr. David Alan Gilbert" <dave@treblig.org>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> 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.

Ah finally!

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> 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.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  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 d50645a071..bd6f865558 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
> +ignored. Thread naming is unconditionally enabled for all platforms
> +where it is supported.
> +
>  User-mode emulator command line arguments
>  -----------------------------------------
>  
> 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",
>          },
>          { /* 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");
>      }
> +
>      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.50.1
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/