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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 21 Oct 2010 12:21:46 +0530
Prerna Saxena <prerna@linux.vnet.ibm.com> wrote:

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

This equivalence is not always desirable, HMP interfaces are designed
for humans. We have to think what's the best machine interface.

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

Ok.

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

Yes.

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