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

[Markus Armbruster]

Lluís Vilanova <address@hidden> writes:

> 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

Yes, please; it's neater.

>>> +
>>> +##
>>> +# @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).

Please document it.

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

Good enough for me, too.

>>> +
>>> +##
>>> +# @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).

Yes, let's keep this simple.

However, I feel keepgoing is unnecessarily vague.  Its purpose is to
enable use of a pattern in the presence of disabled events.  I'd prefer
to nail it down to exactly that purpose rather than having it cover
arbitrary, unspecified errors.

A few ideas on how to do that:

* Have a flag to modify the semantics of the pattern: either "match all
  events", or "match just disabled and enabled events, not unavailable
  events".

* To find out what a trace-event-set-state actually does, you need to
  trace-event-get-state the same pattern.  We could make
  trace-event-set-state return the events it changed, so you never have
  to trace-event-get-state, but it's probably not worthwhile.

[...]