[Qemu-devel] [PATCH 0/3] trace: enable tracing in qemu-io/qemu-nbd

[Qemu-devel] [PATCH 0/3] trace: enable tracing in qemu-io/qemu-nbd

[Denis V. Lunev]

Actually this is a rework of the original patch, set as a part of write-zeroes
patchset. Moving it out to process via trace tree.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>

Denis V. Lunev (3):
  trace: move qemu_trace_opts to trace/control.c
  trace: enable tracing in qemu-io
  trace: enable tracing in qemu-nbd

 qemu-io.c       | 13 ++++++++++---
 qemu-nbd.c      | 15 +++++++++++++++
 trace/control.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 trace/control.h | 24 +++++++++++++-----------
 vl.c            | 37 +------------------------------------
 5 files changed, 82 insertions(+), 51 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] trace: move qemu_trace_opts to trace/control.c

[Denis V. Lunev]

The patch also creates trace_opt_parse() helper in trace/control.c to reuse
this code in next patches for qemu-nbd and qemu-io.

The patch also makes trace_init_events() static, as this call is not used
outside the module anymore.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 trace/control.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 trace/control.h | 24 +++++++++++++-----------
 vl.c            | 37 +------------------------------------
 3 files changed, 57 insertions(+), 48 deletions(-)

diff --git a/trace/control.c b/trace/control.c
index d099f73..75fc731 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -20,11 +20,33 @@
 #include "qemu/log.h"
 #endif
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "monitor/monitor.h"
 
 int trace_events_enabled_count;
 bool trace_events_dstate[TRACE_EVENT_COUNT];
 
+QemuOptsList qemu_trace_opts = {
+    .name = "trace",
+    .implied_opt_name = "enable",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
+    .desc = {
+        {
+            .name = "enable",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "events",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
+
 TraceEvent *trace_event_name(const char *name)
 {
     assert(name != NULL);
@@ -141,7 +163,7 @@ void trace_enable_events(const char *line_buf)
     }
 }
 
-void trace_init_events(const char *fname)
+static void trace_init_events(const char *fname)
 {
     Location loc;
     FILE *fp;
@@ -216,3 +238,23 @@ bool trace_init_backends(void)
 
     return true;
 }
+
+char *trace_opt_parse(const char *optarg, char *trace_file)
+{
+    QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("trace"),
+                                             optarg, true);
+    if (!opts) {
+        exit(1);
+    }
+    if (qemu_opt_get(opts, "enable")) {
+        trace_enable_events(qemu_opt_get(opts, "enable"));
+    }
+    trace_init_events(qemu_opt_get(opts, "events"));
+    if (trace_file) {
+        g_free(trace_file);
+    }
+    trace_file = g_strdup(qemu_opt_get(opts, "file"));
+    qemu_opts_del(opts);
+
+    return trace_file;
+}
diff --git a/trace/control.h b/trace/control.h
index e2ba6d4..942f9f5 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -160,17 +160,6 @@ static void trace_event_set_state_dynamic(TraceEvent *ev, bool state);
 bool trace_init_backends(void);
 
 /**
- * trace_init_events:
- * @events: Name of file with events to be enabled at startup; may be NULL.
- *          Corresponds to commandline option "-trace events=...".
- *
- * Read the list of enabled tracing events.
- *
- * Returns: Whether the backends could be successfully initialized.
- */
-void trace_init_events(const char *file);
-
-/**
  * trace_init_file:
  * @file:   Name of trace output file; may be NULL.
  *          Corresponds to commandline option "-trace file=...".
@@ -197,6 +186,19 @@ void trace_list_events(void);
  */
 void trace_enable_events(const char *line_buf);
 
+/**
+ * Definition of QEMU options describing trace subsystem configuration
+ */
+extern QemuOptsList qemu_trace_opts;
+
+/**
+ * trace_opt_parse:
+ * @optarg: A string argument of --trace command line argument
+ * @trace_file: current filename to save traces to
+ *
+ * Initialize tracing subsystem.
+ */
+char *trace_opt_parse(const char *optarg, char *trace_file);
 
 #include "trace/control-internal.h"
 
diff --git a/vl.c b/vl.c
index 5fd22cb..52e7b53 100644
--- a/vl.c
+++ b/vl.c
@@ -263,26 +263,6 @@ static QemuOptsList qemu_sandbox_opts = {
     },
 };
 
-static QemuOptsList qemu_trace_opts = {
-    .name = "trace",
-    .implied_opt_name = "enable",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
-    .desc = {
-        {
-            .name = "enable",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "events",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "file",
-            .type = QEMU_OPT_STRING,
-        },
-        { /* end of list */ }
-    },
-};
-
 static QemuOptsList qemu_option_rom_opts = {
     .name = "option-rom",
     .implied_opt_name = "romfile",
@@ -3902,23 +3882,8 @@ int main(int argc, char **argv, char **envp)
                 xen_mode = XEN_ATTACH;
                 break;
             case QEMU_OPTION_trace:
-            {
-                opts = qemu_opts_parse_noisily(qemu_find_opts("trace"),
-                                               optarg, true);
-                if (!opts) {
-                    exit(1);
-                }
-                if (qemu_opt_get(opts, "enable")) {
-                    trace_enable_events(qemu_opt_get(opts, "enable"));
-                }
-                trace_init_events(qemu_opt_get(opts, "events"));
-                if (trace_file) {
-                    g_free(trace_file);
-                }
-                trace_file = g_strdup(qemu_opt_get(opts, "file"));
-                qemu_opts_del(opts);
+                trace_file = trace_opt_parse(optarg, trace_file);
                 break;
-            }
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] trace: enable tracing in qemu-io

