[Qemu-devel] Re: [Tracing][RFC v2 PATCH 1/2] Introduce 'query-trace' & 'query-trace-events' interfaces

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

On Fri, Oct 15, 2010 at 10:10:10AM +0530, Prerna Saxena wrote:
> [PATCH 1/2] Introduce QMP interfaces : query-trace & query-trace-events
> 
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  monitor.c     |   46 +++++++++++++++++++++++++++++++++++++++++---
>  simpletrace.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  simpletrace.h |    4 +++
>  3 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index fbb678d..7a150ae 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -941,15 +941,33 @@ 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)
>  {
>      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 = NULL;
> +
> +    st_print_trace_to_qlist(&trace_event_list);
> +
> +    *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 = NULL;
> +
> +    st_print_trace_events_to_qlist(&trace_event_list);
> +
> +    *ret_data = QOBJECT(trace_event_list);
> +}
>  #endif
> 
>  /**
> @@ -2606,14 +2624,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
>      {
> @@ -2748,6 +2768,24 @@ 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,
> +    },
> +#endif
>      { /* NULL */ },
>  };
> 
> diff --git a/simpletrace.c b/simpletrace.c
> index f849e42..a964312 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.
> + * NOTE: This assumes trace_list hasnt already been allocated with a QList.
> + *        The initialization happens here.
> + */
> +void st_print_trace_to_qlist(QList **tlist)

These functions can be simplified by changing their prototypes:

QList *st_get_trace_qlist(void)
QList *st_get_trace_events_qlist(void)

This eliminates the possibility for misuse.  The comment and assert can
be dropped.

Sorry I didn't think of this the first time.

> +{
> +    QObject *data;
> +    unsigned int i;
> +
> +    assert(tlist);
> +
> +    *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);
> +    }
> +}
> +
>  void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
>  {
>      unsigned int i;
> @@ -230,6 +267,27 @@ void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, cons
>      }
>  }
> 
> +/**
> + * Add current set of trace-events as a QList.
> + * NOTE: This assumes trace_list hasnt already been allocated with a QList.
> + *        The initialization happens here.
> + */
> +void st_print_trace_events_to_qlist(QList **tlist)
> +{
> +    QObject *data;
> +    unsigned int i;
> +
> +    assert(tlist);
> +
> +    *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);

Why all the spaces in the format string?

> +          qlist_append_obj(*tlist, data);
> +    }
> +}
> +
>  static TraceEvent* find_trace_event_by_name(const char *tname)
>  {
>      unsigned int i;

Stefan