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

[Markus Armbruster]

Lluís Vilanova <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + * 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.