[Denis V. Lunev]

Moving trace_init_backends() into trace_opt_parse() is not possible. This
should be called after daemonize() in vl.c.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 5ef3ef7..1910fe3 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -18,6 +18,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/readline.h"
+#include "qemu/log.h"
 #include "qapi/qmp/qstring.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
@@ -458,6 +459,7 @@ int main(int argc, char **argv)
     Error *local_error = NULL;
     QDict *opts = NULL;
     const char *format = NULL;
+    char *trace_file = NULL;
 
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
@@ -473,6 +475,7 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_object_opts);
+    qemu_add_opts(&qemu_trace_opts);
     bdrv_init();
 
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
@@ -512,9 +515,7 @@ int main(int argc, char **argv)
             }
             break;
         case 'T':
-            if (!trace_init_backends()) {
-                exit(1); /* error message will have been printed */
-            }
+            trace_file = trace_opt_parse(optarg, trace_file);
             break;
         case 'V':
             printf("%s version %s\n", progname, QEMU_VERSION);
@@ -560,6 +561,12 @@ int main(int argc, char **argv)
         exit(1);
     }
 
+    if (!trace_init_backends()) {
+        exit(1);
+    }
+    trace_init_file(trace_file);
+    qemu_set_log(LOG_TRACE);
+
     /* initialize commands */
     qemuio_add_command(&quit_cmd);
     qemuio_add_command(&open_cmd);
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] trace: enable tracing in qemu-nbd

[Denis V. Lunev]

Pls note, trace_init_backends() must be called in the final process, i.e.
after daemonization. This is necessary to keep tracing thread in the
proper process.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 qemu-nbd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3e54113..8e59098 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -26,12 +26,14 @@
 #include "qemu/main-loop.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
+#include "qemu/log.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
 #include "qom/object_interfaces.h"
 #include "io/channel-socket.h"
 #include "crypto/init.h"
+#include "trace/control.h"
 
 #include <getopt.h>
 #include <libgen.h>
@@ -87,6 +89,7 @@ static void usage(const char *name)
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
 "                            passwords and/or encryption keys\n"
+"  -T, --trace FILE          enable trace events listed in the given file\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -497,6 +500,7 @@ int main(int argc, char **argv)
         { "export-name", required_argument, NULL, 'x' },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
+        { "trace", required_argument, NULL, 'T' },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -517,6 +521,7 @@ int main(int argc, char **argv)
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
+    char *trace_file = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -533,6 +538,7 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_object_opts);
+    qemu_add_opts(&qemu_trace_opts);
     qemu_init_exec_dir(argv[0]);
 
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
@@ -705,6 +711,9 @@ int main(int argc, char **argv)
         case QEMU_NBD_OPT_IMAGE_OPTS:
             imageOpts = true;
             break;
+        case 'T':
+            trace_file = trace_opt_parse(optarg, trace_file);
+            break;
         }
     }
 
