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

[Prerna Saxena]

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