[Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

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

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

diff --git a/monitor.c b/monitor.c
index cdbaa60..0f605f5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -887,19 +887,8 @@ 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);
-    }
+    /* TODO: should propagate QMP errors to HMP */
+    qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);
 }
 
 #ifdef CONFIG_TRACE_SIMPLE
@@ -1079,7 +1068,14 @@ static void do_info_cpu_stats(Monitor *mon, const QDict *qdict)
 
 static void do_trace_print_events(Monitor *mon, const QDict *qdict)
 {
-    trace_print_events((FILE *)mon, &monitor_fprintf);
+    TraceEventStateList *events = qmp_trace_event_get_state("*", NULL);
+    TraceEventStateList *event;
+    for (event = events; event != NULL; event = event->next) {
+        monitor_printf(mon, "%s : state %u\n",
+                       event->value->name,
+                       event->value->sstatic && event->value->sdynamic);
+    }
+    qapi_free_TraceEventStateList(events);
 }
 
 static int client_migrate_info(Monitor *mon, const QDict *qdict,
diff --git a/qapi-schema.json b/qapi-schema.json
index 341f417..42b90df 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -11,6 +11,9 @@
 # QAPI event definitions
 { 'include': 'qapi/event.json' }
 
+# Tracing commands
+{ 'include': 'qapi/trace.json' }
+
 ##
 # LostTickPolicy:
 #
diff --git a/qapi/trace.json b/qapi/trace.json
new file mode 100644
index 0000000..6e6313d
--- /dev/null
+++ b/qapi/trace.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/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/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);
+        }
+    }
+}

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Stefan Hajnoczi]

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

On Thu, Aug 21, 2014 at 07:52:37PM +0200, Lluís Vilanova wrote:
> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
> commands.

Is this commit description correct?  I think we don't want to remove
HMP commands.  It is "legacy" but users may still rely on HMP.  It's
certainly used for ad-hoc debugging.

> diff --git a/monitor.c b/monitor.c
> index cdbaa60..0f605f5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -887,19 +887,8 @@ 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);
> -    }
> +    /* TODO: should propagate QMP errors to HMP */
> +    qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);

The TODO can be resolved with:

if (errp) {
    monitor_printf(mon, "%s", error_get_pretty(errp));
    error_free(errp);
}

> +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);

\n is not needed in error_setg() message.  Please remove it.

There are more instances below.

> +    }
> +
> +    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));

error_setg() can only be called once.  Calling it with non-NULL errp
produces an assertion failure.

Maybe this approach can be used instead:

while ((ev = trace_event_pattern(name, ev)) != NULL) {
    found = true;
    if (!(has_keepgoing && keepgoing) &&
        !trace_event_get_state_static(ev)) {
        error_setg(errp, "cannot set dynamic tracing state for \"%s\"",
	           trace_event_get_name(ev));
	return;
    }
}

The bool error variable can be dropped.

> +            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;
> +    }

This condition has already been checked above.  This if statement can be
dropped.

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

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

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

I can't see any HMP commands removal in your patch.  You 

> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  monitor.c           |   24 +++++++---------
>  qapi-schema.json    |    3 ++
>  qapi/trace.json     |   44 +++++++++++++++++++++++++++++
>  qmp-commands.hx     |   27 ++++++++++++++++++
>  trace/Makefile.objs |    1 +
>  trace/control.c     |   13 ---------
>  trace/control.h     |    7 -----
>  trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 162 insertions(+), 34 deletions(-)
>  create mode 100644 qapi/trace.json
>  create mode 100644 trace/qmp.c
>
> diff --git a/monitor.c b/monitor.c
> index cdbaa60..0f605f5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -887,19 +887,8 @@ 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);
> -    }
> +    /* TODO: should propagate QMP errors to HMP */
> +    qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);

Easy:

    Error *local_err = NULL;
    [...]
    qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
    if (local_err) {
        qerror_report_err(local_err);
        error_free(local_err);
    }

