[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state |
Date: |
Mon, 25 Aug 2014 10:00:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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.
[...]