Re: [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control interface

[Eric Blake]

On 04/16/2013 07:51 AM, Lluís Vilanova wrote:
> Add QMP commands to control (un)loading of dynamic instrumentation library.
> 
> Signed-off-by: Lluís Vilanova <address@hidden>
> ---
>  include/qapi/qmp/qerror.h   |    9 +++++
>  instrument/Makefile.objs    |    1 +
>  instrument/qapi-schema.json |   33 ++++++++++++++++++++
>  instrument/qmp.c            |   70 ++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json            |    2 +
>  qmp-commands.hx             |   71 
> +++++++++++++++++++++++++++++++++++++++++++
>  qmp.c                       |    4 ++
>  7 files changed, 190 insertions(+)
>  create mode 100644 instrument/qapi-schema.json
>  create mode 100644 instrument/qmp.c
> 
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 6c0a18d..67b4528 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -129,6 +129,15 @@ void assert_no_error(Error *err);
>  #define QERR_FEATURE_DISABLED \
>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>  
> +#define QERR_INSTR_LOAD_DL \
> +    ERROR_CLASS_GENERIC_ERROR, "Error loading dynamic library: %s"

These days, it is generally easier to just use error_setg() instead of
defining new QERR_ constants, unless you reuse an error so frequently
that having a QERR_ constant ensures that all use sites have consistent
wording of the message.

> +++ b/instrument/qapi-schema.json
> @@ -0,0 +1,33 @@
> +# *-*- Mode: Python -*-*
> +

New files should generally come with a copyright statement and clear
licensing terms.  Not entirely your fault since qapi-schema.json lacks a
copyright statement, but worth thinking about.

> +##
> +# @instr-dynamic:
> +#
> +# Whether dynamic trace instrumentation is available.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-dynamic',
> +  'returns': 'bool' }

Perhaps naming this 'query-instr' is nicer, to match existing qemu
commands that can query state.  Then again, keeping all instrumentation
commands under the intsr- prefix makes them group together, while using
query-instr would not.

Is the abbreviation 'intsr-' appropriate, or should we be using the full
word 'instrument-'?

Is returning a bool going to make it harder to expand this command in
the future to return additional (optional) information, such as the name
of the current loaded library?  Is it worth returning a full-blown
dictionary type, so that we can add other name-value pairs into the
return as needed?  Is it reasonable to give instrumentation libraries a
callback hook that they can use to optionally generate additional
information to be returned as part of this query?

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

Generally a blank line between arguments.

> +# @iargs: arguments to the dynamic instrumentation library

Any reason you named this 'iargs' instead of the simpler 'args'?

> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-load',
> +  'data':    { 'path': 'str', 'iargs': ['String'] } }

There's another thread going on right now about fixing the generator to
let us use the much simpler ['str'] instead of having to go through the
extra layer of the wrapper type 'String'.

@iargs should be optional (#optional in the comment, listed as '*iargs'
in the JSON form); where omitting it is shorthand for [ ].

Should you return a handle representing the library that got loaded?  By
returning nothing, you have permanently limited this command to only
being able to load one library at a time.  But if we return a handle,
and make instr-unload take a handle argument, then even if we choose to
support only one library at a time now, later on, we will have the
option of extending things to support parallel libraries at once without
having to invent new commands.

> +
> +##
> +# @instr-unload:
> +#
> +# Unload the current dynamic instrumentation library.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'instr-unload' }

See above for comments about whether this should take an argument of the
handle returned from the load command.

> diff --git a/instrument/qmp.c b/instrument/qmp.c
> +++ b/qapi-schema.json
> @@ -3513,3 +3513,5 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +input("instrument/qapi-schema.json")

See my comments on 13/23 about possibly naming this include() instead of
input().

> +Example:
> +
> +-> { "execute": "instr-load", "arguments": { "path": 
> "/tmp/libtrace-instrument.so" } }

This example does not match the JSON listed above, because it omits the
mandatory 'iargs':[ ].

> +++ b/qmp.c
> @@ -24,6 +24,10 @@
>  #include "hw/qdev.h"
>  #include "sysemu/blockdev.h"
>  #include "qom/qom-qobject.h"
> +#if defined(TRACE_INSTRUMENT_DYNAMIC)

I don't know whether qemu coding style prefers to use #ifdef foo instead
of #if defined(foo) when there is only a single item being tested for
definedness.

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

signature.asc
Description: OpenPGP digital signature