[Qemu-devel] [PATCH v2 0/3] Name threads

[Qemu-devel] [PATCH v2 0/3] Name threads

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This series uses pthread_setname_np (when available) to set the names on
threads that QEMU creates to make life easier when debugging.

It's turned off by default (because there were worries that it might break
tools that relied on process names) but is enabled by adding

   debug-threads=on

to the --name option.

Note that the initial thread still has the default name (or the value passed
as the process= parameter to --name).

The naming of the individual threads is not meant to form an API; tools
shall not rely on the names.

The first patch converts --name to use QemuOpts, a side effect of this is that
  --name process=foo,bar
no longer allows a process name of 'foo,bar', since ',' is a separator.

With this enabled in gdb we see:

(gdb) info threads
  Id   Target Id         Frame 
  5    Thread 0x7fb6670f9700 (LWP 18243) "worker" 0x00007fb66b8fcec0 in sem_timedwait () from /lib64/libpthread.so.0
  4    Thread 0x7fb666515700 (LWP 18244) "CPU 0/KVM" 0x00007fb669ab0117 in ioctl () from /lib64/libc.so.6
  3    Thread 0x7fb665d14700 (LWP 18245) "CPU 1/KVM" 0x00007fb669ab0117 in ioctl () from /lib64/libc.so.6
  2    Thread 0x7fb5d7fff700 (LWP 18247) "vnc_worker" 0x00007fb66b8fad20 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
* 1    Thread 0x7fb66d451a00 (LWP 18242) "qemuprocess" 0x00007fb669aaeb4f in ppoll () from /lib64/libc.so.6

and it also shows up well in 'top' using the H and V options.

V1->V2 changes
   Change the name to debug-threads and add more scary warnings to stop
anyone even thinking they should use the thread names as an API.

   Number CPU threads

Dr. David Alan Gilbert (3):
  Rework --name to use QemuOpts
  Add 'debug-threads' suboption to --name
  Add a 'name' parameter to qemu_thread_create

 cpus.c                          | 25 +++++++++++++----
 hw/block/dataplane/virtio-blk.c |  2 +-
 hw/usb/ccid-card-emulated.c     |  8 +++---
 include/qemu/thread.h           |  3 +-
 libcacard/vscclient.c           |  2 +-
 migration.c                     |  2 +-
 qemu-options.hx                 |  7 +++--
 thread-pool.c                   |  2 +-
 ui/vnc-jobs.c                   |  3 +-
 util/compatfd.c                 |  3 +-
 util/qemu-thread-posix.c        | 16 +++++++++--
 util/qemu-thread-win32.c        | 10 ++++++-
 vl.c                            | 61 ++++++++++++++++++++++++++++++++---------
 13 files changed, 110 insertions(+), 34 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 vl.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/vl.c b/vl.c
index 7f4fe0d..5f993e4 100644
--- a/vl.c
+++ b/vl.c
@@ -531,6 +531,27 @@ static QemuOptsList qemu_msg_opts = {
     },
 };
 
