Markus Armbruster writes: > Lluís Vilanova <[email protected]> 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 <[email protected]> >> --- >> 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 <[email protected]> >> + * >> + * 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