>  }
>  
>  #ifdef CONFIG_TRACE_SIMPLE
> @@ -1079,7 +1068,14 @@ static void do_info_cpu_stats(Monitor *mon, const QDict *qdict)
>  
>  static void do_trace_print_events(Monitor *mon, const QDict *qdict)
>  {
> -    trace_print_events((FILE *)mon, &monitor_fprintf);
> +    TraceEventStateList *events = qmp_trace_event_get_state("*", NULL);
> +    TraceEventStateList *event;

Blank line here, please, to visually separate declarations and statements.

> +    for (event = events; event != NULL; event = event->next) {
> +        monitor_printf(mon, "%s : state %u\n",
> +                       event->value->name,
> +                       event->value->sstatic && event->value->sdynamic);
> +    }
> +    qapi_free_TraceEventStateList(events);

Drops "[Event ID %u]" from output.  Is that number interesting to
users?  If no, I don't mind.

>  }
>  
>  static int client_migrate_info(Monitor *mon, const QDict *qdict,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 341f417..42b90df 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -11,6 +11,9 @@
>  # QAPI event definitions
>  { 'include': 'qapi/event.json' }
>  
> +# Tracing commands
> +{ 'include': 'qapi/trace.json' }
> +
>  ##
>  # LostTickPolicy:
>  #
> diff --git a/qapi/trace.json b/qapi/trace.json
> new file mode 100644
> index 0000000..6e6313d
> --- /dev/null
> +++ b/qapi/trace.json
> @@ -0,0 +1,44 @@
> +# -*- mode: python -*-
> +
> +##
> +# @TraceEventState:
> +#
> +# State of a tracing event.
> +#
> +# @name: Event name.
> +# @sstatic: Static tracing state.
> +# @sdynamic: Dynamic tracing state.

Maybe static-state, dynamic-state?

What do static and dynamic state mean?

> +#
> +# 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.

What kind of pattern is this?

> +#
> +# Returns: @TraceEventState of the matched events

Make that "Returns: a list of @TraceEventState for the matching 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.

Suggest to name this argument exactly like the TraceEventState member.

> +# @keepgoing: #optional Apply changes even if not all events can be changed.

How can that happen?  I.e. how can setting an event's state fail?

> +#
> +# Since 2.2
> +##
> +{ 'command': 'trace-event-set-state',
> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }
> 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/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;
> +

Please move this blank line...

> +    bool found = false;
> +    TraceEvent *ev = NULL;

here, so that it visually separates declarations and statements.

Dead initialization of ev, by the way.

> +    while ((ev = trace_event_pattern(name, ev)) != NULL) {
> +        found = true;
> +        TraceEventStateList *elem = g_malloc0(sizeof(*elem));

Declaration follows statement, please don't do that.

> +        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;

The usual way to build a list of some QAPI type would be like this:

    TraceEventStateList *events = NULL;
    TraceEvent *ev;
    TraceEventStateList *elem;

    while ((ev = trace_event_pattern(name, ev)) != NULL) {
        elem = g_new(TraceEventStateList);
        [Initialize *elem...]
        elem->next = events;
        events = elem;
    }

    if (!events && !trace_event_is_pattern(name)) {
        error_setg(errp, "unknown event \"%s\"\n", name);
    }

    return events;

A good deal easier to understand.  Several examples of QMP commands
returning lists can be found in qmp.c.

> +}
> +
> +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;

Dead initialization.

> +
> +    /* 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);

Safe, because when the loop above set an error, it also set found; if we
get here, the loop didn't set one.

> +        return;
> +    }
> +
> +    if (error) {
> +        return;
> +    }

This can't happen.

> +
> +    /* Apply changes */
> +    ev = NULL;

Dead assignment.

> +    while ((ev = trace_event_pattern(name, ev)) != NULL) {
> +        if (trace_event_get_state_static(ev)) {
> +            trace_event_set_state_dynamic(ev, state);
> +        }
> +    }
> +}

When keepgoing, this can apply changes and still return an error.
Intentional?

Your error variable tracks whether an error happened.  Why not test for
that directly?  Of course, you shouldn't test *errp, because that would
require a non-null errp argument.  You could set local_err, then
error_propagate().  Just a thought, perhaps you like it.


Consider splitting this patch into two parts: 1. New QMP commands, 2. 
reimplement HMP commands in term of the new QMP commands.

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Eric Blake]

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