+static QemuOptsList qemu_name_opts = {
+    .name = "name",
+    .implied_opt_name = "guest",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_name_opts.head),
+    .desc = {
+        {
+            .name = "guest",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the guest.\n"
+                    "This name will be displayed in the SDL window caption.\n"
+                    "The name will also be used for the VNC server",
+        }, {
+            .name = "process",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the QEMU process, as shown in top etc",
+        },
+        { /* End of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -981,6 +1002,18 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static void parse_name(QemuOpts *opts)
+{
+    const char *proc_name;
+
+    qemu_name = qemu_opt_get(opts, "guest");
+
+    proc_name = qemu_opt_get(opts, "process");
+    if (proc_name) {
+        os_set_proc_name(proc_name);
+    }
+}
+
 bool usb_enabled(bool default_usb)
 {
     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
@@ -2895,6 +2928,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_tpmdev_opts);
     qemu_add_opts(&qemu_realtime_opts);
     qemu_add_opts(&qemu_msg_opts);
+    qemu_add_opts(&qemu_name_opts);
 
     runstate_init();
 
@@ -3630,19 +3664,11 @@ int main(int argc, char **argv, char **envp)
                                 "is no longer supported.\n");
                 break;
             case QEMU_OPTION_name:
-                qemu_name = g_strdup(optarg);
-		 {
-		     char *p = strchr(qemu_name, ',');
-		     if (p != NULL) {
-		        *p++ = 0;
-			if (strncmp(p, "process=", 8)) {
-			    fprintf(stderr, "Unknown subargument %s to -name\n", p);
-			    exit(1);
-			}
-			p += 8;
-			os_set_proc_name(p);
-		     }
-		 }
+                opts = qemu_opts_parse(qemu_find_opts("name"), optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
+                parse_name(opts);
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Laszlo Ersek]

On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  vl.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 7f4fe0d..5f993e4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -531,6 +531,27 @@ static QemuOptsList qemu_msg_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_name_opts = {
> +    .name = "name",
> +    .implied_opt_name = "guest",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_name_opts.head),
> +    .desc = {
> +        {
> +            .name = "guest",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the guest.\n"
> +                    "This name will be displayed in the SDL window caption.\n"
> +                    "The name will also be used for the VNC server",
> +        }, {
> +            .name = "process",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the QEMU process, as shown in top etc",
> +        },
> +        { /* End of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -981,6 +1002,18 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>      return 0;
>  }
>  
> +static void parse_name(QemuOpts *opts)
> +{
> +    const char *proc_name;
> +
> +    qemu_name = qemu_opt_get(opts, "guest");
> +
> +    proc_name = qemu_opt_get(opts, "process");
> +    if (proc_name) {
> +        os_set_proc_name(proc_name);
> +    }
> +}
> +
>  bool usb_enabled(bool default_usb)
>  {
>      return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
> @@ -2895,6 +2928,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_tpmdev_opts);
>      qemu_add_opts(&qemu_realtime_opts);
>      qemu_add_opts(&qemu_msg_opts);
> +    qemu_add_opts(&qemu_name_opts);
>  
>      runstate_init();
>  
> @@ -3630,19 +3664,11 @@ int main(int argc, char **argv, char **envp)
>                                  "is no longer supported.\n");
>                  break;
>              case QEMU_OPTION_name:
> -                qemu_name = g_strdup(optarg);
> -		 {
> -		     char *p = strchr(qemu_name, ',');
> -		     if (p != NULL) {
> -		        *p++ = 0;
> -			if (strncmp(p, "process=", 8)) {
> -			    fprintf(stderr, "Unknown subargument %s to -name\n", p);
> -			    exit(1);
> -			}
> -			p += 8;
> -			os_set_proc_name(p);
> -		     }
> -		 }
> +                opts = qemu_opts_parse(qemu_find_opts("name"), optarg, 1);
> +                if (!opts) {
> +                    exit(1);
> +                }
> +                parse_name(opts);
>                  break;
>              case QEMU_OPTION_prom_env:
>                  if (nb_prom_envs >= MAX_PROM_ENVS) {
> 

I have one question, but it doesn't block my R-b:

Did you test (and if so, how) the new .help text for "guest"? Because it
seems to be the only such text that has newline characters embedded. I
looked around the tree a bit, and it seems that the only way to get
these option texts is the "query-command-line-options" QMP command
(apparently not available via HMP). If that's the case, then the
embedded newlines could / should be dropped. But I don't really care
about those.

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >  vl.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7f4fe0d..5f993e4 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -531,6 +531,27 @@ static QemuOptsList qemu_msg_opts = {
> >      },
> >  };
> >  
> > +static QemuOptsList qemu_name_opts = {
> > +    .name = "name",
> > +    .implied_opt_name = "guest",
> > +    .merge_lists = true,
> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_name_opts.head),
> > +    .desc = {
> > +        {
> > +            .name = "guest",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "Sets the name of the guest.\n"
> > +                    "This name will be displayed in the SDL window caption.\n"
> > +                    "The name will also be used for the VNC server",
> > +        }, {
> > +            .name = "process",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "Sets the name of the QEMU process, as shown in top etc",

<snip>

> I have one question, but it doesn't block my R-b:
> 
> Did you test (and if so, how) the new .help text for "guest"? Because it
> seems to be the only such text that has newline characters embedded. I
> looked around the tree a bit, and it seems that the only way to get
> these option texts is the "query-command-line-options" QMP command
> (apparently not available via HMP). If that's the case, then the
> embedded newlines could / should be dropped. But I don't really care
> about those.

I'd checked every piece of output I'd found, but hadn't found
the query-command-line-options;   I think you're right there
are no other \n's in there - but also all the other .help texts
are much briefer and less chatty; maybe I need to just chop
them down to a minimum, I wonder if there is anywhere they're
ever displayed to a human?

Dave

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Laszlo Ersek]

On 02/10/14 11:03, Dr. David Alan Gilbert wrote:

> I'd checked every piece of output I'd found, but hadn't found
> the query-command-line-options;   I think you're right there
> are no other \n's in there - but also all the other .help texts
> are much briefer and less chatty; maybe I need to just chop
> them down to a minimum, I wonder if there is anywhere they're
> ever displayed to a human?

I share your wondering :)

Laszlo

Re: [Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Paolo Bonzini]

Il 10/02/2014 17:12, Laszlo Ersek ha scritto:
>> > I'd checked every piece of output I'd found, but hadn't found
>> > the query-command-line-options;   I think you're right there
>> > are no other \n's in there - but also all the other .help texts
>> > are much briefer and less chatty; maybe I need to just chop
>> > them down to a minimum, I wonder if there is anywhere they're
>> > ever displayed to a human?
> I share your wondering :)

I do too, but I'd wager the answer is no.

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] Rework --name to use QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On 02/11/2014 02:07 AM, Paolo Bonzini wrote:
> Il 10/02/2014 17:12, Laszlo Ersek ha scritto:
>>> > I'd checked every piece of output I'd found, but hadn't found
>>> > the query-command-line-options;   I think you're right there
>>> > are no other \n's in there - but also all the other .help texts
>>> > are much briefer and less chatty; maybe I need to just chop
>>> > them down to a minimum, I wonder if there is anywhere they're
>>> > ever displayed to a human?
>> I share your wondering :)
> 
> I do too, but I'd wager the answer is no.

Libvirt logs the result of query-command-line-options, including the
human portion associated with the details.  And libvirt also exposes the
'virsh qemu-monitor-command' backdoor which can be used by developers to
get at the human-readable information in this query.  I _have_ called
query-command-line-options to see what it output, and did appreciate the
human text available there.

But you are correct that in the day-to-day operation of libvirtd, the
human text is ignored - it's there mainly for developers.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Dr. David Alan Gilbert (git)]

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"
+                    "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));
+    }
     qemu_name = qemu_opt_get(opts, "guest");
 
     proc_name = qemu_opt_get(opts, "process");
-- 
1.8.5.3

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

[Laszlo Ersek]

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

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

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> 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

I'd assumed this landed in -help output and that looked OK.

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

OK

> (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.

If 'default' is (a) then what is the word for (c) ? What I was really
trying to express was that if you don't pass the option at all it's
not going to name the threads.

> > +                    "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".

Yes, I think you're right, I was trying to guard against what happened
when you passed -name multiple times, but as you say the code already
does the right thing.

> >      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

Dave

> Thanks
> Laszlo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Laszlo Ersek]

On 02/10/14 11:16, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> 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
> 
> I'd assumed this landed in -help output and that looked OK.
> 
>> (2) the default setting is usually advertised in qemu-options.hx, not here
> 
> OK
> 
>> (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.
> 
> If 'default' is (a) then what is the word for (c) ? What I was really
> trying to express was that if you don't pass the option at all it's
> not going to name the threads.

I see. I couldn't name an example where (c) is explicitly referred to.

Thanks
Laszlo

[Qemu-devel] [PATCH v2 3/3] Add a 'name' parameter to qemu_thread_create

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If enabled, set the thread name at creation (on GNU systems with
  pthread_set_np)
Fix up all the callers with a thread name

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 cpus.c                          | 25 ++++++++++++++++++++-----
 hw/block/dataplane/virtio-blk.c |  2 +-
 hw/usb/ccid-card-emulated.c     |  8 ++++----
 include/qemu/thread.h           |  2 +-
 libcacard/vscclient.c           |  2 +-
 migration.c                     |  2 +-
 thread-pool.c                   |  2 +-
 ui/vnc-jobs.c                   |  3 ++-
 util/compatfd.c                 |  3 ++-
 util/qemu-thread-posix.c        |  9 +++++++--
 util/qemu-thread-win32.c        |  2 +-
 11 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/cpus.c b/cpus.c
index ca4c59f..e5bb271 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1117,16 +1117,23 @@ void resume_all_vcpus(void)
     }
 }
 
+/* For temporary buffers for forming a name */
+#define TMP_THREAD_NAME_LEN 16
+
 static void qemu_tcg_init_vcpu(CPUState *cpu)
 {
+    char thread_name[TMP_THREAD_NAME_LEN];
+
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         cpu->thread = g_malloc0(sizeof(QemuThread));
         cpu->halt_cond = g_malloc0(sizeof(QemuCond));
         qemu_cond_init(cpu->halt_cond);
         tcg_halt_cond = cpu->halt_cond;
-        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
-                           QEMU_THREAD_JOINABLE);
+        snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/TCG",
+                 cpu->cpu_index);
+        qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
+                           cpu, QEMU_THREAD_JOINABLE);
 #ifdef _WIN32
         cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
