diff mbox

[v2] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

Message ID 20140820153431.15111.3916.stgit@fimbulvetr.bsc.es
State New
Headers show

Commit Message

Lluís Vilanova Aug. 20, 2014, 3:34 p.m. UTC
Also removes old "trace-event", "trace-file" and "info trace-events" HMP
commands.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 hmp-commands.hx     |   35 -----------------------
 monitor.c           |   61 ----------------------------------------
 qapi-schema.json    |    3 ++
 qmp-commands.hx     |   27 ++++++++++++++++++
 trace/Makefile.objs |    1 +
 trace/commands.json |   44 +++++++++++++++++++++++++++++
 trace/control.c     |   13 ---------
 trace/control.h     |    7 -----
 trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 trace/simple.c      |   10 +------
 trace/simple.h      |    3 --
 11 files changed, 154 insertions(+), 127 deletions(-)
 create mode 100644 trace/commands.json
 create mode 100644 trace/qmp.c

Comments

Markus Armbruster Aug. 20, 2014, 4:47 p.m. UTC | #1
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
> commands.

We gain the ability to control trace via QMP, but lose the ability to
control it via HMP, correct?

>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  hmp-commands.hx     |   35 -----------------------
>  monitor.c           |   61 ----------------------------------------
>  qapi-schema.json    |    3 ++
>  qmp-commands.hx     |   27 ++++++++++++++++++
>  trace/Makefile.objs |    1 +
>  trace/commands.json |   44 +++++++++++++++++++++++++++++

There's no precedence for keeping schema bits anywhere but in qapi/.
If we decide we want to keep them with their related C parts instead, we
should do so consistently, not just for trace.

>  trace/control.c     |   13 ---------
>  trace/control.h     |    7 -----
>  trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  trace/simple.c      |   10 +------
>  trace/simple.h      |    3 --
>  11 files changed, 154 insertions(+), 127 deletions(-)
>  create mode 100644 trace/commands.json
>  create mode 100644 trace/qmp.c
[...]
Lluís Vilanova Aug. 20, 2014, 6:30 p.m. UTC | #2
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>> commands.

> We gain the ability to control trace via QMP, but lose the ability to
> control it via HMP, correct?

Right. I can keep the HMP commands, but doing so requires exposing the internal
trace event identifier number in the QMP interface. Also, "trace-file" cannot be
implemented on top of the current QMP commands, and is specific to the "simple"
tracing backend.


>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> hmp-commands.hx     |   35 -----------------------
>> monitor.c           |   61 ----------------------------------------
>> qapi-schema.json    |    3 ++
>> qmp-commands.hx     |   27 ++++++++++++++++++
>> trace/Makefile.objs |    1 +
>> trace/commands.json |   44 +++++++++++++++++++++++++++++

> There's no precedence for keeping schema bits anywhere but in qapi/.
> If we decide we want to keep them with their related C parts instead, we
> should do so consistently, not just for trace.

Ok, I'll move it to qapi.


Thanks,
  Lluis
Markus Armbruster Aug. 20, 2014, 7:02 p.m. UTC | #3
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>>> commands.
>
>> We gain the ability to control trace via QMP, but lose the ability to
>> control it via HMP, correct?
>
> Right. I can keep the HMP commands, but doing so requires exposing the internal
> trace event identifier number in the QMP interface. Also, "trace-file" cannot be
> implemented on top of the current QMP commands, and is specific to the "simple"
> tracing backend.

Would rough feature-parity with QMP be feasible with new HMP commands on
top of the QMP interfaces, completely ignoring HMP backward
compatibility?

[...]
Lluís Vilanova Aug. 20, 2014, 7:28 p.m. UTC | #4
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>> 
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>>>> commands.
>> 
>>> We gain the ability to control trace via QMP, but lose the ability to
>>> control it via HMP, correct?
>> 
>> Right. I can keep the HMP commands, but doing so requires exposing the internal
>> trace event identifier number in the QMP interface. Also, "trace-file" cannot be
>> implemented on top of the current QMP commands, and is specific to the "simple"
>> tracing backend.

> Would rough feature-parity with QMP be feasible with new HMP commands on
> top of the QMP interfaces, completely ignoring HMP backward
> compatibility?