On 08/21/2014 11:52 AM, Lluís Vilanova wrote:
> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
> commands.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  monitor.c           |   24 +++++++---------
>  qapi-schema.json    |    3 ++
>  qapi/trace.json     |   44 +++++++++++++++++++++++++++++
>  qmp-commands.hx     |   27 ++++++++++++++++++
>  trace/Makefile.objs |    1 +
>  trace/control.c     |   13 ---------
>  trace/control.h     |    7 -----
>  trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 162 insertions(+), 34 deletions(-)
>  create mode 100644 qapi/trace.json
>  create mode 100644 trace/qmp.c
> 

> +++ b/qapi/trace.json
> @@ -0,0 +1,44 @@
> +# -*- mode: python -*-
> +

No copyright statement (but you are just following (poor) existing
practice).

> +##
> +# @TraceEventState:
> +#
> +# State of a tracing event.
> +#
> +# @name: Event name.
> +# @sstatic: Static tracing state.
> +# @sdynamic: Dynamic tracing state.

Does the leading 's' add any value?  QAPI doesn't have to use obscure
abbreviations.

> +#
> +# Since 2.2
> +##
> +{ 'type': 'TraceEventState',
> +  'data': {'name': 'str', 'sstatic': 'bool', 'sdynamic': 'bool'} }

Are all four states between the combinations of the two bools possible,
or is this more of a tri-state setting (default, sstatic, or sdynamic),
in which case a single parameter of enum type would be better than two
bools?

> +
> +##
> +# @trace-event-get-state:
> +#
> +# Query the state of events.
> +#
> +# @name: Event name pattern.

glob? regex? Is the pattern anchored or just substring analysis?
Case-sensitive?

> +#
> +# Returns: @TraceEventState of the matched events
> +#
> +# Since 2.2
> +##
> +{ 'command': 'trace-event-get-state',
> +  'data': {'name': 'str'},
> +  'returns': ['TraceEventState'] }

What if I want ALL events?  Can name be made optional to avoid any
filtering?

> +
> +##
> +# @trace-event-set-state:
> +#
> +# Set the dynamic state of events.
> +#
> +# @name: Event name pattern.

Same comments about pattern as before.

> +# @state: Dynamic tracing state.
> +# @keepgoing: #optional Apply changes even if not all events can be changed.

keep-going - use dash to separate words for legibility

> +#
> +# Since 2.2
> +##
> +{ 'command': 'trace-event-set-state',
> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }

This only lets me set the state of one name at a time.  Oh, unless I'm
setting a pattern, and it then sets the state of all names that match
that pattern.  I'm wondering if you should have 'name':['str'] to allow
me to set multiple names/patterns in one go, instead of having to call
the command multiple times; but it's probably not worth the complexity.

> 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.

No example usage?

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

Eric Blake writes:

> On 08/21/2014 11:52 AM, Lluís Vilanova wrote:
>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>> commands.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> monitor.c           |   24 +++++++---------
>> qapi-schema.json    |    3 ++
>> qapi/trace.json     |   44 +++++++++++++++++++++++++++++
>> qmp-commands.hx     |   27 ++++++++++++++++++
>> trace/Makefile.objs |    1 +
>> trace/control.c     |   13 ---------
>> trace/control.h     |    7 -----
>> trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 8 files changed, 162 insertions(+), 34 deletions(-)
>> create mode 100644 qapi/trace.json
>> create mode 100644 trace/qmp.c
>> 

>> +++ b/qapi/trace.json
>> @@ -0,0 +1,44 @@
>> +# -*- mode: python -*-
>> +

> No copyright statement (but you are just following (poor) existing
> practice).

Will add.


>> +##
>> +# @TraceEventState:
>> +#
>> +# State of a tracing event.
>> +#
>> +# @name: Event name.
>> +# @sstatic: Static tracing state.
>> +# @sdynamic: Dynamic tracing state.

> Does the leading 's' add any value?  QAPI doesn't have to use obscure
> abbreviations.

It felt wrong to have a JSON attribute named differently from the C struct
(static is a reserved word).


>> +#
>> +# Since 2.2
>> +##
>> +{ 'type': 'TraceEventState',
>> +  'data': {'name': 'str', 'sstatic': 'bool', 'sdynamic': 'bool'} }