@@ -1142,11 +1149,15 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
 
 static void qemu_kvm_start_vcpu(CPUState *cpu)
 {
+    char thread_name[TMP_THREAD_NAME_LEN];
+
     cpu->thread = g_malloc0(sizeof(QemuThread));
     cpu->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(cpu->halt_cond);
-    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
-                       QEMU_THREAD_JOINABLE);
+    snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/KVM",
+             cpu->cpu_index);
+    qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
+                       cpu, QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
     }
@@ -1154,10 +1165,14 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
 
 static void qemu_dummy_start_vcpu(CPUState *cpu)
 {
+    char thread_name[TMP_THREAD_NAME_LEN];
+
     cpu->thread = g_malloc0(sizeof(QemuThread));
     cpu->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(cpu->halt_cond);
-    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
+    snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/DUMMY",
+             cpu->cpu_index);
+    qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu,
                        QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 456d437..980a684 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
 
     qemu_bh_delete(s->start_bh);
     s->start_bh = NULL;
-    qemu_thread_create(&s->thread, data_plane_thread,
+    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
                        s, QEMU_THREAD_JOINABLE);
 }
 
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index aa913df..7213c89 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
         printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
         return -1;
     }
-    qemu_thread_create(&card->event_thread_id, event_thread, card,
-                       QEMU_THREAD_JOINABLE);
-    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
-                       QEMU_THREAD_JOINABLE);
+    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
+                       card, QEMU_THREAD_JOINABLE);
+    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
+                       card, QEMU_THREAD_JOINABLE);
     return 0;
 }
 
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index bf1e110..f7e3b9b 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
                         void *arg, int mode);
 void *qemu_thread_join(QemuThread *thread);
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 24f7088..3477ab3 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
     send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
     /* launch the event_thread. This will trigger reader adds for all the
      * existing readers */
