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

[Lluís Vilanova]

Anything not commented inline has been changed according to your feedback.


Eric Blake writes:
[...]
>> +##
>> +# @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.

I leaned towards having a common prefix for all commands, so that they're easier
to remember/find.


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

It uses the same prefix as the C routines, which are shorter to avoid too much
typing.


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

The command is now 'instr-query', and returns both the available type (none,
static, dynamic), and whether it's currently "active".


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

Hmmm, letting the instrumentation library to interact both ways with QEMU
through QAPI would be an interesting feature. But I'd like to keep this first
implementation as simple as possible regarding this, and instead concentrate on
the instrumentation functionalities.

Such features can be added later, when the main ideas have been seen fit for
landing upstream.


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

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

Yes; the auto-generated code already uses "args" internally. I've fixed it to
allow "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 [ ].

Ok, I thought a list was empty by default when no contents were given to it. It
is now optional.


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

The return structure contains an integer handle, now, together with some other
information.


[...]
>> +++ 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.

I have no strong opinion on either side, so for now I've left it as it is.


Thanks a lot for the review.


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