[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state |
Date: |
Fri, 22 Aug 2014 20:24:12 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) |
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