-    qemu_thread_create(&thread_id, event_thread, NULL, 0);
+    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
     return 0;
 }
 
diff --git a/migration.c b/migration.c
index 7235c23..bddec7e 100644
--- a/migration.c
+++ b/migration.c
@@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
     /* Notify before starting migration thread */
     notifier_list_notify(&migration_state_notifiers, s);
 
-    qemu_thread_create(&s->thread, migration_thread, s,
+    qemu_thread_create(&s->thread, "migration", migration_thread, s,
                        QEMU_THREAD_JOINABLE);
 }
diff --git a/thread-pool.c b/thread-pool.c
index 3735fd3..fbdd3ff 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
     pool->new_threads--;
     pool->pending_threads++;
 
-    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
 }
 
 static void spawn_thread_bh_fn(void *opaque)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 2d3fce8..3f3c47b 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
         return ;
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
+                       QEMU_THREAD_DETACHED);
     queue = q; /* Set global queue */
 }
 
diff --git a/util/compatfd.c b/util/compatfd.c
index 430a41c..341ada6 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     memcpy(&info->mask, mask, sizeof(*mask));
     info->fd = fds[1];
 
-    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
+                       QEMU_THREAD_DETACHED);
 
     return fds[0];
 }
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 0fa6c81..45113b4 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
     }
 }
 
-
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
 {
@@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
     if (err)
         error_exit(err, __func__);
 
+#ifdef _GNU_SOURCE
+    if (name_threads) {
+        pthread_setname_np(thread->thread, name);
+    }
+#endif
+
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
     pthread_attr_destroy(&attr);
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index e42cb77..b9c957b 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-void qemu_thread_create(QemuThread *thread,
+void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void *),
                        void *arg, int mode)
 {
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 3/3] Add a 'name' parameter to qemu_thread_create

[Laszlo Ersek]

comments below

