Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Lluís Vilanova]

Eric Blake writes:

> On 08/21/2014 11:52 AM, Lluís Vilanova wrote:
>> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
>> commands.
>> 
>> 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
>> 

>> +++ b/qapi/trace.json
>> @@ -0,0 +1,44 @@
>> +# -*- mode: python -*-
>> +

> No copyright statement (but you are just following (poor) existing
> practice).

Will add.


>> +##
>> +# @TraceEventState:
>> +#
>> +# State of a tracing event.
>> +#
>> +# @name: Event name.
>> +# @sstatic: Static tracing state.
>> +# @sdynamic: Dynamic tracing state.

> Does the leading 's' add any value?  QAPI doesn't have to use obscure
> abbreviations.

It felt wrong to have a JSON attribute named differently from the C struct
(static is a reserved word).


>> +#
>> +# Since 2.2
>> +##
>> +{ 'type': 'TraceEventState',
>> +  'data': {'name': 'str', 'sstatic': 'bool', 'sdynamic': 'bool'} }

> Are all four states between the combinations of the two bools possible,
> or is this more of a tri-state setting (default, sstatic, or sdynamic),
> in which case a single parameter of enum type would be better than two
> bools?

I can certainly change it to an "state" enum:

* unavailable: statically disabled
* disabled   : statically enabled, dynamically disabled
* enabled    : statically enabled, dynamically enabled


>> +
>> +##
>> +# @trace-event-get-state:
>> +#
>> +# Query the state of events.
>> +#
>> +# @name: Event name pattern.

> glob? regex? Is the pattern anchored or just substring analysis?
> Case-sensitive?

It's the same pattern matching used internally in QEMU (case-sensitive
globbing).


>> +#
>> +# Returns: @TraceEventState of the matched events
>> +#
>> +# Since 2.2
>> +##
>> +{ 'command': 'trace-event-get-state',
>> +  'data': {'name': 'str'},
>> +  'returns': ['TraceEventState'] }

> What if I want ALL events?  Can name be made optional to avoid any
> filtering?

I use "*" as the "name", which seems simple enough to me.


>> +
>> +##
>> +# @trace-event-set-state:
>> +#
>> +# Set the dynamic state of events.
>> +#
>> +# @name: Event name pattern.

> Same comments about pattern as before.

>> +# @state: Dynamic tracing state.
>> +# @keepgoing: #optional Apply changes even if not all events can be changed.

> keep-going - use dash to separate words for legibility

Ok.


>> +#
>> +# Since 2.2
>> +##
>> +{ 'command': 'trace-event-set-state',
>> +  'data': {'name': 'str', 'state': 'bool', '*keepgoing': 'bool'} }

> This only lets me set the state of one name at a time.  Oh, unless I'm
> setting a pattern, and it then sets the state of all names that match
> that pattern.  I'm wondering if you should have 'name':['str'] to allow
> me to set multiple names/patterns in one go, instead of having to call
> the command multiple times; but it's probably not worth the complexity.

I agree with the complexity comment. Also, the keepgoing is useful to set events
using a pattern, even if some of them are statically disabled (otherwise it
gives an error).


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

> No example usage?

Heh, lazy me. Will add.


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