> Are all four states between the combinations of the two bools possible,
> or is this more of a tri-state setting (default, sstatic, or sdynamic),
> in which case a single parameter of enum type would be better than two
> bools?

I can certainly change it to an "state" enum:

* unavailable: statically disabled
* disabled   : statically enabled, dynamically disabled
* enabled    : statically enabled, dynamically enabled


>> +
>> +##
>> +# @trace-event-get-state:
>> +#
>> +# Query the state of events.
>> +#
>> +# @name: Event name pattern.

> glob? regex? Is the pattern anchored or just substring analysis?
> Case-sensitive?

It's the same pattern matching used internally in QEMU (case-sensitive
globbing).


>> +#
>> +# Returns: @TraceEventState of the matched events
>> +#
>> +# Since 2.2
>> +##
>> +{ 'command': 'trace-event-get-state',
>> +  'data': {'name': 'str'},
>> +  'returns': ['TraceEventState'] }

> What if I want ALL events?  Can name be made optional to avoid any
> filtering?

I use "*" as the "name", which seems simple enough to me.


>> +
>> +##
>> +# @trace-event-set-state:
>> +#
>> +# Set the dynamic state of events.
>> +#
>> +# @name: Event name pattern.

> Same comments about pattern as before.

>> +# @state: Dynamic tracing state.
>> +# @keepgoing: #optional Apply changes even if not all events can be changed.

> keep-going - use dash to separate words for legibility

Ok.


>> +#
>> +# Since 2.2
>> +##
>> +{ 'command': 'trace-event-set-state',
>> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }

> This only lets me set the state of one name at a time.  Oh, unless I'm
> setting a pattern, and it then sets the state of all names that match
> that pattern.  I'm wondering if you should have 'name':['str'] to allow
> me to set multiple names/patterns in one go, instead of having to call
> the command multiple times; but it's probably not worth the complexity.

I agree with the complexity comment. Also, the keepgoing is useful to set events
using a pattern, even if some of them are statically disabled (otherwise it
gives an error).


>> 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.

> No example usage?

Heh, lazy me. Will add.


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

Markus Armbruster writes:

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

> I can't see any HMP commands removal in your patch.  You 

Sorry, I forgot to remove that from the commit message.


>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> monitor.c           |   24 +++++++---------
>> qapi-schema.json    |    3 ++
>> qapi/trace.json     |   44 +++++++++++++++++++++++++++++
>> qmp-commands.hx     |   27 ++++++++++++++++++
>> trace/Makefile.objs |    1 +
>> trace/control.c     |   13 ---------
>> trace/control.h     |    7 -----
>> trace/qmp.c         |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 8 files changed, 162 insertions(+), 34 deletions(-)
>> create mode 100644 qapi/trace.json
>> create mode 100644 trace/qmp.c
>> 
>> diff --git a/monitor.c b/monitor.c
>> index cdbaa60..0f605f5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -887,19 +887,8 @@ 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);
>> -    }
>> +    /* TODO: should propagate QMP errors to HMP */
>> +    qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);

> Easy:

>     Error *local_err = NULL;
>     [...]
>     qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
>     if (local_err) {
>         qerror_report_err(local_err);
>         error_free(local_err);
>     }

Nice, thanks.