@@ -720,6 +729,12 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    if (!trace_init_backends()) {
+        exit(1);
+    }
+    trace_init_file(trace_file);
+    qemu_set_log(LOG_TRACE);
+
     if (tlscredsid) {
         if (sockpath) {
             error_report("TLS is only supported with IPv4/IPv6");
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/3] trace: enable tracing in qemu-io/qemu-nbd

[Paolo Bonzini]

On 17/05/2016 10:20, Denis V. Lunev wrote:
> Actually this is a rework of the original patch, set as a part of write-zeroes
> patchset. Moving it out to process via trace tree.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Nitpick: the subject for patch 1 could be "trace: move QemuOpts
processing from vl.c to trace/control.c".  Can be fixed on commit.

Paolo

Re: [Qemu-devel] [PATCH 1/3] trace: move qemu_trace_opts to trace/control.c

[Eric Blake]

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

On 05/17/2016 02:20 AM, Denis V. Lunev wrote:
> The patch also creates trace_opt_parse() helper in trace/control.c to reuse
> this code in next patches for qemu-nbd and qemu-io.
> 
> The patch also makes trace_init_events() static, as this call is not used
> outside the module anymore.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  trace/control.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  trace/control.h | 24 +++++++++++++-----------
>  vl.c            | 37 +------------------------------------
>  3 files changed, 57 insertions(+), 48 deletions(-)
> 
>  
> +QemuOptsList qemu_trace_opts = {
> +    .name = "trace",
> +    .implied_opt_name = "enable",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
> +    .desc = {
> +        {
> +            .name = "enable",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "events",
> +            .type = QEMU_OPT_STRING,
> +        },{

I know this is straight code motion, but we aren't very consistent on
spacing between elements.  Might be worth the extra newline here while
touching it?

> +            .name = "file",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 2/3] trace: enable tracing in qemu-io

[Eric Blake]

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

On 05/17/2016 02:20 AM, Denis V. Lunev wrote:
> Moving trace_init_backends() into trace_opt_parse() is not possible. This
> should be called after daemonize() in vl.c.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Please add documentation of the new syntax of the -T option, so that it
shows up in --help output (I'd ask for it in the man page, too, except
qemu-io doesn't seem to have one).

With just this patch applied, I still see in 'qemu-io --help':

  -T, --trace FILE     enable trace events listed in the given file

which doesn't tell me how to turn on particular trace events, vs. the
more useful details in 'qemu-system-x86_64 --help':

-trace [[enable=]<pattern>][,events=<file>][,file=<file>]
                specify tracing options

It is okay to just say "see qemu(1) man page for full description" (the
way we did for the --object option), rather than copy-and-paste a
paragraph that might go out of sync over time.

-- 
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 3/3] trace: enable tracing in qemu-nbd

[Eric Blake]

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

On 05/17/2016 02:20 AM, Denis V. Lunev wrote:
> Pls note, trace_init_backends() must be called in the final process, i.e.

s/Pls/Please/

> after daemonization. This is necessary to keep tracing thread in the
> proper process.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-nbd.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Same comments as on 2/3 - missing documentation - this time, it is the
man page that is incomplete.

> @@ -87,6 +89,7 @@ static void usage(const char *name)
>  "General purpose options:\n"
>  "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
>  "                            passwords and/or encryption keys\n"
> +"  -T, --trace FILE          enable trace events listed in the given file\n"

Insufficient, compared to how qemu's help text reads.

But overall a cool concept.  Are there any plans to convert some of the
TRACE() debugging printfs in nbd/* into actual trace points?

-- 
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 0/3] trace: enable tracing in qemu-io/qemu-nbd

[Stefan Hajnoczi]

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

On Tue, May 17, 2016 at 11:20:28AM +0300, Denis V. Lunev wrote:
> Actually this is a rework of the original patch, set as a part of write-zeroes
> patchset. Moving it out to process via trace tree.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> 
> Denis V. Lunev (3):
>   trace: move qemu_trace_opts to trace/control.c
>   trace: enable tracing in qemu-io
>   trace: enable tracing in qemu-nbd
> 
>  qemu-io.c       | 13 ++++++++++---
>  qemu-nbd.c      | 15 +++++++++++++++
>  trace/control.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  trace/control.h | 24 +++++++++++++-----------
>  vl.c            | 37 +------------------------------------
>  5 files changed, 82 insertions(+), 51 deletions(-)

Looks useful.  I'd like to apply the v2 when you send it.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] trace: enable tracing in qemu-io/qemu-nbd

[Denis V. Lunev]

On 05/27/2016 11:39 PM, Stefan Hajnoczi wrote:
> On Tue, May 17, 2016 at 11:20:28AM +0300, Denis V. Lunev wrote:
>> Actually this is a rework of the original patch, set as a part of write-zeroes
>> patchset. Moving it out to process via trace tree.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>>
>> Denis V. Lunev (3):
>>    trace: move qemu_trace_opts to trace/control.c
>>    trace: enable tracing in qemu-io
>>    trace: enable tracing in qemu-nbd
>>
>>   qemu-io.c       | 13 ++++++++++---
>>   qemu-nbd.c      | 15 +++++++++++++++
>>   trace/control.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>   trace/control.h | 24 +++++++++++++-----------
>>   vl.c            | 37 +------------------------------------
>>   5 files changed, 82 insertions(+), 51 deletions(-)
> Looks useful.  I'd like to apply the v2 when you send it.
>
> Stefan
yep. I'll do that tomorrow or on monday.

Re: [Qemu-devel] [PATCH 3/3] trace: enable tracing in qemu-nbd

[Denis V. Lunev]

On 05/20/2016 02:26 AM, Eric Blake wrote:
> On 05/17/2016 02:20 AM, Denis V. Lunev wrote:
>> Pls note, trace_init_backends() must be called in the final process, i.e.
> s/Pls/Please/
>
>> after daemonization. This is necessary to keep tracing thread in the
>> proper process.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   qemu-nbd.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
> Same comments as on 2/3 - missing documentation - this time, it is the
> man page that is incomplete.
>
>> @@ -87,6 +89,7 @@ static void usage(const char *name)
>>   "General purpose options:\n"
>>   "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
>>   "                            passwords and/or encryption keys\n"
>> +"  -T, --trace FILE          enable trace events listed in the given file\n"
> Insufficient, compared to how qemu's help text reads.
>
> But overall a cool concept.  Are there any plans to convert some of the
> TRACE() debugging printfs in nbd/* into actual trace points?
>
hmm, interesting. Will think on this.