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

[Lluís Vilanova]

Eric Blake writes:

> 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 <address@hidden>
>> ---
>> 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.

Maybe there was a problem with using the word "args"? I honestly do not remember
the reason for this, so I'll just remove the underscore.


>  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).

Ha! Looks like I was really lazy when I implemented this... I'll see if I can
find some time to properly implement it and pass an array of strings to the
instrumentation library.


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

It's not. Right now you'll get an error if that happens. The rationale behind
this is that for multiple libraries to be supported you would need to implement
an instrumentation "driver" that loops through a list of callbacks, which is
slower than just calling into a single function pointer.

I actually thought about writing an "auto-optimizing" version. The idea was to
assign the client's callback to the per-event pointer if only one library
requested that event, and otherwise assign a pointer to a per-event
auto-generated routine that loops through all assigned callbacks when multiple
clients have registered for that event. But this looked like too much trouble
for an initial implementation.

Besides, a single top-level instrumentation library could then provide an
interface to load multiple "child" libraries, if that is necessary.


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

> 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).

This is probably the result of merges, or that I just changed the
instrumentation command names and did not update the position; thanks.

BTW, I think that this "input" primitive should be used wherever possible to
split the files by subsystem and also have each part in the directory where the
implementation resides.


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

> Missing out on args documentation.

Is this one also able to handle a list of strings for "args"?

If so, then I'm just missing how to expose it in the command-line (maybe as an
"accumulable" argument), and we'll be set for having proper argument support.


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth