[Qemu-devel] [Tracing][v4 PATCH 0/2] QMP Query interfaces for tracing

[Qemu-devel] [Tracing][v4 PATCH 0/2] QMP Query interfaces for tracing

[Prerna Saxena]

This patch set introduces three QMP query interfaces for tracing :

* query-trace        : to list current contents of trace-buffer
* query-trace-events : to list all available trace-events with their 
                       state.
* query-trace-file   : to list currently set trace-file with its status.

Changelog :
-----------
Changes v3 -> v4 :
- Add 'query-trace-file' interface to query currently active trace-file.
- Cleanup.

Changes v2 -> v3 :
- Change declarations of st_print_trace_to_qlist() and 
st_print_trace_events_to_qlist() to return QList*

Changes v1 -> v2 :
- Add 'timestamp' field for query-trace output.
- Misc cleanups.


-- 
Prerna Saxena

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

[Qemu-devel] [Tracing][v4 PATCH 1/2] Introduce QMP interfaces

[Prerna Saxena]

[PATCH 1/2] Introduce QMP interfaces :
 - query-trace
 - query-trace-events
 - query-trace-file


Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c     |   53 ++++++++++++++++++++++++++++++++++++++++---
 simpletrace.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 simpletrace.h |    5 ++++
 3 files changed, 123 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 260cc02..c7e1f53 100644
--- a/monitor.c
+++ b/monitor.c
@@ -578,6 +578,11 @@ static void do_trace_file(Monitor *mon, const QDict *qdict)
         help_cmd(mon, "trace-file");
     }
 }
+
+static void do_info_trace_file_to_qmp(Monitor *mon, QObject **ret_data)
+{
+    *ret_data = st_print_file_to_qobject();
+}
 #endif
 
 static void user_monitor_complete(void *opaque, QObject *ret_data)
@@ -945,15 +950,27 @@ static void do_info_cpu_stats(Monitor *mon)
 #endif
 
 #if defined(CONFIG_SIMPLE_TRACE)
-static void do_info_trace(Monitor *mon)
+static void do_info_trace_print(Monitor *mon, const QObject *data)
 {
     st_print_trace((FILE *)mon, &monitor_fprintf);
 }
 
-static void do_info_trace_events(Monitor *mon)
+static void do_info_trace(Monitor *mon, QObject **ret_data)
+{
+    QList *trace_event_list = st_print_trace_to_qlist();
+    *ret_data = QOBJECT(trace_event_list);
+}
+
+static void do_info_trace_events_print(Monitor *mon, const QObject *data)
 {
     st_print_trace_events((FILE *)mon, &monitor_fprintf);
 }
+
+static void do_info_trace_events(Monitor *mon, QObject **ret_data)
+{
+    QList *trace_event_list = st_print_trace_events_to_qlist();
+    *ret_data = QOBJECT(trace_event_list);
+}
 #endif
 
 /**
@@ -2610,14 +2627,16 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show current contents of trace buffer",
-        .mhandler.info = do_info_trace,
+        .user_print = do_info_trace_print,
+        .mhandler.info_new = do_info_trace,
     },
     {
         .name       = "trace-events",
         .args_type  = "",
         .params     = "",
         .help       = "show available trace-events & their state",
-        .mhandler.info = do_info_trace_events,
+        .user_print = do_info_trace_events_print,
+        .mhandler.info_new = do_info_trace_events,
     },
 #endif
     {
@@ -2752,6 +2771,32 @@ static const mon_cmd_t qmp_query_cmds[] = {
         .mhandler.info_async = do_info_balloon,
         .flags      = MONITOR_CMD_ASYNC,
     },
+#if defined(CONFIG_SIMPLE_TRACE)
+    {
+        .name       = "trace",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show current contents of trace buffer",
+        .user_print = do_info_trace_print,
+        .mhandler.info_new = do_info_trace,
+    },
+    {
+        .name       = "trace-events",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show available trace-events & their state",
+        .user_print = do_info_trace_events_print,
+        .mhandler.info_new = do_info_trace_events,
+    },
+    {
+        .name       = "trace-file",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show currently active trace output file and its status",
+        .user_print = monitor_user_noop,
+        .mhandler.info_new = do_info_trace_file_to_qmp,
+    },
+#endif
     { /* NULL */ },
 };
 
diff --git a/simpletrace.c b/simpletrace.c
index deb1e07..d24d6b0 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -220,6 +220,43 @@ void st_print_trace(FILE *stream, int (*stream_printf)(FILE *stream, const char
     }
 }
 
+/**
+ * Add the current contents of trace-buffer as a QList.
+ *
+ */
+QList* st_print_trace_to_qlist(void)
+{
+    QObject *data;
+    QList *tlist;
+    unsigned int i;
+
+    tlist = qlist_new();
+
+    for (i = 0; i < trace_idx; i++) {
+          data = qobject_from_jsonf("{"
+                                    " 'timestamp': %" PRId64 ","
+                                    " 'event': %" PRId64 ","
+                                    " 'arg1': %" PRId64 ","
+                                    " 'arg2': %" PRId64 ","
+                                    " 'arg3': %" PRId64 ","
+                                    " 'arg4': %" PRId64 ","
+                                    " 'arg5': %" PRId64 ","
+                                    " 'arg6': %" PRId64
+                                    "}",
+                                    trace_buf[i].timestamp_ns,
+                                    trace_buf[i].event,
+                                    trace_buf[i].x1,
+                                    trace_buf[i].x2,
+                                    trace_buf[i].x3,
+                                    trace_buf[i].x4,
+                                    trace_buf[i].x5,
+                                    trace_buf[i].x6);
+          qlist_append_obj(tlist, data);
+    }
+
+    return tlist;
+}
+
 void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
 {
     unsigned int i;
@@ -230,6 +267,38 @@ void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, cons
     }
 }
 
+/**
+ * Add current set of trace-events as a QList.
+ *
+ */
+QList* st_print_trace_events_to_qlist(void)
+{
+    QObject *data;
+    QList *tlist;
+    unsigned int i;
+
+    tlist = qlist_new();
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+          data = qobject_from_jsonf("{ 'name': %s, 'event-id': %d, 'state': %d
+                                     }", trace_list[i].tp_name, i,
+                                    trace_list[i].state);
+          qlist_append_obj(tlist, data);
+    }
+
+    return tlist;
+}
+
+/**
+ * Display the current trace-file with its status, as a QObject.
+ *
+ */
+QObject* st_print_file_to_qobject(void)
+{
+    return qobject_from_jsonf("{ 'trace-file': %s, 'status':  %d }",
+                                trace_file_name, trace_file_enabled);
+}
+
 static TraceEvent* find_trace_event_by_name(const char *tname)
 {
     unsigned int i;
diff --git a/simpletrace.h b/simpletrace.h
index 72614ec..76104b0 100644
--- a/simpletrace.h
+++ b/simpletrace.h
@@ -14,6 +14,8 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <stdio.h>
+#include "qdict.h"
+#include "qjson.h"
 
 typedef uint64_t TraceEventID;
 
@@ -36,5 +38,8 @@ void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE *stream,
 void st_set_trace_file_enabled(bool enable);
 bool st_set_trace_file(const char *file);
 void st_flush_trace_buffer(void);
+QList* st_print_trace_to_qlist(void);
+QList* st_print_trace_events_to_qlist(void);
+QObject* st_print_file_to_qobject(void);
 
 #endif /* SIMPLETRACE_H */
-- 
1.7.2.2



-- 
Prerna Saxena

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

[Qemu-devel] Re: [Tracing][v4 PATCH 1/2] Introduce QMP interfaces

[Stefan Hajnoczi]

On Tue, Oct 19, 2010 at 11:55:50AM +0530, Prerna Saxena wrote:
> [PATCH 1/2] Introduce QMP interfaces :
>  - query-trace
>  - query-trace-events
>  - query-trace-file
> 
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  monitor.c     |   53 ++++++++++++++++++++++++++++++++++++++++---
>  simpletrace.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  simpletrace.h |    5 ++++
>  3 files changed, 123 insertions(+), 4 deletions(-)

Acked-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

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

[Prerna Saxena]

[PATCH 2/2] Add documentation for QMP commands:
 - query-trace
 - query-trace-events
 - query-trace-file.


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
+-------------
+
+Show contents of trace buffer.
+
+Returns a set of json-objects containing the following data:
+
+- "event": Event ID for the trace-event(json-int)
+- "timestamp": trace timestamp (json-int)
+- "arg1 .. arg6": Arguments logged by the trace-event (json-int)
+
+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
+       },
+       ...
+   }
+
+EQMP
+
+SQMP
+query-trace-events
+------------------
+
+Show all available trace-events & their state.
+
+Returns a set of json-objects containing the following data:
+
+- "name": Name of Trace-event (json-string)
+- "event-id": Event ID of Trace-event (json-int)
+- "state": State of trace-event [ '0': inactive; '1':active  ] (json-int)
+
+Example:
+
+-> { "execute": "query-trace-events" }
+<- {
+      "return":{
+         "name": "qemu_malloc",
+         "event-id": 0
+         "state": 0,
+      },
+      {
+         "name": "qemu_realloc",
+         "event-id": 1,
+         "state": 0
+      },
+      ...
+   }
+
+EQMP
+
+SQMP
+query-trace-file
+----------------
+
+Display currently set trace file name and its status.
+
+Returns a set of json-objects containing the following data:
+
+- "trace-file": Name of Trace-file (json-string)
+- "status": State of trace-event [ '0': disabled; '1':enabled  ] (json-int)
+
+Example:
+
+-> { "execute": "query-trace-file" }
+<- {
+      "return":{
+         "trace-file": "trace-26609",
+         "status": 1
+      }
+   }
+
+EQMP
-- 
1.7.2.2



-- 
Prerna Saxena

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

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

[Stefan Hajnoczi]

On Tue, Oct 19, 2010 at 11:57:50AM +0530, Prerna Saxena wrote:
> [PATCH 2/2] Add documentation for QMP commands:
>  - query-trace
>  - query-trace-events
>  - query-trace-file.
> 
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  qmp-commands.hx |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 94 insertions(+), 0 deletions(-)

Acked-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

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

[Prerna Saxena]

On 10/19/2010 11:57 AM, Prerna Saxena wrote:
> [PATCH 2/2] Add documentation for QMP commands:
>   - query-trace
>   - query-trace-events
>   - query-trace-file.
>
>

I've been trying ways to avoid building this documentation for other 
trace backends ( since these commands are only available with the 
'simple' backend ). However, looks like hxtool blindly copies text 
between SQMP and EQMP.
I can only think of making hxtool a wee bit intelligent to be able to 
parse CONFIG_* options and build documentation accordingly. Is there a 
workaround I'm missing ?

-- 
Prerna Saxena

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

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

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

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

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

[Luiz Capitulino]

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