On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If enabled, set the thread name at creation (on GNU systems with
>   pthread_set_np)
> Fix up all the callers with a thread name
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  cpus.c                          | 25 ++++++++++++++++++++-----
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  hw/usb/ccid-card-emulated.c     |  8 ++++----
>  include/qemu/thread.h           |  2 +-
>  libcacard/vscclient.c           |  2 +-
>  migration.c                     |  2 +-
>  thread-pool.c                   |  2 +-
>  ui/vnc-jobs.c                   |  3 ++-
>  util/compatfd.c                 |  3 ++-
>  util/qemu-thread-posix.c        |  9 +++++++--
>  util/qemu-thread-win32.c        |  2 +-
>  11 files changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index ca4c59f..e5bb271 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1117,16 +1117,23 @@ void resume_all_vcpus(void)
>      }
>  }
>  
> +/* For temporary buffers for forming a name */
> +#define TMP_THREAD_NAME_LEN 16

This should be called _SIZE in my opinion, because it seems to include
the trailing NUL character.

Alternatively, as much as I hate glib's presence in qemu, you could
replace each (fixed size buffer, snprintf()) pair with a (pointer,
g_strdup_printf(), g_free()) triplet.

> +
>  static void qemu_tcg_init_vcpu(CPUState *cpu)
>  {
> +    char thread_name[TMP_THREAD_NAME_LEN];
> +
>      /* share a single thread for all cpus with TCG */
>      if (!tcg_cpu_thread) {
>          cpu->thread = g_malloc0(sizeof(QemuThread));
>          cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>          qemu_cond_init(cpu->halt_cond);
>          tcg_halt_cond = cpu->halt_cond;
> -        qemu_thread_create(cpu->thread, qemu_tcg_cpu_thread_fn, cpu,
> -                           QEMU_THREAD_JOINABLE);
> +        snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/TCG",
> +                 cpu->cpu_index);
> +        qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
> +                           cpu, QEMU_THREAD_JOINABLE);
>  #ifdef _WIN32
>          cpu->hThread = qemu_thread_get_handle(cpu->thread);
>  #endif
> @@ -1142,11 +1149,15 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>  
>  static void qemu_kvm_start_vcpu(CPUState *cpu)
>  {
> +    char thread_name[TMP_THREAD_NAME_LEN];
> +
>      cpu->thread = g_malloc0(sizeof(QemuThread));
>      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>      qemu_cond_init(cpu->halt_cond);
> -    qemu_thread_create(cpu->thread, qemu_kvm_cpu_thread_fn, cpu,
> -                       QEMU_THREAD_JOINABLE);
> +    snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/KVM",
> +             cpu->cpu_index);
> +    qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
> +                       cpu, QEMU_THREAD_JOINABLE);
>      while (!cpu->created) {
>          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
>      }
> @@ -1154,10 +1165,14 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
>  
>  static void qemu_dummy_start_vcpu(CPUState *cpu)
>  {
> +    char thread_name[TMP_THREAD_NAME_LEN];
> +
>      cpu->thread = g_malloc0(sizeof(QemuThread));
>      cpu->halt_cond = g_malloc0(sizeof(QemuCond));
>      qemu_cond_init(cpu->halt_cond);
> -    qemu_thread_create(cpu->thread, qemu_dummy_cpu_thread_fn, cpu,
> +    snprintf(thread_name, TMP_THREAD_NAME_LEN, "CPU %d/DUMMY",
> +             cpu->cpu_index);
> +    qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu,
>                         QEMU_THREAD_JOINABLE);
>      while (!cpu->created) {
>          qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 456d437..980a684 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
>  
>      qemu_bh_delete(s->start_bh);
>      s->start_bh = NULL;
> -    qemu_thread_create(&s->thread, data_plane_thread,
> +    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
>                         s, QEMU_THREAD_JOINABLE);
>  }

Don't really want to suggest feature creep, but since we can have
several dataplane threads as well, I suggest appending the serial number
of the virtio-blk device, s->blk->serial (can be NULL). Example for
access is in do_get_id_cmd(). Of course with this free-form string,
g_strdup_printf() becomes more attractive.

>  
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index aa913df..7213c89 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
>          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
>          return -1;
>      }
> -    qemu_thread_create(&card->event_thread_id, event_thread, card,
> -                       QEMU_THREAD_JOINABLE);
> -    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> -                       QEMU_THREAD_JOINABLE);
> +    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> +                       card, QEMU_THREAD_JOINABLE);
> +    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> +                       card, QEMU_THREAD_JOINABLE);
>      return 0;
>  }

No clue how many there can be of these.

