Re: [Qemu-devel] [PATCH v2 2/3] Add 'debug-threads' suboption to --name

[Laszlo Ersek <lersek@redhat.com>]

a few irrelevant comments below:

On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add flag storage to qemu-thread-* to store the namethreads flag
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/qemu/thread.h    | 1 +
>  qemu-options.hx          | 7 +++++--
>  util/qemu-thread-posix.c | 7 +++++++
>  util/qemu-thread-win32.c | 8 ++++++++
>  vl.c                     | 9 +++++++++
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index 3e32c65..bf1e110 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -59,5 +59,6 @@ void *qemu_thread_join(QemuThread *thread);
>  void qemu_thread_get_self(QemuThread *thread);
>  bool qemu_thread_is_self(QemuThread *thread);
>  void qemu_thread_exit(void *retval);
> +void qemu_thread_naming(bool enable);
>  
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 56e5fdf..068da2d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -328,9 +328,11 @@ possible drivers and properties, use @code{-device help} and
>  ETEXI
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,
> -    "-name string1[,process=string2]\n"
> +    "-name string1[,process=string2][,debug-threads=on|off]\n"
>      "                set the name of the guest\n"
> -    "                string1 sets the window title and string2 the process name (on Linux)\n",
> +    "                string1 sets the window title and string2 the process name (on Linux)\n"
> +    "                When debug-threads is enabled, individual threads are given a separate name (on Linux)\n"
> +    "                NOTE: The thread names are for debugging and not a stable API.\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -name @var{name}
> @@ -339,6 +341,7 @@ Sets the @var{name} of the guest.
>  This name will be displayed in the SDL window caption.
>  The @var{name} will also be used for the VNC server.
>  Also optionally set the top visible process name in Linux.
> +Naming of individual threads can also be enabled on Linux to aid debugging.
>  ETEXI
>  
>  DEF("uuid", HAS_ARG, QEMU_OPTION_uuid,
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 37dd298..0fa6c81 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -27,6 +27,13 @@
>  #include "qemu/thread.h"
>  #include "qemu/atomic.h"
>  
> +static bool name_threads;
> +
> +void qemu_thread_naming(bool enable)
> +{
> +    name_threads = enable;
> +}
> +
>  static void error_exit(int err, const char *msg)
>  {
>      fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 27a5217..e42cb77 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -16,6 +16,14 @@
>  #include <assert.h>
>  #include <limits.h>
>  
> +static bool name_threads;
> +
> +void qemu_thread_naming(bool enable)
> +{
> +    /* But note we don't actually name them on Windows yet */
> +    name_threads = enable;
> +}
> +
>  static void error_exit(int err, const char *msg)
>  {
>      char *pstr;
> diff --git a/vl.c b/vl.c
> index 5f993e4..77d6d9e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -547,6 +547,12 @@ static QemuOptsList qemu_name_opts = {
>              .name = "process",
>              .type = QEMU_OPT_STRING,
>              .help = "Sets the name of the QEMU process, as shown in top etc",
> +        }, {
> +            .name = "debug-threads",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "When enabled, name the individual threads; defaults off.\n"

(1) same question about newlines applies

(2) the default setting is usually advertised in qemu-options.hx, not here

(3) the meaning of "default" is relative to the case when the option is
specified and the option argument is not, *not* relative to when the
option is missing. Cf.

(a)  -name debug-threads
(b)  -name debug-threads=on
(c)  [nothing]

The default as explained by the help text (in qemu-options.hx),
independently of option argument type, is usually the difference between
(a) and (b), not (a) and (c), nor between (b) and (c).

Here, due to the way boolean options are parsed in parse_option_bool(),
(a) is identical to (b), so the default is "on". (See -msg timestamp in
qemu-options.hx, it's similar.)

I *think*. But I could never really form a decisive understanding of the
tradition here, so feel free to ignore it.

> +                    "NOTE: The thread names are for debugging and not a\n"
> +                    "stable API.",
>          },
>          { /* End of list */ }
>      },
> @@ -1006,6 +1012,9 @@ static void parse_name(QemuOpts *opts)
>  {
>      const char *proc_name;
>  
> +    if (qemu_opt_get(opts, "debug-threads")) {
> +        qemu_thread_naming(qemu_opt_get_bool(opts, "debug-threads", false));
> +    }

(4) I'm not sure you need the surrounding "if". qemu_opt_get() and
qemu_opt_get_bool() both start with qemu_opt_find().

With the "if", if the option is not passed, then you leave
"name_threads" at its default value (== false).

Without the "if", if the option is not passed, then you overwrite
"name_threads" with the option default (== false). I think the code
would be simpler if you dropped the surrounding "if".

>      qemu_name = qemu_opt_get(opts, "guest");
>  
>      proc_name = qemu_opt_get(opts, "process");
> 

Anyway I shouldn't obsess about this (also because if I do and you
respin, I get to review v3 too :))

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo