[Qemu-devel] Re: [RFC][PATCH 0/6] trace-state: make the behaviour of "disable" consistent across all backends

[Fabien Chouteau]

On 04/06/2011 01:42 PM, Stefan Hajnoczi wrote:
> On Tue, Apr 5, 2011 at 2:30 PM, Stefan Hajnoczi <address@hidden> wrote:
>> On Mon, Apr 4, 2011 at 10:49 PM, Lluís <address@hidden> wrote:
>>> This patch defines the "disable" trace event state to always use the "nop"
>>> backend.
>>>
>>> As a side-effect, all events are now enabled (without "disable") by 
>>> default, as
>>> all backends (except "stderr") have programmatic support for dynamically
>>> (de)activating each trace event.
>>>
>>> In order to make this true, the "simple" backend now has a "-trace
>>> events=<file>" argument to let the user select which events must be enabled 
>>> from
>>> the very beginning.
>>>
>>> Signed-off-by: Lluís Vilanova <address@hidden>
>>> ---
>>>
>>> Lluís Vilanova (6):
>>>      trace: [ust] fix generation of 'trace.c' on events without args
>>>      trace: generalize the "property" concept in the trace-events file
>>>      trace-state: always use the "nop" backend on events with the "disable" 
>>> keyword
>>>      trace-state: [simple] disable all trace points by default
>>>      trace-state: [simple] add "-trace events" argument to control initial 
>>> state
>>>      trace: enable all events
>>>
>>>
>>>  docs/tracing.txt  |   12 +-
>>>  qemu-config.c     |    5 +
>>>  qemu-options.hx   |   18 ++
>>>  scripts/tracetool |   88 +++++-------
>>>  trace-events      |  385 
>>> ++++++++++++++++++++++++++---------------------------
>>>  vl.c              |   94 ++++++++-----
>>>  6 files changed, 313 insertions(+), 289 deletions(-)
>>
>> Excellent, thanks for implementing this.  I'll review the patches in
>> detail shortly.
> 
> I've left feedback on the individual patches.  This is a nice cleanup,
> thanks for doing this work!
> 
> The stderr backend is impacted - but not severely.  You now need to
> disable all trace events that should not generate output.  Previously
> it was the opposite; you needed to enable all trace events that should
> generate output.
> 
> Adding Prerna (simpletrace) and Fabien (stderr) on CC so they can take a look.

Patches look good to me, it will be very useful to change event selections
without recompiling (I didn't know that trace-events could be enabled/disabled
in the monitor...).

I attach a patch that adds event selection to stderr based on what is done for
the simple backend.

Lluís can you please add it in your patch set?

>From 71fd4ae792aea78691415241be5cec5f2e9afbca Mon Sep 17 00:00:00 2001
From: Fabien Chouteau <address@hidden>
Date: Wed, 6 Apr 2011 16:15:53 +0200
Subject: [PATCH 1/1] Add trace-event selection from monitor in the stderr 
backend


Signed-off-by: Fabien Chouteau <address@hidden>
---
 Makefile.objs     |    5 +++++
 configure         |    3 +++
 monitor.c         |    6 ++++--
 qemu-config.c     |    2 +-
 qemu-options.hx   |    2 +-
 scripts/tracetool |   33 ++++++++++++++++++++++++++++-----
 stderrtrace.c     |   14 ++++++++++++++
 stderrtrace.h     |   13 +++++++++++++
 vl.c              |   10 ++++++++--
 9 files changed, 77 insertions(+), 11 deletions(-)
 create mode 100644 stderrtrace.c
 create mode 100644 stderrtrace.h

diff --git a/Makefile.objs b/Makefile.objs
index c05f5e5..d58565a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -342,6 +342,7 @@ trace-dtrace.o: trace-dtrace.dtrace $(GENERATED_HEADERS)
        $(call quiet-command,dtrace -o $@ -G -s $<, "  GEN trace-dtrace.o")
 
 simpletrace.o: simpletrace.c $(GENERATED_HEADERS)
+stderrtrace.o: stderrtrace.c $(GENERATED_HEADERS)
 
 ifeq ($(TRACE_BACKEND),dtrace)
 trace-obj-y = trace-dtrace.o
@@ -351,6 +352,10 @@ ifeq ($(TRACE_BACKEND),simple)
 trace-obj-y += simpletrace.o
 user-obj-y += qemu-timer-common.o
 endif
+
+ifeq ($(TRACE_BACKEND),stderr)
+trace-obj-y += stderrtrace.o
+endif
 endif
 
 ######################################################################
diff --git a/configure b/configure
index faaed60..d80bb38 100755
--- a/configure
+++ b/configure
@@ -2933,6 +2933,9 @@ echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
 if test "$trace_backend" = "simple"; then
   echo "CONFIG_SIMPLE_TRACE=y" >> $config_host_mak
 fi
+if test "$trace_backend" = "stderr"; then
+  echo "CONFIG_STDERR_TRACE=y" >> $config_host_mak
+fi
 # Set the appropriate trace file.
 if test "$trace_backend" = "simple"; then
   trace_file="\"$trace_file-%u\""
diff --git a/monitor.c b/monitor.c
index f1a08dc..cb695c6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -57,7 +57,7 @@
 #include "json-parser.h"
 #include "osdep.h"
 #include "exec-all.h"
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 #include "trace.h"
 #endif
 #include "ui/qemu-spice.h"