>> }
>> 
>> #ifdef CONFIG_TRACE_SIMPLE
>> @@ -1079,7 +1068,14 @@ static void do_info_cpu_stats(Monitor *mon, const QDict *qdict)
>> 
>> static void do_trace_print_events(Monitor *mon, const QDict *qdict)
>> {
>> -    trace_print_events((FILE *)mon, &monitor_fprintf);
>> +    TraceEventStateList *events = qmp_trace_event_get_state("*", NULL);
>> +    TraceEventStateList *event;

> Blank line here, please, to visually separate declarations and statements.

Ok.


>> +    for (event = events; event != NULL; event = event->next) {
>> +        monitor_printf(mon, "%s : state %u\n",
>> +                       event->value->name,
>> +                       event->value->sstatic && event->value->sdynamic);
>> +    }
>> +    qapi_free_TraceEventStateList(events);

> Drops "[Event ID %u]" from output.  Is that number interesting to
> users?  If no, I don't mind.

I think it's not, that's why I removed it. The removal also avoids exposing that
number through the QAPI interface.


>> }
>> 
>> static int client_migrate_info(Monitor *mon, const QDict *qdict,
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 341f417..42b90df 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -11,6 +11,9 @@
>> # QAPI event definitions
>> { 'include': 'qapi/event.json' }
>> 
>> +# Tracing commands
>> +{ 'include': 'qapi/trace.json' }
>> +
>> ##
>> # LostTickPolicy:
>> #
>> diff --git a/qapi/trace.json b/qapi/trace.json
>> new file mode 100644
>> index 0000000..6e6313d
>> --- /dev/null
>> +++ b/qapi/trace.json
>> @@ -0,0 +1,44 @@
>> +# -*- mode: python -*-
>> +
>> +##
>> +# @TraceEventState:
>> +#
>> +# State of a tracing event.
>> +#
>> +# @name: Event name.
>> +# @sstatic: Static tracing state.
>> +# @sdynamic: Dynamic tracing state.

> Maybe static-state, dynamic-state?

> What do static and dynamic state mean?

If "sstatic" is false, it means the event is not available (the event has the
"disable" property). Maybe it will be clearer if I merge them into a tristate
enum as Eric suggested.


>> +#
>> +# 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.

> What kind of pattern is this?

See response to Eric.


>> +#
>> +# Returns: @TraceEventState of the matched events

> Make that "Returns: a list of @TraceEventState for the matching events".

Ok.


>> +#
>> +# 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.

> Suggest to name this argument exactly like the TraceEventState member.

Will do.


>> +# @keepgoing: #optional Apply changes even if not all events can be changed.

> How can that happen?  I.e. how can setting an event's state fail?

See my explanation about "sstate" above.


>> +#
>> +# Since 2.2
>> +##
>> +{ 'command': 'trace-event-set-state',
>> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }
>> 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/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;
>> +

> Please move this blank line...

>> +    bool found = false;
>> +    TraceEvent *ev = NULL;

> here, so that it visually separates declarations and statements.

> Dead initialization of ev, by the way.

>> +    while ((ev = trace_event_pattern(name, ev)) != NULL) {
>> +        found = true;
>> +        TraceEventStateList *elem = g_malloc0(sizeof(*elem));

> Declaration follows statement, please don't do that.

>> +        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;

> The usual way to build a list of some QAPI type would be like this:

>     TraceEventStateList *events = NULL;
>     TraceEvent *ev;
>     TraceEventStateList *elem;

>     while ((ev = trace_event_pattern(name, ev)) != NULL) {
>         elem = g_new(TraceEventStateList);
>         [Initialize *elem...]
elem-> next = events;
>         events = elem;
>     }

>     if (!events && !trace_event_is_pattern(name)) {
>         error_setg(errp, "unknown event \"%s\"\n", name);
>     }

>     return events;

> A good deal easier to understand.  Several examples of QMP commands
> returning lists can be found in qmp.c.

Ok, thanks.


>> +}
>> +
>> +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;

> Dead initialization.

Ok.


>> +
>> +    /* 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);

> Safe, because when the loop above set an error, it also set found; if we
> get here, the loop didn't set one.

>> +        return;
>> +    }
>> +
>> +    if (error) {
>> +        return;
>> +    }

> This can't happen.

>> +
>> +    /* Apply changes */
>> +    ev = NULL;

> Dead assignment.

>> +    while ((ev = trace_event_pattern(name, ev)) != NULL) {
>> +        if (trace_event_get_state_static(ev)) {
>> +            trace_event_set_state_dynamic(ev, state);
>> +        }
>> +    }
>> +}

> When keepgoing, this can apply changes and still return an error.
> Intentional?

Yes, but it does sound that good now...


> Your error variable tracks whether an error happened.  Why not test for
> that directly?  Of course, you shouldn't test *errp, because that would
> require a non-null errp argument.  You could set local_err, then
> error_propagate().  Just a thought, perhaps you like it.

Right, and I can skip propagation if "keepgoing" is set. Much better.


> Consider splitting this patch into two parts: 1. New QMP commands, 2. 
> reimplement HMP commands in term of the new QMP commands.

