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

[Luiz Capitulino]

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.

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.

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

> +
> +- "event": Event ID for the trace-event(json-int)

Maybe this should be called event_id or just id.

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

Unit?

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

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

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

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

> +- "state": State of trace-event [ '0': inactive; '1':active  ] (json-int)

This should be a json-bool.

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

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

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

Name or path?

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

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