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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3539 bytes --]

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 <vilanova@ac.upc.edu>
> ---
>  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).

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

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

> +
> +##
> +# @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?

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

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

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]