[Qemu-devel] Re: [PATCH 2/3] Monitor command 'info trace'

[Prerna Saxena]

Hi Stefan, Jan,
Thanks for taking a look.

On 06/17/2010 08:38 PM, Stefan Hajnoczi wrote:
> On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
> > diff --git a/simpletrace.c b/simpletrace.c
> > index 2fec4d3..239ae3f 100644
> > --- a/simpletrace.c
> > +++ b/simpletrace.c
> > @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned 
> > long x2, unsigned long
> >   void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3, unsigned long x4, unsigned long x5) {
> >       trace(event, x1, x2, x3, x4, x5);
> >   }
> > +
> > +void do_info_trace(Monitor *mon)
> > +{
> > +    unsigned int i, max_idx;
> > +
> > +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
>
> trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
> to perform this test.

I only display the logged contents in the trace buffer (till trace_idx) 
, and not the entire trace buffer. Only when the index is full that the 
entire buffer is displayed.

> > +
> > +    for (i=0; i<max_idx ;i++) {
>
> Whitespace "i=0; i<max_idx ;i++".  "i = 0; i<  max_idx; i++" is pretty
> common across QEMU.

I'll fix this.

> > +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
> > +                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
> > +                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
>
> Getting only numeric output is the limitation of a binary trace.  It
> would probably be possible to pretty-print without much additional code
> by using the format strings from the trace-events file.
>
> I think the numeric dump is good for now though.  Hex is more compact
> than decimal and would make pointers easier to spot.  Want to change
> this?

I agree, but we can let this be a todo till after the first prototype 
goes upstream ?

> > +    }
> > +}
> > diff --git a/tracetool b/tracetool
> > index 9ea9c08..2c73bab 100755
> > --- a/tracetool
> > +++ b/tracetool
> > @@ -130,6 +130,7 @@ void trace2(TraceEvent event, unsigned long x1, unsigned 
> > long x2);
> >   void trace3(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3);
> >   void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3, unsigned long x4);
> >   void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3, unsigned long x4, unsigned long x5);
> > +void do_info_trace(Monitor *mon);
> >   EOF
> >
> >       simple_event_num=0
> > @@ -289,6 +290,7 @@ tracetoh()
> >   #define TRACE_H
> >
> >   #include "qemu-common.h"
> > +#include "monitor.h"
>
> qemu-common.h forward-declares Monitor, I don't think you need
> monitor.h.

Right.

> Stefan

I'll post patches by Monday that addresses your suggestions, and try to 
get it integrated with QMP.

--
Prerna Saxena

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