From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47095) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCnuq-0005kI-N2 for qemu-devel@nongnu.org; Mon, 10 Feb 2014 05:16:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCnuj-0007GK-NF for qemu-devel@nongnu.org; Mon, 10 Feb 2014 05:16:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCnuj-0007GB-Fp for qemu-devel@nongnu.org; Mon, 10 Feb 2014 05:16:33 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1AAGW2W032251 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 10 Feb 2014 05:16:32 -0500 Date: Mon, 10 Feb 2014 10:16:28 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20140210101627.GF3545@work-vm> References: <1391077232-14649-1-git-send-email-dgilbert@redhat.com> <1391077232-14649-3-git-send-email-dgilbert@redhat.com> <52F745A2.5020101@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52F745A2.5020101@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 2/3] Add 'debug-threads' suboption to --name List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com * 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" > > > > Add flag storage to qemu-thread-* to store the namethreads flag > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > 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 > > #include > > > > +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 Thanks Dave > Thanks > Laszlo > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK