Re: [Qemu-devel] [PATCH 14/22] instrument: [qmp, qapi] Add control interface

[Eric Blake <eblake@redhat.com>]

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

On 03/26/2013 08:01 AM, Lluís Vilanova wrote:
> Add QMP commands to control (un)loading of dynamic instrumentation library.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  include/qapi/qmp/qerror.h   |    9 +++++
>  instrument/Makefile.objs    |    1 +
>  instrument/qapi-schema.json |   33 ++++++++++++++++++++
>  instrument/qmp.c            |   59 ++++++++++++++++++++++++++++++++++++
>  qapi-schema.json            |    2 +
>  qmp-commands.hx             |   71 +++++++++++++++++++++++++++++++++++++++++++
>  qmp.c                       |    4 ++
>  7 files changed, 179 insertions(+)
>  create mode 100644 instrument/qapi-schema.json
>  create mode 100644 instrument/qmp.c
> 

> +++ b/instrument/qapi-schema.json
> @@ -0,0 +1,33 @@
> +# *-*- Mode: Python -*-*
> +
> +##
> +# @instr-dynamic:
> +#
> +# Whether dynamic trace instrumentation is available.
> +#
> +# Since: 1.4

1.5

> +##
> +{ 'command': 'instr-dynamic',
> +  'returns': 'bool' }
> +
> +##
> +# @instr-load:
> +#
> +# Load a dynamic instrumentation library.
> +#
> +# @path: path to the dynamic instrumentation library
> +# @args: arguments to the dynamic instrumentation library

No trailing underscore here...

> +#
> +# Since: 1.4

1.5

> +##
> +{ 'command': 'instr-load',
> +  'data':    { 'path': 'str', 'args_': 'str' } }

...but there is one here.  Something is wrong.  Furthermore, this is
gross - how does the receiver know how to break args into chunks?  This
should be '*args':['str'], taking an optional array of args (omitting is
the same as a 0-length array).

> +##
> +# @instr-unload:
> +#
> +# Unload the current dynamic instrumentation library.
> +#
> +# Since: 1.4

1.5

> +##
> +{ 'command': 'instr-unload' }

Is it possible to load more than one library at a time?  If so, then
instr-load needs to return a handle, and instr-unload needs to take a
handle as an argument.  Also, a query-instr command might be useful for
showing which library (or libraries) are loaded.

> +++ b/instrument/qmp.c
> @@ -0,0 +1,59 @@
> +/*
> + * QMP interface for dynamic trace instrumentation control commands.
> + *
> + * Copyright (C) 2012 Lluís Vilanova <vilanova@ac.upc.edu>

It's 2013, now.

> +++ b/qapi-schema.json
> @@ -2360,6 +2360,8 @@
>  ##
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
> +input("instrument/qapi-schema.json")
> +
>  ##
>  # @dump-guest-memory

Unusual placement, having it in the middle of the file in no particular
alphabetic ordering.  I would typically expect to see includes done at
the beginning, or maybe at the end, or at least in alphabetical order
where the 'instr-*' commands would fall (but the file is already messy,
so not entirely your fault).


> +instr-load
> +----------
> +
> +Load a dynamic instrumentation library.
> +
> +Arguments:
> +
> +- path: path to the dynamic instrumentation library

Missing out on args documentation.

-- 
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: 621 bytes --]