Except for "trace-file", it would be quite easy, yes. For "trace-file", we can
either keep the current HMP implementation, or add new QMP commands to support
it (I'd rather devote my time to something else, though).


Thanks,
  Lluis
Markus Armbruster Aug. 21, 2014, 7:14 a.m. UTC | #5
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>> 
>>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>>>>> commands.
>>> 
>>>> We gain the ability to control trace via QMP, but lose the ability to
>>>> control it via HMP, correct?
>>> 
>>> Right. I can keep the HMP commands, but doing so requires exposing
>>> the internal
>>> trace event identifier number in the QMP interface. Also,
>>> "trace-file" cannot be
>>> implemented on top of the current QMP commands, and is specific to
>>> the "simple"
>>> tracing backend.
>
>> Would rough feature-parity with QMP be feasible with new HMP commands on
>> top of the QMP interfaces, completely ignoring HMP backward
>> compatibility?
>
> Except for "trace-file", it would be quite easy, yes.

Yes, please!

>                                                       For "trace-file", we can
> either keep the current HMP implementation, or add new QMP commands to support
> it (I'd rather devote my time to something else, though).

trace-file is only available with CONFIG_TRACE_SIMPLE.  Losing it would
be tolerable, I guess.  But I'd keep it as is until its maintenance
becomes a burden.
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d0943b1..a0d8999 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -269,36 +269,6 @@  Output logs to @var{filename}.
 ETEXI
 
     {
-        .name       = "trace-event",
-        .args_type  = "name:s,option:b",
-        .params     = "name on|off",
-        .help       = "changes status of a specific trace event",
-        .mhandler.cmd = do_trace_event_set_state,
-    },
-
-STEXI
-@item trace-event
-@findex trace-event
-changes status of a trace event
-ETEXI
-
-#if defined(CONFIG_TRACE_SIMPLE)
-    {
-        .name       = "trace-file",
-        .args_type  = "op:s?,arg:F?",
-        .params     = "on|off|flush|set [arg]",
-        .help       = "open, close, or flush trace file, or set a new file name",
-        .mhandler.cmd = do_trace_file,
-    },
-
-STEXI
-@item trace-file on|off|flush
-@findex trace-file
-Open, close, or flush the trace file.  If no argument is given, the status of the trace file is displayed.
-ETEXI
-#endif
-
-    {
         .name       = "log",
         .args_type  = "items:s",
         .params     = "item1[,...]",
@@ -1784,10 +1754,5 @@  show the TPM device
 ETEXI
 
 STEXI
-@item info trace-events
-show available trace events and their state
-ETEXI
-
-STEXI
 @end table
 ETEXI
diff --git a/monitor.c b/monitor.c
index cdbaa60..d856d84 100644
--- a/monitor.c
+++ b/monitor.c
@@ -61,10 +61,6 @@ 
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "trace.h"
-#include "trace/control.h"
-#ifdef CONFIG_TRACE_SIMPLE
-#include "trace/simple.h"
-#endif
 #include "exec/memory.h"
 #include "exec/cpu_ldst.h"
 #include "qmp-commands.h"
@@ -882,51 +878,6 @@  static void do_help_cmd(Monitor *mon, const QDict *qdict)
     help_cmd(mon, qdict_get_try_str(qdict, "name"));
 }
 
-static void do_trace_event_set_state(Monitor *mon, const QDict *qdict)
-{
-    const char *tp_name = qdict_get_str(qdict, "name");
-    bool new_state = qdict_get_bool(qdict, "option");
-
-    bool found = false;
-    TraceEvent *ev = NULL;
-    while ((ev = trace_event_pattern(tp_name, ev)) != NULL) {
-        found = true;
-        if (!trace_event_get_state_static(ev)) {
-            monitor_printf(mon, "event \"%s\" is not traceable\n", tp_name);
-        } else {
-            trace_event_set_state_dynamic(ev, new_state);
-        }
-    }
-    if (!trace_event_is_pattern(tp_name) && !found) {
-        monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
-    }
-}
-
-#ifdef CONFIG_TRACE_SIMPLE
-static void do_trace_file(Monitor *mon, const QDict *qdict)
-{
-    const char *op = qdict_get_try_str(qdict, "op");
-    const char *arg = qdict_get_try_str(qdict, "arg");
-
-    if (!op) {
-        st_print_trace_file_status((FILE *)mon, &monitor_fprintf);
-    } else if (!strcmp(op, "on")) {
-        st_set_trace_file_enabled(true);
-    } else if (!strcmp(op, "off")) {
-        st_set_trace_file_enabled(false);
-    } else if (!strcmp(op, "flush")) {
-        st_flush_trace_buffer();
-    } else if (!strcmp(op, "set")) {
-        if (arg) {
-            st_set_trace_file(arg);
-        }
-    } else {
-        monitor_printf(mon, "unexpected argument \"%s\"\n", op);
-        help_cmd(mon, "trace-file");
-    }
-}
-#endif
-
 static void user_monitor_complete(void *opaque, QObject *ret_data)
 {
     MonitorCompletionData *data = (MonitorCompletionData *)opaque; 
@@ -1077,11 +1028,6 @@  static void do_info_cpu_stats(Monitor *mon, const QDict *qdict)
     cpu_dump_statistics(cpu, (FILE *)mon, &monitor_fprintf, 0);
 }
 
-static void do_trace_print_events(Monitor *mon, const QDict *qdict)
-{
-    trace_print_events((FILE *)mon, &monitor_fprintf);
-}
-
 static int client_migrate_info(Monitor *mon, const QDict *qdict,
                                MonitorCompletion cb, void *opaque)
 {
@@ -2898,13 +2844,6 @@  static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = do_info_roms,
     },
     {
-        .name       = "trace-events",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show available trace-events & their state",
-        .mhandler.cmd = do_trace_print_events,
-    },
-    {
         .name       = "tpm",
         .args_type  = "",
         .params     = "",
diff --git a/qapi-schema.json b/qapi-schema.json
index 341f417..0b90f60 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -11,6 +11,9 @@ 
 # QAPI event definitions
 { 'include': 'qapi/event.json' }
 
+# Tracing commands
+{ 'include': 'trace/commands.json' }
+
 ##
 # LostTickPolicy:
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4be4765..443dd16 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3753,5 +3753,32 @@  Example:
 
 -> { "execute": "rtc-reset-reinjection" }
 <- { "return": {} }
+EQMP
+
+    {
+        .name       = "trace-event-get-state",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_input_trace_event_get_state,
+    },
+
+SQMP
+trace-event-get-state
+---------------------
+
+Query the state of events.
+
+EQMP
+
+    {
+        .name       = "trace-event-set-state",
+        .args_type  = "name:s,state:b,keepgoing:b?",
+        .mhandler.cmd_new = qmp_marshal_input_trace_event_set_state,
+    },
+
+SQMP
+trace-event-set-state
+---------------------
+
+Set the state of events.
 
 EQMP
diff --git a/trace/Makefile.objs b/trace/Makefile.objs
index 387f191..01b3718 100644
--- a/trace/Makefile.objs
+++ b/trace/Makefile.objs
@@ -145,3 +145,4 @@  util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o
 util-obj-$(CONFIG_TRACE_UST) += generated-ust.o
 util-obj-y += control.o
 util-obj-y += generated-tracers.o
+util-obj-y += qmp.o
diff --git a/trace/commands.json b/trace/commands.json
new file mode 100644
index 0000000..6e6313d
--- /dev/null
+++ b/trace/commands.json
@@ -0,0 +1,44 @@ 
+# -*- mode: python -*-
+
+##
+# @TraceEventState:
+#
+# State of a tracing event.
+#
+# @name: Event name.
+# @sstatic: Static tracing state.
+# @sdynamic: Dynamic tracing state.
+#
+# Since 2.2
+##
+{ 'type': 'TraceEventState',
+  'data': {'name': 'str', 'sstatic': 'bool', 'sdynamic': 'bool'} }
+
+##
+# @trace-event-get-state:
+#
+# Query the state of events.
+#
+# @name: Event name pattern.
+#
+# Returns: @TraceEventState of the matched events
+#
+# Since 2.2
+##
+{ 'command': 'trace-event-get-state',
+  'data': {'name': 'str'},
+  'returns': ['TraceEventState'] }
+
+##
+# @trace-event-set-state:
+#
+# Set the dynamic state of events.
+#
+# @name: Event name pattern.
+# @state: Dynamic tracing state.
+# @keepgoing: #optional Apply changes even if not all events can be changed.
+#
+# Since 2.2
+##
+{ 'command': 'trace-event-set-state',
+  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }
diff --git a/trace/control.c b/trace/control.c
index 9631a40..0d30801 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -85,19 +85,6 @@  TraceEvent *trace_event_pattern(const char *pat, TraceEvent *ev)
     return NULL;
 }
 
-void trace_print_events(FILE *stream, fprintf_function stream_printf)
-{
-    TraceEventID i;
-
-    for (i = 0; i < trace_event_count(); i++) {
-        TraceEvent *ev = trace_event_id(i);
-        stream_printf(stream, "%s [Event ID %u] : state %u\n",
-                      trace_event_get_name(ev), i,
-                      trace_event_get_state_static(ev) &&
-                      trace_event_get_state_dynamic(ev));
-    }
-}
-
 static void trace_init_events(const char *fname)
 {
     Location loc;
diff --git a/trace/control.h b/trace/control.h
index e1ec033..da9bb6b 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -149,13 +149,6 @@  static void trace_event_set_state_dynamic(TraceEvent *ev, bool state);
 
 
 /**
- * trace_print_events:
- *
- * Print the state of all events.
- */
-void trace_print_events(FILE *stream, fprintf_function stream_printf);
-
-/**
  * trace_init_backends:
  * @events: Name of file with events to be enabled at startup; may be NULL.
  *          Corresponds to commandline option "-trace events=...".
diff --git a/trace/qmp.c b/trace/qmp.c
new file mode 100644
index 0000000..d22d8fa
--- /dev/null
+++ b/trace/qmp.c
@@ -0,0 +1,77 @@ 
+/*
+ * QMP commands for tracing events.
+ *
+ * Copyright (C) 2014 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/typedefs.h"
+#include "qmp-commands.h"
+#include "trace/control.h"
+
+
+TraceEventStateList *qmp_trace_event_get_state(const char *name, Error **errp)
+{
+    TraceEventStateList dummy = {};
+    TraceEventStateList *prev = &dummy;
+
+    bool found = false;
+    TraceEvent *ev = NULL;
+    while ((ev = trace_event_pattern(name, ev)) != NULL) {
+        found = true;
+        TraceEventStateList *elem = g_malloc0(sizeof(*elem));
+        elem->value = g_malloc0(sizeof(*elem->value));
+        elem->value->name = g_strdup(trace_event_get_name(ev));
+        elem->value->sstatic = trace_event_get_state_static(ev);
+        elem->value->sdynamic = trace_event_get_state_dynamic(ev);
+        prev->next = elem;
+        prev = elem;
+    }
+    if (!found && !trace_event_is_pattern(name)) {
+        error_setg(errp, "unknown event \"%s\"\n", name);
+    }
+
+    return dummy.next;
+}
+
+void qmp_trace_event_set_state(const char *name, bool state, bool has_keepgoing,
+                               bool keepgoing, Error **errp)
+{
+    bool error = false;
+    bool found = false;
+    TraceEvent *ev = NULL;
+
+    /* Check all selected events are dynamic */
+    while ((ev = trace_event_pattern(name, ev)) != NULL) {
+        found = true;
+        if (!trace_event_get_state_static(ev)) {
+            error_setg(errp, "cannot set dynamic tracing state for \"%s\"\n",
+                       trace_event_get_name(ev));
+            if (!(has_keepgoing && keepgoing)) {
+                error = true;
+            }
+            break;
+        }
+    }
+    if (error) {
+        return;
+    }
+    if (!found && !trace_event_is_pattern(name)) {
+        error_setg(errp, "unknown event \"%s\"\n", name);
+        return;
+    }
+
+    if (error) {
+        return;
+    }
+
+    /* Apply changes */
+    ev = NULL;
+    while ((ev = trace_event_pattern(name, ev)) != NULL) {
+        if (trace_event_get_state_static(ev)) {
+            trace_event_set_state_dynamic(ev, state);
+        }
+    }
+}
diff --git a/trace/simple.c b/trace/simple.c
index 11ad030..7a2e44d 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -277,7 +277,7 @@  void trace_record_finish(TraceBufferRecord *rec)
     }
 }
 
-void st_set_trace_file_enabled(bool enable)
+static void st_set_trace_file_enabled(bool enable)
 {
     if (enable == !!trace_fp) {
         return; /* no change */
@@ -322,7 +322,7 @@  void st_set_trace_file_enabled(bool enable)
  * @file        The trace file name or NULL for the default name-<pid> set at
  *              config time
  */
-bool st_set_trace_file(const char *file)
+static bool st_set_trace_file(const char *file)
 {
     st_set_trace_file_enabled(false);
 
@@ -338,12 +338,6 @@  bool st_set_trace_file(const char *file)
     return true;
 }
 
-void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
-{
-    stream_printf(stream, "Trace file \"%s\" %s.\n",
-                  trace_file_name, trace_fp ? "on" : "off");
-}
-
 void st_flush_trace_buffer(void)
 {
     flush_trace_file(true);
diff --git a/trace/simple.h b/trace/simple.h
index 6997996..316a479 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -18,9 +18,6 @@ 
 #include "trace/generated-events.h"
 
 
-void st_print_trace_file_status(FILE *stream, fprintf_function stream_printf);
-void st_set_trace_file_enabled(bool enable);
-bool st_set_trace_file(const char *file);
 bool st_init(const char *file);
 void st_flush_trace_buffer(void);