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

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


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

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

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