[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control in
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control interface |
Date: |
Thu, 18 Apr 2013 21:07:49 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5 |
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
- [Qemu-devel] [PATCH v2 13/23] qapi: Add a primitive to include other files from a QAPI schema file, (continued)
- [Qemu-devel] [PATCH v2 13/23] qapi: Add a primitive to include other files from a QAPI schema file, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 02/23] trace: [simple] Do not include "trace/simple.h" in generated tracer headers, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 04/23] tracetool: Use method 'Event.api' to get the name of public routines, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 14/23] [trivial] Set the input root directory when parsing QAPI files, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 10/23] instrument: [dynamic] Call dynamically linked user-provided routines, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 16/23] Let makefiles add entries to the set of target architecture objects, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 01/23] instrument: Add documentation, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control interface, Lluís Vilanova, 2013/04/16
- Re: [Qemu-devel] [PATCH v2 15/23] instrument: [qmp, qapi] Add control interface,
Eric Blake <=
- [Qemu-devel] [PATCH v2 03/23] trace: Let the user specify her own trace-events file, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 11/23] instrument: Add internal control interface, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 05/23] trace: Minimize inclusions of "qemu-common.h" to avoid inclusion loops, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 12/23] instrument: [hmp] Add control interface, Lluís Vilanova, 2013/04/16
- [Qemu-devel] [PATCH v2 23/23] trace: Do not use the word 'new' in event arguments, Lluís Vilanova, 2013/04/16
- Re: [Qemu-devel] [RFC][PATCH v2 00/23] instrument: Let the user wrap/override specific event tracing routines, Lluís Vilanova, 2013/04/16