Markus Armbruster writes: > Lluís Vilanova <[email protected]> writes: >> Signed-off-by: Lluís Vilanova <[email protected]> >> Reviewed-by: Stefan Hajnoczi <[email protected]> >> --- >> monitor.c | 4 +- >> qapi/trace.json | 20 ++++++-- >> qmp-commands.hx | 17 ++++++- >> trace/qmp.c | 143 >> ++++++++++++++++++++++++++++++++++++++++++++----------- >> 4 files changed, 147 insertions(+), 37 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index a27e115..bb89877 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -910,7 +910,7 @@ static void hmp_trace_event(Monitor *mon, const QDict >> *qdict) >> bool new_state = qdict_get_bool(qdict, "option"); >> Error *local_err = NULL; >> >> - qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err); >> + qmp_trace_event_set_state(tp_name, new_state, true, true, false, 0, >> &local_err); >> if (local_err) { >> error_report_err(local_err); >> } >> @@ -1069,7 +1069,7 @@ static void hmp_info_cpustats(Monitor *mon, const >> QDict *qdict) >> >> static void hmp_info_trace_events(Monitor *mon, const QDict *qdict) >> { >> - TraceEventInfoList *events = qmp_trace_event_get_state("*", NULL); >> + TraceEventInfoList *events = qmp_trace_event_get_state("*", false, 0, >> NULL); >> TraceEventInfoList *elem; >> >> for (elem = events; elem != NULL; elem = elem->next) {
> The new feature remains inaccessible in HMP. Any plans to extend HMP? > Any reasons not to? My bad, I forgot about it. I'll see to adding it. >> diff --git a/qapi/trace.json b/qapi/trace.json >> index 01b0a52..25d8095 100644 >> --- a/qapi/trace.json >> +++ b/qapi/trace.json >> @@ -1,6 +1,6 @@ >> # -*- mode: python -*- >> # >> -# Copyright (C) 2011-2014 Lluís Vilanova <[email protected]> >> +# Copyright (C) 2011-2016 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. >> @@ -29,11 +29,12 @@ >> # >> # @name: Event name. >> # @state: Tracing state. >> +# @vcpu: Whether this is a per-vCPU event (since 2.7). >> # >> # Since 2.2 >> ## >> { 'struct': 'TraceEventInfo', >> - 'data': {'name': 'str', 'state': 'TraceEventState'} } >> + 'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} } >> >> ## >> # @trace-event-get-state: >> @@ -41,13 +42,18 @@ >> # Query the state of events. >> # >> # @name: Event name pattern (case-sensitive glob). >> +# @vcpu: #optional The vCPU to check (any by default; since 2.7). >> # >> # Returns: a list of @TraceEventInfo for the matching events >> # >> +# For any event without the "vcpu" property: > All events have the vcpu property, but for some of them the property's > value is false. Hmmm, do you mean in the generated event array? Here, I was referring to wether an event has the "vcpu" property in "trace-events". I can clarify that. >> +# - If @name is a pattern and @vcpu is set, events are ignored. > I figure "ignored" means they're not included in the return value. > Correct? Exactly. >> +# - If @name is not a pattern and @vcpu is set, an error is raised. > Perhaps we could clarify as follows: > # Returns: a list of @TraceEventInfo for the matching events > # > # An event matches if > # - its name matches the @name pattern, and > # - if @vcpu is given, its vCPU equals @vcpu. > # It follows that with @vcpu given, the query can match only per-vCPU > # events. Special case: if the name is an exact match, you get an error > # instead of an empty list. Doesn't sound entirely right to me: # Returns: a list of @TraceEventInfo for the matching events # # An event matches if # - its name matches the @name pattern, and # - if @vcpu is given, the event is a per-vCPU one (has the "vcpu" property in # "trace-events"). # # Therefore, if @vcpu is given, the query will only match per-vCPU events. # Special case: if @name is an exact match and @vcpu is given, you will get an # error if the event does not have the "vcpu" property. >> +# >> # Since 2.2 >> ## >> { 'command': 'trace-event-get-state', >> - 'data': {'name': 'str'}, >> + 'data': {'name': 'str', '*vcpu': 'int'}, >> 'returns': ['TraceEventInfo'] } >> >> ## >> @@ -58,8 +64,14 @@ >> # @name: Event name pattern (case-sensitive glob). >> # @enable: Whether to enable tracing. >> # @ignore-unavailable: #optional Do not match unavailable events with @name. >> +# @vcpu: #optional The vCPU to act upon (all by default; since 2.7). >> +# >> +# For any event without the "vcpu" property: >> +# - If @name is a pattern and @vcpu is set, events are ignored. >> +# - If @name is not a pattern and @vcpu is set, an error is raised. > Likewise. Ok. >> # >> # Since 2.2 >> ## >> { 'command': 'trace-event-set-state', >> - 'data': {'name': 'str', 'enable': 'bool', '*ignore-unavailable': 'bool'} } >> + 'data': {'name': 'str', 'enable': 'bool', '*ignore-unavailable': 'bool', >> + '*vcpu': 'int'} } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 28801a2..c9eb25c 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -4676,7 +4676,7 @@ EQMP >> >> { >> .name = "trace-event-get-state", >> - .args_type = "name:s", >> + .args_type = "name:s,vcpu:i?", >> .mhandler.cmd_new = qmp_marshal_trace_event_get_state, >> }, >> >> @@ -4686,6 +4686,11 @@ trace-event-get-state >> >> Query the state of events. >> >> +Arguments: >> + >> +- "name": Event name pattern (json-string). >> +- "vcpu": Specific vCPU to query, any vCPU by default (json-int, optional). >> + > Less information than you give in the schema. Easy enough to fix. I guess you mean extending the docs here to cover the same info given in the JSON file. >> Example: >> -> { "execute": "trace-event-get-state", "arguments": { "name": "qemu_memalign" } } >> @@ -4694,7 +4699,7 @@ EQMP >> >> { >> .name = "trace-event-set-state", >> - .args_type = "name:s,enable:b,ignore-unavailable:b?", >> + .args_type = "name:s,enable:b,ignore-unavailable:b?,vcpu:i?", >> .mhandler.cmd_new = qmp_marshal_trace_event_set_state, >> }, >> >> @@ -4704,6 +4709,14 @@ trace-event-set-state >> >> Set the state of events. >> >> +Arguments: >> + >> +- "name": Event name pattern (json-string). >> +- "enable": Whether to enable or disable the event (json-bool). >> +- "ignore-unavailable": Whether to ignore errors for events that cannot be >> + changed (json-bool, optional). >> +- "vcpu": Specific vCPU to set, all vCPUs by default (json-int, optional). >> + > Likewise. Ok. >> Example: >> -> { "execute": "trace-event-set-state", "arguments": { "name": "qemu_memalign", "enable": "true" } } >> diff --git a/trace/qmp.c b/trace/qmp.c >> index 8aa2660..c18e750 100644 >> --- a/trace/qmp.c >> +++ b/trace/qmp.c >> @@ -1,7 +1,7 @@ >> /* >> * QMP commands for tracing events. >> * >> - * Copyright (C) 2014 Lluís Vilanova <[email protected]> >> + * Copyright (C) 2014-2016 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. >> @@ -12,63 +12,148 @@ >> #include "trace/control.h" >> >> >> -TraceEventInfoList *qmp_trace_event_get_state(const char *name, Error >> **errp) >> +static bool get_cpu(bool has_vcpu, int vcpu, CPUState **cpu, Error **errp) >> +{ >> + if (has_vcpu) { >> + *cpu = qemu_get_cpu(vcpu); >> + if (*cpu == NULL) { >> + error_setg(errp, "invalid vCPU index %u", vcpu); >> + return false; >> + } >> + } else { >> + *cpu = NULL; >> + } >> + return true; >> +} > This returns three things: a bool (via return value), a CPUState * > (via @cpu) and an Error (via customary @errp). Possible combinations: > * Success with has_vcpu: true, non-null CPU *, no error > * Failure with has_vcpu: false, null CPU *, error set > * Success without has_vcpu: true, null CPU *, no error > Usage: > if (!get_cpu(has_vcpu, vcpu, &cpu, errp)) { > error out... > } > If you dispense with the bool and return the pointer instead, you'd get: > Error *err = NULL; > cpu = get_cpu(has_vcpu, vcpu, &err); > if (err) { > error_propagate(errp, err); > error out... > } > This is more in line with how we use Error elsewhere. Needs more code, > though. Since it's just a local helper function, the choice is yours. Oh, I didn't think of using err to check errors. That seems more readable. Thanks, Lluis