>  
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index bf1e110..f7e3b9b 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
>  void qemu_event_wait(QemuEvent *ev);
>  void qemu_event_destroy(QemuEvent *ev);
>  
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                          void *(*start_routine)(void *),
>                          void *arg, int mode);
>  void *qemu_thread_join(QemuThread *thread);
> diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> index 24f7088..3477ab3 100644
> --- a/libcacard/vscclient.c
> +++ b/libcacard/vscclient.c
> @@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
>      send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
>      /* launch the event_thread. This will trigger reader adds for all the
>       * existing readers */
> -    qemu_thread_create(&thread_id, event_thread, NULL, 0);
> +    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
>      return 0;
>  }

Ditto.

>  
> diff --git a/migration.c b/migration.c
> index 7235c23..bddec7e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
>      /* Notify before starting migration thread */
>      notifier_list_notify(&migration_state_notifiers, s);
>  
> -    qemu_thread_create(&s->thread, migration_thread, s,
> +    qemu_thread_create(&s->thread, "migration", migration_thread, s,
>                         QEMU_THREAD_JOINABLE);
>  }

At most one such thread at a time, right?

> diff --git a/thread-pool.c b/thread-pool.c
> index 3735fd3..fbdd3ff 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
>      pool->new_threads--;
>      pool->pending_threads++;
>  
> -    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
>  }

I do think these guys should be numbered somehow. "pending_threads"
looks like an obvious choice, but it might be incorrect.

>  
>  static void spawn_thread_bh_fn(void *opaque)
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index 2d3fce8..3f3c47b 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
>          return ;
>  
>      q = vnc_queue_init();
> -    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> +                       QEMU_THREAD_DETACHED);
>      queue = q; /* Set global queue */
>  }

Probably one thread only.

>  
> diff --git a/util/compatfd.c b/util/compatfd.c
> index 430a41c..341ada6 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>      memcpy(&info->mask, mask, sizeof(*mask));
>      info->fd = fds[1];
>  
> -    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
> +                       QEMU_THREAD_DETACHED);
>  
>      return fds[0];
>  }

Ditto.

> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 0fa6c81..45113b4 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
>      }
>  }
>  
> -
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                         void *(*start_routine)(void*),
>                         void *arg, int mode)
>  {
> @@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
>      if (err)
>          error_exit(err, __func__);
>  
> +#ifdef _GNU_SOURCE
> +    if (name_threads) {
> +        pthread_setname_np(thread->thread, name);
> +    }
> +#endif
> +

Maybe a retval check? Or do we ignore ERANGE on purpose?

>      pthread_sigmask(SIG_SETMASK, &oldset, NULL);
>  
>      pthread_attr_destroy(&attr);
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index e42cb77..b9c957b 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
>      return ret;
>  }
>  
> -void qemu_thread_create(QemuThread *thread,
> +void qemu_thread_create(QemuThread *thread, const char *name,
>                         void *(*start_routine)(void *),
>                         void *arg, int mode)
>  {
> 

Anyway this is for helping debugging, and there's nothing in here that
I'd call a bug, so I won't waste your time with asking for a new version. :)

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

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 3/3] Add a 'name' parameter to qemu_thread_create

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> comments below

Thanks,

> On 01/30/14 11:20, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > If enabled, set the thread name at creation (on GNU systems with
> >   pthread_set_np)
> > Fix up all the callers with a thread name
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  cpus.c                          | 25 ++++++++++++++++++++-----
> >  hw/block/dataplane/virtio-blk.c |  2 +-
> >  hw/usb/ccid-card-emulated.c     |  8 ++++----
> >  include/qemu/thread.h           |  2 +-
> >  libcacard/vscclient.c           |  2 +-
> >  migration.c                     |  2 +-
> >  thread-pool.c                   |  2 +-
> >  ui/vnc-jobs.c                   |  3 ++-
> >  util/compatfd.c                 |  3 ++-
> >  util/qemu-thread-posix.c        |  9 +++++++--
> >  util/qemu-thread-win32.c        |  2 +-
> >  11 files changed, 41 insertions(+), 19 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index ca4c59f..e5bb271 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1117,16 +1117,23 @@ void resume_all_vcpus(void)
> >      }
> >  }
> >  
> > +/* For temporary buffers for forming a name */
> > +#define TMP_THREAD_NAME_LEN 16
> 
> This should be called _SIZE in my opinion, because it seems to include
> the trailing NUL character.

OK, that's fair enough.

