[Qemu-devel] Re: [Tracing][v4 PATCH 2/2] Add documentation for QMP interfaces

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

Hi Luiz,
Thanks for your feedback.

On 10/21/2010 12:47 AM, Luiz Capitulino wrote:
> On Tue, 19 Oct 2010 11:57:50 +0530
> Prerna Saxena<prerna@linux.vnet.ibm.com>  wrote:
>
>> [PATCH 2/2] Add documentation for QMP commands:
>>   - query-trace
>>   - query-trace-events
>>   - query-trace-file.
>
> Please, split this. Each command should be in a separate patch.
>
>>
>>
>> Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com>
>> ---
>>   qmp-commands.hx |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 94 insertions(+), 0 deletions(-)
>>
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 793cf1c..bc79b55 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1539,3 +1539,97 @@ Example:
>>
>>   EQMP
>>
>> +SQMP
>> +query-trace
>> +-------------
>
> It's recommended to first send documentation patches when adding new QMP
> commands, it can be catastrophic to do both at the same time.
>

Right. I'm posting a set of separate set of documentation patches, where 
we can discuss the interfaces individually.

> So, I'll ignore the code for now and discuss the interface only.
>
> My main question is: What are the expected use cases for this interface in
> the perspective of a QMP client?
>
> I can think of two:
>
>   1. Enabling/Disabling trace points (eg. from a GUI)
>   2. Get trace data to generate trace output or do some kind of analysis
>
> If we're only interested in 1, then we don't need query-trace and if we
> do need query-trace then we'll have to rethink some things as it can be
> automatically flushed.
>

At present, qemu has all trace-events disabled at build-time, by 
default. The trace-events of interest are dynamically enabled using the 
human monitor at run time.  '1' is useful to have, so that a QMP client 
can do the same.

The 'query-trace' interface simply displays the current contents of 
trace-buffer before they are flushed to disk.(equivalent to the 'info 
trace' HMP command )

To enabled logged traces to be forcibly flushed, HMP has a command : 
(qemu)trace-file flush
I'm still working on the QMP interface for the same. ( I'll cover the 
proposed QMP interface in my documentation patchset that I'll be sending 
out shortly.)

>> +
>> +Show contents of trace buffer.
>> +
>> +Returns a set of json-objects containing the following data:
>
> Looks like you return a json-array of json-objects, is that correct?
>

Yes.

>> +
>> +- "event": Event ID for the trace-event(json-int)
>
> Maybe this should be called event_id or just id.
>

I've renamed it to event_id for next patch.

>> +- "timestamp": trace timestamp (json-int)
>
> Unit?

ns. I've corrected the description for the upcoming patchset.

>
>> +- "arg1 .. arg6": Arguments logged by the trace-event (json-int)
>
> Are they positional or named arguments?
>
> If they are positional, you should use a json-array, if we have the
> argument name, then we could be nicer and have a json-object of arguments.
>

These are keyword arguments. I have changed the definition to have these 
enclosed in a QMP object for the next patchset version.

>> +
>> +Example:
>> +
>> +->  { "execute": "query-trace" }
>> +<- {
>> +      "return":{
>> +         "event": 22,
>> +         "timestamp": 129456235912365,
>> +         "arg1": 886
>> +         "arg2": 80,
>> +         "arg3": 0,
>> +         "arg4": 0,
>> +         "arg5": 0,
>> +         "arg6": 0,
>> +       },
>> +       {
>> +         "event": 22,
>> +         "timestamp": 129456235973407,
>> +         "arg1": 886,
>> +         "arg2": 80,
>> +         "arg3": 0,
>> +         "arg4": 0,
>> +         "arg5": 0,
>> +         "arg6": 0
>> +       },
>> +       ...
>> +   }
>
> The example above is invalid json.

I'm guessing this is invalid because it could be denoted as an array of 
objects ?
Corrected in upcoming patchset.

>
>> +
>> +EQMP
>> +
>> +SQMP
>> +query-trace-events
>> +------------------
>> +
>> +Show all available trace-events&  their state.
>> +
>> +Returns a set of json-objects containing the following data:
>
> Again, I believe you want to return a json-array of json-objects.

Agree, corrected this for the documentation patchset.

>
>> +
>> +- "name": Name of Trace-event (json-string)
>> +- "event-id": Event ID of Trace-event (json-int)
>
> query-trace's key is called event, we should use either event_id or just id
> (I think I prefer the former).
>

Renamed to event_id.


>> +- "state": State of trace-event [ '0': inactive; '1':active  ] (json-int)
>
> This should be a json-bool.
>

Done.

>> +
>> +Example:
>> +
>> +->  { "execute": "query-trace-events" }
>> +<- {
>> +      "return":{
>> +         "name": "qemu_malloc",
>> +         "event-id": 0
>> +         "state": 0,
>> +      },
>> +      {
>> +         "name": "qemu_realloc",
>> +         "event-id": 1,
>> +         "state": 0
>> +      },
>> +      ...
>> +   }
>
> This also invalid json.
>

Again, I'm guessing the reason it is invalid that it ought to be an 
array. Changed for next patches.

>> +
>> +EQMP
>> +
>> +SQMP
>> +query-trace-file
>> +----------------
>> +
>> +Display currently set trace file name and its status.
>> +
>> +Returns a set of json-objects containing the following data:
>
> This is actually just one json-object.
>

Oops, typo. Changed !

>> +
>> +- "trace-file": Name of Trace-file (json-string)
>
> Name or path?
>

This ought to display full path -- changed for next patch set.

>> +- "status": State of trace-event [ '0': disabled; '1':enabled  ] (json-int)
>
> This should be a json bool called 'enabled' or 'disabled', but what happens
> when a file is not defined?
>

Changed type to json bool.
The trace infrastructure sets the trace-output file to trace-<PID> ( 
created in current dir) if no explicit trace-file is specified at 
startup. (Users can also change the default trace-file at runtime using 
the hmp command 'trace-file set FILE' I'll be covering QMP interface for 
the same in the upcoming patchset. )

>> +
>> +Example:
>> +
>> +->  { "execute": "query-trace-file" }
>> +<- {
>> +      "return":{
>> +         "trace-file": "trace-26609",
>> +         "status": 1
>> +      }
>> +   }
>> +
>> +EQMP
>


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India