Will do.


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Thu, Aug 21, 2014 at 07:52:37PM +0200, Lluís Vilanova wrote:
>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>> commands.

> Is this commit description correct?  I think we don't want to remove
> HMP commands.  It is "legacy" but users may still rely on HMP.  It's
> certainly used for ad-hoc debugging.

The description is a leftover from a previous version.


>> diff --git a/monitor.c b/monitor.c
>> index cdbaa60..0f605f5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -887,19 +887,8 @@ 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);
>> -    }
>> +    /* TODO: should propagate QMP errors to HMP */
>> +    qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);

> The TODO can be resolved with:

> if (errp) {
>     monitor_printf(mon, "%s", error_get_pretty(errp));
>     error_free(errp);
> }

Thanks.


>> +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);

> \n is not needed in error_setg() message.  Please remove it.

> There are more instances below.

Ok.


>> +    }
>> +
>> +    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));

> error_setg() can only be called once.  Calling it with non-NULL errp
> produces an assertion failure.

> Maybe this approach can be used instead:

> while ((ev = trace_event_pattern(name, ev)) != NULL) {
>     found = true;
>     if (!(has_keepgoing && keepgoing) &&
>         !trace_event_get_state_static(ev)) {
>         error_setg(errp, "cannot set dynamic tracing state for \"%s\"",
> 	           trace_event_get_name(ev));
> 	return;
>     }
> }

> The bool error variable can be dropped.

Ok.


>> +            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;
>> +    }

> This condition has already been checked above.  This if statement can be
> dropped.

Copy/paste is the root of many evils...


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Eric Blake]

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

On 08/22/2014 12:16 PM, Lluís Vilanova wrote:

>>> +# @name: Event name.
>>> +# @sstatic: Static tracing state.
>>> +# @sdynamic: Dynamic tracing state.
> 
>> Does the leading 's' add any value?  QAPI doesn't have to use obscure
>> abbreviations.
> 
> It felt wrong to have a JSON attribute named differently from the C struct
> (static is a reserved word).

The qapi generator will automatically convert 'static' in QMP to
q_static in C code.  I'd rather the QMP looks clean, even if the
underlying C code has to deal with a munged name.

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Eric Blake writes:
>
>> On 08/21/2014 11:52 AM, Lluís Vilanova wrote:
>>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>>> commands.
>>> 
>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> ---
>>> monitor.c           |   24 +++++++---------
>>> qapi-schema.json    |    3 ++
>>> qapi/trace.json     |   44 +++++++++++++++++++++++++++++
>>> qmp-commands.hx     |   27 ++++++++++++++++++
>>> trace/Makefile.objs |    1 +
>>> trace/control.c     |   13 ---------
>>> trace/control.h     |    7 -----
>>> trace/qmp.c | 77
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 8 files changed, 162 insertions(+), 34 deletions(-)
>>> create mode 100644 qapi/trace.json
>>> create mode 100644 trace/qmp.c
>>> 
>
>>> +++ b/qapi/trace.json
>>> @@ -0,0 +1,44 @@
>>> +# -*- mode: python -*-
>>> +
>
>> No copyright statement (but you are just following (poor) existing
>> practice).
>
> Will add.
>
>
>>> +##
>>> +# @TraceEventState:
>>> +#
>>> +# State of a tracing event.
>>> +#
>>> +# @name: Event name.
>>> +# @sstatic: Static tracing state.
>>> +# @sdynamic: Dynamic tracing state.
>
>> Does the leading 's' add any value?  QAPI doesn't have to use obscure
>> abbreviations.
>
> It felt wrong to have a JSON attribute named differently from the C struct
> (static is a reserved word).
>
>
>>> +#
>>> +# Since 2.2
>>> +##
>>> +{ 'type': 'TraceEventState',
>>> +  'data': {'name': 'str', 'sstatic': 'bool', 'sdynamic': 'bool'} }
>
>> Are all four states between the combinations of the two bools possible,
>> or is this more of a tri-state setting (default, sstatic, or sdynamic),
>> in which case a single parameter of enum type would be better than two
>> bools?
>
> I can certainly change it to an "state" enum:
>
> * unavailable: statically disabled
> * disabled   : statically enabled, dynamically disabled
> * enabled    : statically enabled, dynamically enabled

