From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59578) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKtVf-0000jV-Hl for qemu-devel@nongnu.org; Fri, 22 Aug 2014 14:24:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKtVY-0003wn-Qr for qemu-devel@nongnu.org; Fri, 22 Aug 2014 14:24:23 -0400 Received: from roura.ac.upc.es ([147.83.33.10]:43354) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKtVY-0003wd-8c for qemu-devel@nongnu.org; Fri, 22 Aug 2014 14:24:16 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <20140821175235.12061.22630.stgit@fimbulvetr.bsc.es> <87lhqgbntm.fsf@blackfin.pond.sub.org> Date: Fri, 22 Aug 2014 20:24:12 +0200 In-Reply-To: <87lhqgbntm.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Fri, 22 Aug 2014 16:28:53 +0200") Message-ID: <8738comlgz.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Luiz Capitulino , qemu-devel@nongnu.org, Stefan Hajnoczi , Michael Roth Markus Armbruster writes: > Llu=C3=ADs Vilanova 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=20 Sorry, I forgot to remove that from the commit message. >> Signed-off-by: Llu=C3=ADs Vilanova >> --- >> 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 >>=20 >> 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 =3D qdict_get_str(qdict, "name"); >> bool new_state =3D qdict_get_bool(qdict, "option"); >>=20 >> - bool found =3D false; >> - TraceEvent *ev =3D NULL; >> - while ((ev =3D trace_event_pattern(tp_name, ev)) !=3D NULL) { >> - found =3D true; >> - if (!trace_event_get_state_static(ev)) { >> - monitor_printf(mon, "event \"%s\" is not traceable\n", tp_n= ame); >> - } 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 =3D 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. >> } >>=20 >> #ifdef CONFIG_TRACE_SIMPLE >> @@ -1079,7 +1068,14 @@ static void do_info_cpu_stats(Monitor *mon, const= QDict *qdict) >>=20 >> static void do_trace_print_events(Monitor *mon, const QDict *qdict) >> { >> - trace_print_events((FILE *)mon, &monitor_fprintf); >> + TraceEventStateList *events =3D qmp_trace_event_get_state("*", NULL= ); >> + TraceEventStateList *event; > Blank line here, please, to visually separate declarations and statements. Ok. >> + for (event =3D events; event !=3D NULL; event =3D 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. >> } >>=20 >> 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' } >>=20 >> +# 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 t= he "disable" property). Maybe it will be clearer if I merge them into a trista= te 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 cha= nged. > 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: >>=20 -> { "execute": "rtc-reset-reinjection" } >> <- { "return": {} } >> +EQMP >> + >> + { >> + .name =3D "trace-event-get-state", >> + .args_type =3D "name:s", >> + .mhandler.cmd_new =3D qmp_marshal_input_trace_event_get_state, >> + }, >> + >> +SQMP >> +trace-event-get-state >> +--------------------- >> + >> +Query the state of events. >> + >> +EQMP >> + >> + { >> + .name =3D "trace-event-set-state", >> + .args_type =3D "name:s,state:b,keepgoing:b?", >> + .mhandler.cmd_new =3D qmp_marshal_input_trace_event_set_state, >> + }, >> + >> +SQMP >> +trace-event-set-state >> +--------------------- >> + >> +Set the state of events. >>=20 >> 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) +=3D ftrace.o >> util-obj-$(CONFIG_TRACE_UST) +=3D generated-ust.o >> util-obj-y +=3D control.o >> util-obj-y +=3D generated-tracers.o >> +util-obj-y +=3D 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, Trac= eEvent *ev) >> return NULL; >> } >>=20 >> -void trace_print_events(FILE *stream, fprintf_function stream_printf) >> -{ >> - TraceEventID i; >> - >> - for (i =3D 0; i < trace_event_count(); i++) { >> - TraceEvent *ev =3D 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(TraceEven= t *ev, bool state); >>=20 >>=20 >> /** >> - * 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 NUL= L. >> * Corresponds to commandline option "-trace events=3D...". >> 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=C3=ADs Vilanova >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or l= ater. >> + * 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 =3D {}; >> + TraceEventStateList *prev =3D &dummy; >> + > Please move this blank line... >> + bool found =3D false; >> + TraceEvent *ev =3D NULL; > here, so that it visually separates declarations and statements. > Dead initialization of ev, by the way. >> + while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { >> + found =3D true; >> + TraceEventStateList *elem =3D g_malloc0(sizeof(*elem)); > Declaration follows statement, please don't do that. >> + elem->value =3D g_malloc0(sizeof(*elem->value)); >> + elem->value->name =3D g_strdup(trace_event_get_name(ev)); >> + elem->value->sstatic =3D trace_event_get_state_static(ev); >> + elem->value->sdynamic =3D trace_event_get_state_dynamic(ev); >> + prev->next =3D elem; >> + prev =3D 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 =3D NULL; > TraceEvent *ev; > TraceEventStateList *elem; > while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > elem =3D g_new(TraceEventStateList); > [Initialize *elem...] elem-> next =3D events; > events =3D 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_k= eepgoing, >> + bool keepgoing, Error **errp) >> +{ >> + bool error =3D false; >> + bool found =3D false; >> + TraceEvent *ev =3D NULL; > Dead initialization. Ok. >> + >> + /* Check all selected events are dynamic */ >> + while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { >> + found =3D 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 =3D 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 =3D NULL; > Dead assignment. >> + while ((ev =3D trace_event_pattern(name, ev)) !=3D 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.=20 > reimplement HMP commands in term of the new QMP commands. Will do. Thanks, Lluis --=20 "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