@@ -592,7 +592,7 @@ static void do_help_cmd(Monitor *mon, const QDict *qdict)
     help_cmd(mon, qdict_get_try_str(qdict, "name"));
 }
 
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
 {
     const char *tp_name = qdict_get_str(qdict, "name");
@@ -603,7 +603,9 @@ static void do_change_trace_event_state(Monitor *mon, const 
QDict *qdict)
         monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
     }
 }
+#endif
 
+#ifdef CONFIG_SIMPLE_TRACE
 static void do_trace_file(Monitor *mon, const QDict *qdict)
 {
     const char *op = qdict_get_try_str(qdict, "op");
diff --git a/qemu-config.c b/qemu-config.c
index 0a00081..a524680 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -297,7 +297,7 @@ static QemuOptsList qemu_mon_opts = {
     },
 };
 
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 static QemuOptsList qemu_trace_opts = {
     .name = "trace",
     .implied_opt_name = "file",
diff --git a/qemu-options.hx b/qemu-options.hx
index e7b93b5..13e3d71 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2356,7 +2356,7 @@ Normally QEMU loads a configuration file from 
@var{sysconfdir}/qemu.conf and
 @var{sysconfdir}/address@hidden on startup.  The @code{-nodefconfig}
 option will prevent QEMU from loading these configuration files at startup.
 ETEXI
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 DEF("trace", HAS_ARG, QEMU_OPTION_trace,
     "-trace [file=]<file>[,events=<file>]\n"
     "                specify tracing options\n",
diff --git a/scripts/tracetool b/scripts/tracetool
index e3aec89..d43fbf0 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -241,7 +241,12 @@ linetoh_begin_stderr()
 {
     cat <<EOF
 #include <stdio.h>
+#include "stderrtrace.h"
+
+extern TraceEvent trace_list[];
 EOF
+
+    simple_event_num=0
 }
 
 linetoh_stderr()
@@ -260,29 +265,47 @@ linetoh_stderr()
     cat <<EOF
 static inline void trace_$name($args)
 {
-    fprintf(stderr, "$name $fmt\n" $argnames);
+    if (trace_list[$simple_event_num].state != 0) {
+        fprintf(stderr, "$name $fmt\n" $argnames);
+    }
 }
 EOF
+    simple_event_num=$((simple_event_num + 1))
+
 }
 
 linetoh_end_stderr()
 {
-return
+    cat <<EOF
+#define NR_TRACE_EVENTS $simple_event_num
+EOF
 }
 
 linetoc_begin_stderr()
 {
-return
+    cat <<EOF
+#include "trace.h"
+
+TraceEvent trace_list[] = {
+EOF
+    simple_event_num=0
 }
 
 linetoc_stderr()
 {
-return
+    local name
+    name=$(get_name "$1")
+    cat <<EOF
+{.tp_name = "$name", .state=0},
+EOF
+    simple_event_num=$(($simple_event_num + 1))
 }
 
 linetoc_end_stderr()
 {
-return
+    cat <<EOF
+};
+EOF
 }
 #END OF STDERR
 
diff --git a/stderrtrace.c b/stderrtrace.c
new file mode 100644
index 0000000..7c27203
--- /dev/null
+++ b/stderrtrace.c
@@ -0,0 +1,14 @@
+#include "trace.h"
+
+bool st_change_trace_event_state(const char *name, bool enabled)
+{
+    int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        if (!strcmp(trace_list[i].tp_name, name)) {
+            trace_list[i].state = enabled;
+            return true;
+        }
+    }
+    return false;
+}
diff --git a/stderrtrace.h b/stderrtrace.h
new file mode 100644
index 0000000..c2b148c
--- /dev/null
+++ b/stderrtrace.h
@@ -0,0 +1,13 @@
+#ifndef _STDERRTRACE_H_
+#define _STDERRTRACE_H_
+
+typedef uint64_t TraceEventID;
+
+typedef struct {
+    const char *tp_name;
+    bool state;
+} TraceEvent;
+
+bool st_change_trace_event_state(const char *name, bool enabled);
+
+#endif /* ! _STDERRTRACE_H_ */
diff --git a/vl.c b/vl.c
index 9fb6a41..d9160bf 100644
--- a/vl.c
+++ b/vl.c
@@ -155,8 +155,9 @@ int main(int argc, char **argv)
 
 #include "slirp/libslirp.h"
 
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 #include "trace.h"
-#include "simpletrace.h"
+#endif
 #include "qemu-queue.h"
 #include "cpus.h"
 #include "arch_init.h"
@@ -2761,7 +2762,7 @@ int main(int argc, char **argv, char **envp)
                 }
                 xen_mode = XEN_ATTACH;
                 break;
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
             case QEMU_OPTION_trace:
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 1);
                 if (opts) {
@@ -2815,9 +2816,13 @@ int main(int argc, char **argv, char **envp)
     }
     loc_set_none();
 
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
+
+# if defined(CONFIG_SIMPLE_TRACE)
     if (!st_init(trace_file)) {
         fprintf(stderr, "warning: unable to initialize simple trace 
backend\n");
     }
+# endif
     if (trace_events_file) {
         FILE *trace_events_fp = fopen(trace_events_file, "r");
         if (!trace_events_fp) {
@@ -2844,6 +2849,7 @@ int main(int argc, char **argv, char **envp)
             exit(1);
         }
     }
+#endif
 
     /* If no data_dir is specified then try to find it relative to the
        executable path.  */
-- 
1.7.1


-- 
Fabien Chouteau