diff mbox

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

Message ID 4D9C791E.20505@adacore.com
State New
Headers show

Commit Message

Fabien Chouteau April 6, 2011, 2:30 p.m. UTC 
On 04/06/2011 01:42 PM, Stefan Hajnoczi wrote:
> On Tue, Apr 5, 2011 at 2:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Apr 4, 2011 at 10:49 PM, Lluís <xscript@gmx.net> 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 <vilanova@ac.upc.edu>
>>> ---
>>>
>>> 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 <chouteau@adacore.com>
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 <chouteau@adacore.com>
---
 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 mbox

Patch

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}/target-@var{ARCH}.conf 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.  */