> Alternatively, as much as I hate glib's presence in qemu, you could
> replace each (fixed size buffer, snprintf()) pair with a (pointer,
> g_strdup_printf(), g_free()) triplet.

It seemed to be getting over kill for a name/number pair.

<snip>

> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index 456d437..980a684 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -358,7 +358,7 @@ static void start_data_plane_bh(void *opaque)
> >  
> >      qemu_bh_delete(s->start_bh);
> >      s->start_bh = NULL;
> > -    qemu_thread_create(&s->thread, data_plane_thread,
> > +    qemu_thread_create(&s->thread, "data_plane", data_plane_thread,
> >                         s, QEMU_THREAD_JOINABLE);
> >  }
> 
> Don't really want to suggest feature creep, but since we can have
> several dataplane threads as well, I suggest appending the serial number
> of the virtio-blk device, s->blk->serial (can be NULL). Example for
> access is in do_get_id_cmd(). Of course with this free-form string,
> g_strdup_printf() becomes more attractive.

Right, however, pthread_setname_np's manpage says:
    'The thread name is a meaningful C language string, whose length is
     restricted to 16 characters, including the terminating null byte ('\0')'

so we're a bit tight for space (and hence the magical 16 char length above -
they're doesn't seem to be a documented macro for that len),
so while data_plane 'n' might be a good idea, starting putting serial numbers
etc in ain't going to work.

I also thought the best bet was to leave how to use the naming down
to the maintainers of the threads themselves and I'd just take a first punt
at sane names.


> > diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> > index aa913df..7213c89 100644
> > --- a/hw/usb/ccid-card-emulated.c
> > +++ b/hw/usb/ccid-card-emulated.c
> > @@ -546,10 +546,10 @@ static int emulated_initfn(CCIDCardState *base)
> >          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
> >          return -1;
> >      }
> > -    qemu_thread_create(&card->event_thread_id, event_thread, card,
> > -                       QEMU_THREAD_JOINABLE);
> > -    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> > -                       QEMU_THREAD_JOINABLE);
> > +    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
> > +                       card, QEMU_THREAD_JOINABLE);
> > +    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
> > +                       card, QEMU_THREAD_JOINABLE);
> >      return 0;
> >  }
> 
> No clue how many there can be of these.
> 
> >  
> > diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> > index bf1e110..f7e3b9b 100644
> > --- a/include/qemu/thread.h
> > +++ b/include/qemu/thread.h
> > @@ -52,7 +52,7 @@ void qemu_event_reset(QemuEvent *ev);
> >  void qemu_event_wait(QemuEvent *ev);
> >  void qemu_event_destroy(QemuEvent *ev);
> >  
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                          void *(*start_routine)(void *),
> >                          void *arg, int mode);
> >  void *qemu_thread_join(QemuThread *thread);
> > diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> > index 24f7088..3477ab3 100644
> > --- a/libcacard/vscclient.c
> > +++ b/libcacard/vscclient.c
> > @@ -269,7 +269,7 @@ on_host_init(VSCMsgHeader *mhHeader, VSCMsgInit *incoming)
> >      send_msg(VSC_ReaderRemove, VSCARD_MINIMAL_READER_ID, NULL, 0);
> >      /* launch the event_thread. This will trigger reader adds for all the
> >       * existing readers */
> > -    qemu_thread_create(&thread_id, event_thread, NULL, 0);
> > +    qemu_thread_create(&thread_id, "vsc/event", event_thread, NULL, 0);
> >      return 0;
> >  }
> 
> Ditto.
> 
> >  
> > diff --git a/migration.c b/migration.c
> > index 7235c23..bddec7e 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -679,6 +679,6 @@ void migrate_fd_connect(MigrationState *s)
> >      /* Notify before starting migration thread */
> >      notifier_list_notify(&migration_state_notifiers, s);
> >  
> > -    qemu_thread_create(&s->thread, migration_thread, s,
> > +    qemu_thread_create(&s->thread, "migration", migration_thread, s,
> >                         QEMU_THREAD_JOINABLE);
> >  }
> 
> At most one such thread at a time, right?

Right (for the moment...)