Yes, please; it's neater.

>>> +
>>> +##
>>> +# @trace-event-get-state:
>>> +#
>>> +# Query the state of events.
>>> +#
>>> +# @name: Event name pattern.
>
>> glob? regex? Is the pattern anchored or just substring analysis?
>> Case-sensitive?
>
> It's the same pattern matching used internally in QEMU (case-sensitive
> globbing).

Please document it.

>>> +#
>>> +# Returns: @TraceEventState of the matched events
>>> +#
>>> +# Since 2.2
>>> +##
>>> +{ 'command': 'trace-event-get-state',
>>> +  'data': {'name': 'str'},
>>> +  'returns': ['TraceEventState'] }
>
>> What if I want ALL events?  Can name be made optional to avoid any
>> filtering?
>
> I use "*" as the "name", which seems simple enough to me.

Good enough for me, too.

>>> +
>>> +##
>>> +# @trace-event-set-state:
>>> +#
>>> +# Set the dynamic state of events.
>>> +#
>>> +# @name: Event name pattern.
>
>> Same comments about pattern as before.
>
>>> +# @state: Dynamic tracing state.
>>> +# @keepgoing: #optional Apply changes even if not all events can be changed.
>
>> keep-going - use dash to separate words for legibility
>
> Ok.
>
>
>>> +#
>>> +# Since 2.2
>>> +##
>>> +{ 'command': 'trace-event-set-state',
>>> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }
>
>> This only lets me set the state of one name at a time.  Oh, unless I'm
>> setting a pattern, and it then sets the state of all names that match
>> that pattern.  I'm wondering if you should have 'name':['str'] to allow
>> me to set multiple names/patterns in one go, instead of having to call
>> the command multiple times; but it's probably not worth the complexity.
>
> I agree with the complexity comment. Also, the keepgoing is useful to set events
> using a pattern, even if some of them are statically disabled (otherwise it
> gives an error).

Yes, let's keep this simple.

However, I feel keepgoing is unnecessarily vague.  Its purpose is to
enable use of a pattern in the presence of disabled events.  I'd prefer
to nail it down to exactly that purpose rather than having it cover
arbitrary, unspecified errors.

A few ideas on how to do that:

* Have a flag to modify the semantics of the pattern: either "match all
  events", or "match just disabled and enabled events, not unavailable
  events".

* To find out what a trace-event-set-state actually does, you need to
  trace-event-get-state the same pattern.  We could make
  trace-event-set-state return the events it changed, so you never have
  to trace-event-get-state, but it's probably not worthwhile.

[...]

Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Eric Blake writes:
>> 
>>> On 08/21/2014 11:52 AM, Lluís Vilanova wrote:
[...]
>>>> +#
>>>> +# Since 2.2
>>>> +##
>>>> +{ 'command': 'trace-event-set-state',
>>>> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }
>> 
>>> This only lets me set the state of one name at a time.  Oh, unless I'm
>>> setting a pattern, and it then sets the state of all names that match
>>> that pattern.  I'm wondering if you should have 'name':['str'] to allow
>>> me to set multiple names/patterns in one go, instead of having to call
>>> the command multiple times; but it's probably not worth the complexity.
>> 
>> I agree with the complexity comment. Also, the keepgoing is useful to set events
>> using a pattern, even if some of them are statically disabled (otherwise it
>> gives an error).

> Yes, let's keep this simple.

> However, I feel keepgoing is unnecessarily vague.  Its purpose is to
> enable use of a pattern in the presence of disabled events.  I'd prefer
> to nail it down to exactly that purpose rather than having it cover
> arbitrary, unspecified errors.

> A few ideas on how to do that:

> * Have a flag to modify the semantics of the pattern: either "match all
>   events", or "match just disabled and enabled events, not unavailable
>   events".

> * To find out what a trace-event-set-state actually does, you need to
>   trace-event-get-state the same pattern.  We could make
>   trace-event-set-state return the events it changed, so you never have
>   to trace-event-get-state, but it's probably not worthwhile.

> [...]

I've renamed it to "ignore-unavailable" (off by default).


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth