* Laszlo Ersek (ler...@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" <dgilb...@redhat.com>
> >
> > Add flag storage to qemu-thread-* to store the namethreads flag
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@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 <ler...@redhat.com>
Thanks
Dave
> Thanks
> Laszlo
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