> > diff --git a/thread-pool.c b/thread-pool.c
> > index 3735fd3..fbdd3ff 100644
> > --- a/thread-pool.c
> > +++ b/thread-pool.c
> > @@ -140,7 +140,7 @@ static void do_spawn_thread(ThreadPool *pool)
> >      pool->new_threads--;
> >      pool->pending_threads++;
> >  
> > -    qemu_thread_create(&t, worker_thread, pool, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
> >  }
> 
> I do think these guys should be numbered somehow. "pending_threads"
> looks like an obvious choice, but it might be incorrect.
> 
> >  
> >  static void spawn_thread_bh_fn(void *opaque)
> > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > index 2d3fce8..3f3c47b 100644
> > --- a/ui/vnc-jobs.c
> > +++ b/ui/vnc-jobs.c
> > @@ -333,7 +333,8 @@ void vnc_start_worker_thread(void)
> >          return ;
> >  
> >      q = vnc_queue_init();
> > -    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> > +                       QEMU_THREAD_DETACHED);
> >      queue = q; /* Set global queue */
> >  }
> 
> Probably one thread only.
> 
> >  
> > diff --git a/util/compatfd.c b/util/compatfd.c
> > index 430a41c..341ada6 100644
> > --- a/util/compatfd.c
> > +++ b/util/compatfd.c
> > @@ -88,7 +88,8 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> >      memcpy(&info->mask, mask, sizeof(*mask));
> >      info->fd = fds[1];
> >  
> > -    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
> > +    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
> > +                       QEMU_THREAD_DETACHED);
> >  
> >      return fds[0];
> >  }
> 
> Ditto.
> 
> > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > index 0fa6c81..45113b4 100644
> > --- a/util/qemu-thread-posix.c
> > +++ b/util/qemu-thread-posix.c
> > @@ -394,8 +394,7 @@ void qemu_event_wait(QemuEvent *ev)
> >      }
> >  }
> >  
> > -
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                         void *(*start_routine)(void*),
> >                         void *arg, int mode)
> >  {
> > @@ -421,6 +420,12 @@ void qemu_thread_create(QemuThread *thread,
> >      if (err)
> >          error_exit(err, __func__);
> >  
> > +#ifdef _GNU_SOURCE
> > +    if (name_threads) {
> > +        pthread_setname_np(thread->thread, name);
> > +    }
> > +#endif
> > +
> 
> Maybe a retval check? Or do we ignore ERANGE on purpose?

I decided it was best for QEMU not to die just because we can't get some debug
facility.

> >      pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> >  
> >      pthread_attr_destroy(&attr);
> > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > index e42cb77..b9c957b 100644
> > --- a/util/qemu-thread-win32.c
> > +++ b/util/qemu-thread-win32.c
> > @@ -333,7 +333,7 @@ void *qemu_thread_join(QemuThread *thread)
> >      return ret;
> >  }
> >  
> > -void qemu_thread_create(QemuThread *thread,
> > +void qemu_thread_create(QemuThread *thread, const char *name,
> >                         void *(*start_routine)(void *),
> >                         void *arg, int mode)
> >  {
> > 
> 
> Anyway this is for helping debugging, and there's nothing in here that
> I'd call a bug, so I won't waste your time with asking for a new version. :)
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!

Thanks!

> Laszlo

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 0/3] Name threads

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On 01/30/2014 03:20 AM, Dr. David Alan Gilbert (git) wrote:

> The first patch converts --name to use QemuOpts, a side effect of this is that
>   --name process=foo,bar
> no longer allows a process name of 'foo,bar', since ',' is a separator.

Just to make sure I understand correctly, am I right that if you want a
process name of 'foo,bar' you can still invoke --name process=foo,,bar ?

Libvirt will probably not be using this option, since it is not intended
to be a stable API, but I can definitely see the value of it for
debugging :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] Name threads

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 01/30/2014 03:20 AM, Dr. David Alan Gilbert (git) wrote:
> 
> > The first patch converts --name to use QemuOpts, a side effect of this is that
> >   --name process=foo,bar
> > no longer allows a process name of 'foo,bar', since ',' is a separator.
> 
> Just to make sure I understand correctly, am I right that if you want a
> process name of 'foo,bar' you can still invoke --name process=foo,,bar ?

Much to my surprise, apparently so:

dgilbert 18228 18104 37 13:01 pts/12   00:00:03 ./bin/qemu-system-x86_64 --name process=foo,,bar
[dgilbert@dgilbert-t530 try-thread]$ cat /proc/18228/comm 
foo,bar

(I hadn't realised QemuOpts did that)

> Libvirt will probably not be using this option, since it is not intended
> to be a stable API, but I can definitely see the value of it for
> debugging :)

Great :-)

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK