On Thu, 21 Oct 2010 12:21:46 +0530
Prerna Saxena <pre...@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<pre...@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<pre...@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
> >
>
>
