[Qemu-devel] [PATCH][Tracing] Fix build errors for target i386-linux-user

[Qemu-devel] [PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

[PATCH 1/1] Move definitions of monitor command handlers (do_info_trace, 
do_info_all_trace_events) to monitor.c. This removes build errors for 
user targets such as i386-linux-user, which are not linked with monitor.

The export of trace_buf and trace_idx is an unfortunate side effect, 
since these are needed by do_info_trace which dumps buffer 
contents.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c     |   21 +++++++++++++++++++++
 simpletrace.c |   39 ++-------------------------------------
 tracetool     |   16 ++++++++++++++++
 3 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/monitor.c b/monitor.c
index 433a3ec..9b5d65a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -540,6 +540,27 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
     bool new_state = qdict_get_bool(qdict, "option");
     change_trace_event_state(tp_name, new_state);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i;
+
+    for (i = 0; i < trace_idx ; i++) {
+        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
+    }
+}
+
+void do_info_all_trace_events(Monitor *mon)
+{
+    unsigned int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
+                                trace_list[i].tp_name, i, trace_list[i].state);
+    }
+}
 #endif
 
 static void user_monitor_complete(void *opaque, QObject *ret_data)
diff --git a/simpletrace.c b/simpletrace.c
index 57c41fc..834b4c1 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -1,23 +1,9 @@
 #include <stdlib.h>
 #include <stdio.h>
-#include "monitor.h"
 #include "trace.h"
 
-typedef struct {
-    unsigned long event;
-    unsigned long x1;
-    unsigned long x2;
-    unsigned long x3;
-    unsigned long x4;
-    unsigned long x5;
-} TraceRecord;
-
-enum {
-    TRACE_BUF_LEN = 64 * 1024 / sizeof(TraceRecord),
-};
-
-static TraceRecord trace_buf[TRACE_BUF_LEN];
-static unsigned int trace_idx;
+TraceRecord trace_buf[TRACE_BUF_LEN];
+unsigned int trace_idx;
 static FILE *trace_fp;
 
 static void trace(TraceEventID event, unsigned long x1,
@@ -69,27 +55,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
     trace(event, x1, x2, x3, x4, x5);
 }
 
-void do_info_trace(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < trace_idx ; i++) {
-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
-    }
-}
-
-void do_info_all_trace_events(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < NR_TRACE_EVENTS; i++) {
-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
-                                trace_list[i].tp_name, i, trace_list[i].state);
-    }
-}
-
 static TraceEvent* find_trace_event_by_name(const char *tname)
 {
     unsigned int i;
diff --git a/tracetool b/tracetool
index c77280d..c7a690d 100755
--- a/tracetool
+++ b/tracetool
@@ -125,6 +125,22 @@ typedef struct {
     bool state;
 } TraceEvent;
 
+typedef struct {
+    unsigned long event;
+    unsigned long x1;
+    unsigned long x2;
+    unsigned long x3;
+    unsigned long x4;
+    unsigned long x5;
+} TraceRecord;
+
+enum {
+    TRACE_BUF_LEN = 64 * 1024 / sizeof(TraceRecord),
+};
+
+extern TraceRecord trace_buf[TRACE_BUF_LEN];
+extern unsigned int trace_idx;
+
 void trace1(TraceEventID event, unsigned long x1);
 void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
 void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
-- 
1.6.2.5



-- 
Prerna Saxena

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

[Qemu-devel] Re: [PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi]

On Wed, Jun 30, 2010 at 09:11:45PM +0530, Prerna Saxena wrote:
> [PATCH 1/1] Move definitions of monitor command handlers (do_info_trace, 
> do_info_all_trace_events) to monitor.c. This removes build errors for 
> user targets such as i386-linux-user, which are not linked with monitor.
> 
> The export of trace_buf and trace_idx is an unfortunate side effect, 
> since these are needed by do_info_trace which dumps buffer 
> contents.

"git grep monitor_printf" suggests that other source files do their own
monitor printing.  Did you look into alternatives that don't expose the
trace buffer internals?

I feel #ifndef CONFIG_USER_ONLY in simpletrace.c would be would be nicer
than exposing the internals.  (CONFIG_USER_ONLY is just a guess, I
haven't investigated which is the right thing to test for.)

Stefan

> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  monitor.c     |   21 +++++++++++++++++++++
>  simpletrace.c |   39 ++-------------------------------------
>  tracetool     |   16 ++++++++++++++++
>  3 files changed, 39 insertions(+), 37 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 433a3ec..9b5d65a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -540,6 +540,27 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
>      bool new_state = qdict_get_bool(qdict, "option");
>      change_trace_event_state(tp_name, new_state);
>  }
> +
> +void do_info_trace(Monitor *mon)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < trace_idx ; i++) {
> +        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
> +    }
> +}
> +
> +void do_info_all_trace_events(Monitor *mon)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> +                                trace_list[i].tp_name, i, trace_list[i].state);
> +    }
> +}
>  #endif
> 
>  static void user_monitor_complete(void *opaque, QObject *ret_data)
> diff --git a/simpletrace.c b/simpletrace.c
> index 57c41fc..834b4c1 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -1,23 +1,9 @@
>  #include <stdlib.h>
>  #include <stdio.h>
> -#include "monitor.h"
>  #include "trace.h"
> 
> -typedef struct {
> -    unsigned long event;
> -    unsigned long x1;
> -    unsigned long x2;
> -    unsigned long x3;
> -    unsigned long x4;
> -    unsigned long x5;
> -} TraceRecord;
> -
> -enum {
> -    TRACE_BUF_LEN = 64 * 1024 / sizeof(TraceRecord),
> -};
> -
> -static TraceRecord trace_buf[TRACE_BUF_LEN];
> -static unsigned int trace_idx;
> +TraceRecord trace_buf[TRACE_BUF_LEN];
> +unsigned int trace_idx;
>  static FILE *trace_fp;
> 
>  static void trace(TraceEventID event, unsigned long x1,
> @@ -69,27 +55,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>      trace(event, x1, x2, x3, x4, x5);
>  }
> 
> -void do_info_trace(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < trace_idx ; i++) {
> -        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
> -    }
> -}
> -
> -void do_info_all_trace_events(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> -        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> -                                trace_list[i].tp_name, i, trace_list[i].state);
> -    }
> -}
> -
>  static TraceEvent* find_trace_event_by_name(const char *tname)
>  {
>      unsigned int i;
> diff --git a/tracetool b/tracetool
> index c77280d..c7a690d 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -125,6 +125,22 @@ typedef struct {
>      bool state;
>  } TraceEvent;
> 
> +typedef struct {
> +    unsigned long event;
> +    unsigned long x1;
> +    unsigned long x2;
> +    unsigned long x3;
> +    unsigned long x4;
> +    unsigned long x5;
> +} TraceRecord;
> +
> +enum {
> +    TRACE_BUF_LEN = 64 * 1024 / sizeof(TraceRecord),
> +};
> +
> +extern TraceRecord trace_buf[TRACE_BUF_LEN];
> +extern unsigned int trace_idx;
> +
>  void trace1(TraceEventID event, unsigned long x1);
>  void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
>  void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
> -- 
> 1.6.2.5
> 
> 
> 
> -- 
> Prerna Saxena
> 
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
> 

[Qemu-devel] [RFC v2][PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

On Thu, 1 Jul 2010 10:18:41 +0100
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:

> On Wed, Jun 30, 2010 at 09:11:45PM +0530, Prerna Saxena wrote:
> > [PATCH 1/1] Move definitions of monitor command handlers (do_info_trace, 
> > do_info_all_trace_events) to monitor.c. This removes build errors for 
> > user targets such as i386-linux-user, which are not linked with monitor.
> > 
> > The export of trace_buf and trace_idx is an unfortunate side effect, 
> > since these are needed by do_info_trace which dumps buffer 
> > contents.
> 
> "git grep monitor_printf" suggests that other source files do their own
> monitor printing.  Did you look into alternatives that don't expose the
> trace buffer internals?

$grep "monitor_printf" would show this function being used in lot of
places, but all of those include "monitor.h" and are linked to the
monitor object as a part of the build process. I could not find any other 
definition of this function apart from the one in "monitor.c", and a 
dummy definition in qemu_tool.c.

> 
> I feel #ifndef CONFIG_USER_ONLY in simpletrace.c would be would be nicer
> than exposing the internals.  (CONFIG_USER_ONLY is just a guess, I
> haven't investigated which is the right thing to test for.)
> 
> Stefan
> 

Yes, I did explore some alternatives.

The monitor command handlers for tracing need knowledge of buffer internals.
The top-level trace object gets compiled earlier, and all target objects
include this later at link time. None of the user targets (linux-user,
bsd-user, darwin-user) are linked with monitor.o, and consequently the
trace object when linked with these targets still has dangling
references to monitor commands. This flags the linker errors.

Even if I were to enclose the monitor-specific interfaces with a
#ifdef...#endif , it would allow the trace-object to be built
homogenously for all targets -- either with or without the code blocks
enclosed in #ifdef..#endif. I am not being able to see how we can
utilize CONFIG_USER_ONLY here.

There are 3 ways I could think of, to fix this :

a) Do not link the trace object for user targets, that don't link with
monitor.o
Advantage : errors disappear.
Drawback : tracing not available for user targets.

b) Keep the trace buffer definitions intact. In places where the linker
doesn't find the monitor references, put a dummy file that has empty
definitions for these monitor interfaces, and link the target object
with these to remove the dangling references.
Advantage : trace buffer internals are safeguarded.
Drawback : Code & build process gets complicated ; not a clean approach.

c) Separate the trace buffer functions and the monitor interfaces. In
user targets, only the trace object is included which allows
trace-events to be built for these targets as well. Just that, the
monitor support is not available.
Advantage : Clean separation of monitor commands and basic trace
functionality.
Drawback : Trace buffer internals get exposed.

My earlier patch took approach 'c', the one below takes approach 'b'.
This patch flags a compile warning :

exec.o: warning: multiple common of `logfile'
qemu-tool.o: warning: previous common is here

I'm running short of ideas to fix this. Any suggestions would be very 
helpful.

[PATCH] User targets dont include monitor.o. Fix to allow compilation of 
trace object for these.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 Makefile.target |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 493233a..b05e37e 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -86,7 +86,7 @@ $(call set-vpath, $(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR
 QEMU_CFLAGS+=-I$(SRC_PATH)/linux-user -I$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
 obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
       elfload.o linuxload.o uaccess.o gdbstub.o cpu-uname.o \
-      qemu-malloc.o
+      qemu-malloc.o qemu-tool.o
 
 obj-$(TARGET_HAS_BFLT) += flatload.o
 
@@ -124,7 +124,7 @@ LDFLAGS+=-Wl,-segaddr,__STD_PROG_ZONE,0x1000 -image_base 0x0e000000
 LIBS+=-lmx
 
 obj-y = main.o commpage.o machload.o mmap.o signal.o syscall.o thunk.o \
-        gdbstub.o
+        gdbstub.o qemu-tool.o
 
 obj-i386-y += ioport-user.o
 
@@ -146,7 +146,7 @@ $(call set-vpath, $(SRC_PATH)/bsd-user)
 QEMU_CFLAGS+=-I$(SRC_PATH)/bsd-user -I$(SRC_PATH)/bsd-user/$(TARGET_ARCH)
 
 obj-y = main.o bsdload.o elfload.o mmap.o signal.o strace.o syscall.o \
-        gdbstub.o uaccess.o
+        gdbstub.o uaccess.o qemu-tool.o
 
 obj-i386-y += ioport-user.o
 
-- 


-- 
Prerna Saxena

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

[Qemu-devel] Re: [RFC v2][PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi]

On Tue, Jul 06, 2010 at 01:34:17PM +0530, Prerna Saxena wrote:
> On Thu, 1 Jul 2010 10:18:41 +0100
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Jun 30, 2010 at 09:11:45PM +0530, Prerna Saxena wrote:
> > > [PATCH 1/1] Move definitions of monitor command handlers (do_info_trace, 
> > > do_info_all_trace_events) to monitor.c. This removes build errors for 
> > > user targets such as i386-linux-user, which are not linked with monitor.
> > > 
> > > The export of trace_buf and trace_idx is an unfortunate side effect, 
> > > since these are needed by do_info_trace which dumps buffer 
> > > contents.
> > 
> > "git grep monitor_printf" suggests that other source files do their own
> > monitor printing.  Did you look into alternatives that don't expose the
> > trace buffer internals?
> 
> $grep "monitor_printf" would show this function being used in lot of
> places, but all of those include "monitor.h" and are linked to the
> monitor object as a part of the build process. I could not find any other 
> definition of this function apart from the one in "monitor.c", and a 
> dummy definition in qemu_tool.c.
> 
> > 
> > I feel #ifndef CONFIG_USER_ONLY in simpletrace.c would be would be nicer
> > than exposing the internals.  (CONFIG_USER_ONLY is just a guess, I
> > haven't investigated which is the right thing to test for.)
> > 
> > Stefan
> > 
> 
> Yes, I did explore some alternatives.
> 
> The monitor command handlers for tracing need knowledge of buffer internals.
> The top-level trace object gets compiled earlier, and all target objects
> include this later at link time. None of the user targets (linux-user,
> bsd-user, darwin-user) are linked with monitor.o, and consequently the
> trace object when linked with these targets still has dangling
> references to monitor commands. This flags the linker errors.
> 
> Even if I were to enclose the monitor-specific interfaces with a
> #ifdef...#endif , it would allow the trace-object to be built
> homogenously for all targets -- either with or without the code blocks
> enclosed in #ifdef..#endif. I am not being able to see how we can
> utilize CONFIG_USER_ONLY here.
> 
> There are 3 ways I could think of, to fix this :
> 
> a) Do not link the trace object for user targets, that don't link with
> monitor.o
> Advantage : errors disappear.
> Drawback : tracing not available for user targets.
> 
> b) Keep the trace buffer definitions intact. In places where the linker
> doesn't find the monitor references, put a dummy file that has empty
> definitions for these monitor interfaces, and link the target object
> with these to remove the dangling references.
> Advantage : trace buffer internals are safeguarded.
> Drawback : Code & build process gets complicated ; not a clean approach.
> 
> c) Separate the trace buffer functions and the monitor interfaces. In
> user targets, only the trace object is included which allows
> trace-events to be built for these targets as well. Just that, the
> monitor support is not available.
> Advantage : Clean separation of monitor commands and basic trace
> functionality.
> Drawback : Trace buffer internals get exposed.
> 
> My earlier patch took approach 'c', the one below takes approach 'b'.
> This patch flags a compile warning :
> 
> exec.o: warning: multiple common of `logfile'
> qemu-tool.o: warning: previous common is here
> 
> I'm running short of ideas to fix this. Any suggestions would be very 
> helpful.
> 
> [PATCH] User targets dont include monitor.o. Fix to allow compilation of 
> trace object for these.
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  Makefile.target |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile.target b/Makefile.target
> index 493233a..b05e37e 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -86,7 +86,7 @@ $(call set-vpath, $(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR
>  QEMU_CFLAGS+=-I$(SRC_PATH)/linux-user -I$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
>  obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
>        elfload.o linuxload.o uaccess.o gdbstub.o cpu-uname.o \
> -      qemu-malloc.o
> +      qemu-malloc.o qemu-tool.o

qemu-tool.o provides stub implementations of monitor_printf() and other
functions which are not available in qemu-io, qemu-image, and qemu-nbd.

logfile is defined in exec.c, which is not linked into qemu-io but is
linked in for QEMU user targets.  This is the cause of the duplicate
symbol.

I think this approach would work if qemu-tool.c was split into tool
stubs and user stubs.

An alternative to modifying the build is to define an interface for
accessing the "simple" backend trace buffer, e.g.:

/**
 * Pretty-print a trace buffer record
 *
 * @param index     Trace buffer index
 * @return str      Record string (heap allocated) or NULL
 *
 * The caller must free the returned string.  If the index is beyond the
 * last valid trace buffer record, NULL is returned.
 */
char *simpletrace_format_record(unsigned int index)

Then the code in monitor.c can just use simpletrace_format_record() in a
loop to extract formatted trace records.  It doesn't need to know about
trace buffer internals.

Perhaps this is the simplest solution?

> 
>  obj-$(TARGET_HAS_BFLT) += flatload.o
> 
> @@ -124,7 +124,7 @@ LDFLAGS+=-Wl,-segaddr,__STD_PROG_ZONE,0x1000 -image_base 0x0e000000
>  LIBS+=-lmx
> 
>  obj-y = main.o commpage.o machload.o mmap.o signal.o syscall.o thunk.o \
> -        gdbstub.o
> +        gdbstub.o qemu-tool.o
> 
>  obj-i386-y += ioport-user.o
> 
> @@ -146,7 +146,7 @@ $(call set-vpath, $(SRC_PATH)/bsd-user)
>  QEMU_CFLAGS+=-I$(SRC_PATH)/bsd-user -I$(SRC_PATH)/bsd-user/$(TARGET_ARCH)
> 
>  obj-y = main.o bsdload.o elfload.o mmap.o signal.o strace.o syscall.o \
> -        gdbstub.o uaccess.o
> +        gdbstub.o uaccess.o qemu-tool.o
> 
>  obj-i386-y += ioport-user.o
> 
> -- 
> 
> 
> -- 
> Prerna Saxena
> 
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
> 

Stefan

[Qemu-devel] [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

[PATCH] Separate monitor command handler interfaces and tracing internals.


Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
 tracetool     |    7 +++++++
 3 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/monitor.c b/monitor.c
index 433a3ec..1f89938 100644
--- a/monitor.c
+++ b/monitor.c
@@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
     bool new_state = qdict_get_bool(qdict, "option");
     change_trace_event_state(tp_name, new_state);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i;
+    char rec[MAX_TRACE_STR_LEN];
+    unsigned int trace_idx = get_trace_idx();
+
+    for (i = 0; i < trace_idx ; i++) {
+        if (format_trace_string(i, rec)) {
+            monitor_printf(mon, rec);
+        }
+    }
+}
+
+void do_info_all_trace_events(Monitor *mon)
+{
+    unsigned int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
+                                trace_list[i].tp_name, i, trace_list[i].state);
+    }
+}
 #endif
 
 static void user_monitor_complete(void *opaque, QObject *ret_data)
diff --git a/simpletrace.c b/simpletrace.c
index 57c41fc..c7b1e7e 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -1,8 +1,8 @@
 #include <stdlib.h>
 #include <stdio.h>
-#include "monitor.h"
 #include "trace.h"
 
+/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */
 typedef struct {
     unsigned long event;
     unsigned long x1;
@@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
     trace(event, x1, x2, x3, x4, x5);
 }
 
-void do_info_trace(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < trace_idx ; i++) {
-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
-    }
-}
-
-void do_info_all_trace_events(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < NR_TRACE_EVENTS; i++) {
-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
-                                trace_list[i].tp_name, i, trace_list[i].state);
-    }
-}
-
 static TraceEvent* find_trace_event_by_name(const char *tname)
 {
     unsigned int i;
@@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate)
         tp->state = tstate;
     }
 }
+
+/**
+ * Return the current trace index.
+ *
+ */
+unsigned int get_trace_idx(void)
+{
+    return trace_idx;
+}
+
+/**
+ * returns formatted TraceRecord at a given index in the trace buffer.
+ * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
+ * 
+ * @idx : index in the buffer for which trace record is returned.
+ * @trace_str : output string passed.
+ */
+char* format_trace_string(unsigned int idx, char trace_str[])
+{
+    TraceRecord rec;
+    if (idx >= TRACE_BUF_LEN || sizeof(trace_str) >= MAX_TRACE_STR_LEN) {
+        return NULL;
+    }
+    rec = trace_buf[idx];
+    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
+                            rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5);
+    return &trace_str[0];
+}
diff --git a/tracetool b/tracetool
index c77280d..b7a0499 100755
--- a/tracetool
+++ b/tracetool
@@ -125,6 +125,11 @@ typedef struct {
     bool state;
 } TraceEvent;
 
+/* Max size of trace string to be displayed via the monitor.
+ * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
+ */
+#define MAX_TRACE_STR_LEN 100
+
 void trace1(TraceEventID event, unsigned long x1);
 void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
 void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
@@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
 void do_info_trace(Monitor *mon);
 void do_info_all_trace_events(Monitor *mon);
 void change_trace_event_state(const char *tname, bool tstate);
+unsigned int get_trace_idx(void);
+char* format_trace_string(unsigned int idx, char *trace_str);
 EOF
 
     simple_event_num=0
-- 
1.6.2.5



-- 
Prerna Saxena

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

[Qemu-devel] Re: [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi]

On Thu, Jul 08, 2010 at 10:58:58AM +0530, Prerna Saxena wrote:
> [PATCH] Separate monitor command handler interfaces and tracing internals.
> 
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  monitor.c     |   23 +++++++++++++++++++++++
>  simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
>  tracetool     |    7 +++++++
>  3 files changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 433a3ec..1f89938 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
>      bool new_state = qdict_get_bool(qdict, "option");
>      change_trace_event_state(tp_name, new_state);
>  }
> +
> +void do_info_trace(Monitor *mon)
> +{
> +    unsigned int i;
> +    char rec[MAX_TRACE_STR_LEN];
> +    unsigned int trace_idx = get_trace_idx();
> +
> +    for (i = 0; i < trace_idx ; i++) {
> +        if (format_trace_string(i, rec)) {
> +            monitor_printf(mon, rec);
> +        }
> +    }
> +}
> +
> +void do_info_all_trace_events(Monitor *mon)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> +                                trace_list[i].tp_name, i, trace_list[i].state);
> +    }
> +}
>  #endif
> 
>  static void user_monitor_complete(void *opaque, QObject *ret_data)
> diff --git a/simpletrace.c b/simpletrace.c
> index 57c41fc..c7b1e7e 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -1,8 +1,8 @@
>  #include <stdlib.h>
>  #include <stdio.h>
> -#include "monitor.h"
>  #include "trace.h"
> 
> +/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */

I can't see TRACE_REC_SIZE anywhere else in this patch.

>  typedef struct {
>      unsigned long event;
>      unsigned long x1;
> @@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>      trace(event, x1, x2, x3, x4, x5);
>  }
> 
> -void do_info_trace(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < trace_idx ; i++) {
> -        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
> -    }
> -}
> -
> -void do_info_all_trace_events(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> -        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> -                                trace_list[i].tp_name, i, trace_list[i].state);
> -    }
> -}
> -
>  static TraceEvent* find_trace_event_by_name(const char *tname)
>  {
>      unsigned int i;
> @@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate)
>          tp->state = tstate;
>      }
>  }
> +
> +/**
> + * Return the current trace index.
> + *
> + */
> +unsigned int get_trace_idx(void)
> +{
> +    return trace_idx;
> +}

format_trace_string() returns NULL if the index is beyond the last valid
trace record.  monitor.c doesn't need to know how many trace records
there are ahead of time, it can just keep printing until it gets NULL.
I don't feel strongly about this but wanted to mention it.

> +
> +/**
> + * returns formatted TraceRecord at a given index in the trace buffer.
> + * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
> + * 
> + * @idx : index in the buffer for which trace record is returned.
> + * @trace_str : output string passed.
> + */
> +char* format_trace_string(unsigned int idx, char trace_str[])
> +{
> +    TraceRecord rec;
> +    if (idx >= TRACE_BUF_LEN || sizeof(trace_str) >= MAX_TRACE_STR_LEN) {

sizeof(trace_str) == sizeof(char *), not the size of the caller's array in bytes.

The fixed size limit can be eliminated using asprintf(3), which
allocates a string of the right size while doing the string formatting.
The caller of format_trace_string() is then responsible for freeing the
string when they are done with it.

> +        return NULL;
> +    }
> +    rec = trace_buf[idx];

Is it necessary to copy the trace record here?

> +    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
> +                            rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5);
> +    return &trace_str[0];
> +}
> diff --git a/tracetool b/tracetool
> index c77280d..b7a0499 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -125,6 +125,11 @@ typedef struct {
>      bool state;
>  } TraceEvent;
> 
> +/* Max size of trace string to be displayed via the monitor.
> + * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
> + */
> +#define MAX_TRACE_STR_LEN 100
> +
>  void trace1(TraceEventID event, unsigned long x1);
>  void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
>  void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
> @@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>  void do_info_trace(Monitor *mon);
>  void do_info_all_trace_events(Monitor *mon);
>  void change_trace_event_state(const char *tname, bool tstate);
> +unsigned int get_trace_idx(void);
> +char* format_trace_string(unsigned int idx, char *trace_str);

I think we need to choose a prefix like simpletrace_*() or something
more concise (but not "strace_" because it's too confusing).  Other
subsystems tend to do this: pci_*(), ram_*(), etc.

Perhaps let's do it as a separate patch to fix up all of the simple
trace backend.

>  EOF
> 
>      simple_event_num=0
> -- 
> 1.6.2.5
> 
> 
> 
> -- 
> Prerna Saxena
> 
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
> 

Stefan

[Qemu-devel] Re: [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

On 07/08/2010 02:50 PM, Stefan Hajnoczi wrote:
> On Thu, Jul 08, 2010 at 10:58:58AM +0530, Prerna Saxena wrote:
>> [PATCH] Separate monitor command handler interfaces and tracing internals.
>>
>>
>> Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com>
>> ---
>>   monitor.c     |   23 +++++++++++++++++++++++
>>   simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
>>   tracetool     |    7 +++++++
>>   3 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 433a3ec..1f89938 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
>>       bool new_state = qdict_get_bool(qdict, "option");
>>       change_trace_event_state(tp_name, new_state);
>>   }
>> +
>> +void do_info_trace(Monitor *mon)
>> +{
>> +    unsigned int i;
>> +    char rec[MAX_TRACE_STR_LEN];
>> +    unsigned int trace_idx = get_trace_idx();
>> +
>> +    for (i = 0; i<  trace_idx ; i++) {
>> +        if (format_trace_string(i, rec)) {
>> +            monitor_printf(mon, rec);
>> +        }
>> +    }
>> +}
>> +
>> +void do_info_all_trace_events(Monitor *mon)
>> +{
>> +    unsigned int i;
>> +
>> +    for (i = 0; i<  NR_TRACE_EVENTS; i++) {
>> +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
>> +                                trace_list[i].tp_name, i, trace_list[i].state);
>> +    }
>> +}
>>   #endif
>>
>>   static void user_monitor_complete(void *opaque, QObject *ret_data)
>> diff --git a/simpletrace.c b/simpletrace.c
>> index 57c41fc..c7b1e7e 100644
>> --- a/simpletrace.c
>> +++ b/simpletrace.c
>> @@ -1,8 +1,8 @@
>>   #include<stdlib.h>
>>   #include<stdio.h>
>> -#include "monitor.h"
>>   #include "trace.h"
>>
>> +/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */
>
> I can't see TRACE_REC_SIZE anywhere else in this patch.

Oops. This comment must go. The connotation was for MAX_TRACE_STR_LEN to 
be large enough to hold the formatted string, but I'm not sure if there 
is a way to test that.

>
>>   typedef struct {
>>       unsigned long event;
>>       unsigned long x1;
>> @@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>>       trace(event, x1, x2, x3, x4, x5);
>>   }
>>
>> -void do_info_trace(Monitor *mon)
>> -{
>> -    unsigned int i;
>> -
>> -    for (i = 0; i<  trace_idx ; i++) {
>> -        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
>> -    }
>> -}
>> -
>> -void do_info_all_trace_events(Monitor *mon)
>> -{
>> -    unsigned int i;
>> -
>> -    for (i = 0; i<  NR_TRACE_EVENTS; i++) {
>> -        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
>> -                                trace_list[i].tp_name, i, trace_list[i].state);
>> -    }
>> -}
>> -
>>   static TraceEvent* find_trace_event_by_name(const char *tname)
>>   {
>>       unsigned int i;
>> @@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate)
>>           tp->state = tstate;
>>       }
>>   }
>> +
>> +/**
>> + * Return the current trace index.
>> + *
>> + */
>> +unsigned int get_trace_idx(void)
>> +{
>> +    return trace_idx;
>> +}
>
> format_trace_string() returns NULL if the index is beyond the last valid
> trace record.  monitor.c doesn't need to know how many trace records
> there are ahead of time, it can just keep printing until it gets NULL.
> I don't feel strongly about this but wanted to mention it.

format_trace_string() returns NULL when the index passed exceeds the 
size of trace buffer. This function is meant for printing current 
contents of trace buffer, which may be less than the entire buffer size.

>
>> +
>> +/**
>> + * returns formatted TraceRecord at a given index in the trace buffer.
>> + * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
>> + *
>> + * @idx : index in the buffer for which trace record is returned.
>> + * @trace_str : output string passed.
>> + */
>> +char* format_trace_string(unsigned int idx, char trace_str[])
>> +{
>> +    TraceRecord rec;
>> +    if (idx>= TRACE_BUF_LEN || sizeof(trace_str)>= MAX_TRACE_STR_LEN) {
>
> sizeof(trace_str) == sizeof(char *), not the size of the caller's array in bytes.

Hmm, I'll need to scrap off this check.

>
> The fixed size limit can be eliminated using asprintf(3), which
> allocates a string of the right size while doing the string formatting.
> The caller of format_trace_string() is then responsible for freeing the
> string when they are done with it.
>

I am somehow reluctant to allocate memory here and free it somewhere 
else. Calls for memory leaks quite easily in case it gets missed.
I'd rather use stack-allocated arrays that clean up after the call to 
the handler is done.

>> +        return NULL;
>> +    }
>> +    rec = trace_buf[idx];
>
> Is it necessary to copy the trace record here?

In my understanding, this would run in the context of monitor handlers, 
which are executed in a separate thread other than the main qemu 
execution loop. Since sprintf() is a longer operation, considering the 
trace_idx might get incremented in the meantime -- I had thought copying 
the TraceRecord would be achieved more quickly with lesser probability 
of index slipping away. Might be an over-optimization -- pls correct me 
if I'm wrong :-)

>
>> +    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
>> +                            rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5);
>> +    return&trace_str[0];
>> +}
>> diff --git a/tracetool b/tracetool
>> index c77280d..b7a0499 100755
>> --- a/tracetool
>> +++ b/tracetool
>> @@ -125,6 +125,11 @@ typedef struct {
>>       bool state;
>>   } TraceEvent;
>>
>> +/* Max size of trace string to be displayed via the monitor.
>> + * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
>> + */
>> +#define MAX_TRACE_STR_LEN 100
>> +
>>   void trace1(TraceEventID event, unsigned long x1);
>>   void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
>>   void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
>> @@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>>   void do_info_trace(Monitor *mon);
>>   void do_info_all_trace_events(Monitor *mon);
>>   void change_trace_event_state(const char *tname, bool tstate);
>> +unsigned int get_trace_idx(void);
>> +char* format_trace_string(unsigned int idx, char *trace_str);
>
> I think we need to choose a prefix like simpletrace_*() or something
> more concise (but not "strace_" because it's too confusing).  Other
> subsystems tend to do this: pci_*(), ram_*(), etc.
>

Agree, it is useful. However, simpletrace_ is too big for a prefix. 
Maybe st_ works, though it is slightly on the ambiguous side ?

> Perhaps let's do it as a separate patch to fix up all of the simple
> trace backend.
>

Will do.

Thanks,
-- 
Prerna Saxena

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

[Qemu-devel] Re: [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi]

On Thu, Jul 08, 2010 at 04:50:52PM +0530, Prerna Saxena wrote:
> On 07/08/2010 02:50 PM, Stefan Hajnoczi wrote:
> >On Thu, Jul 08, 2010 at 10:58:58AM +0530, Prerna Saxena wrote:
> >>[PATCH] Separate monitor command handler interfaces and tracing internals.
> >>
> >>
> >>Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com>
> >>---
> >>  monitor.c     |   23 +++++++++++++++++++++++
> >>  simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
> >>  tracetool     |    7 +++++++
> >>  3 files changed, 59 insertions(+), 22 deletions(-)
> >>
> >>diff --git a/monitor.c b/monitor.c
> >>index 433a3ec..1f89938 100644
> >>--- a/monitor.c
> >>+++ b/monitor.c
> >>@@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
> >>      bool new_state = qdict_get_bool(qdict, "option");
> >>      change_trace_event_state(tp_name, new_state);
> >>  }
> >>+
> >>+void do_info_trace(Monitor *mon)
> >>+{
> >>+    unsigned int i;
> >>+    char rec[MAX_TRACE_STR_LEN];
> >>+    unsigned int trace_idx = get_trace_idx();
> >>+
> >>+    for (i = 0; i<  trace_idx ; i++) {
> >>+        if (format_trace_string(i, rec)) {
> >>+            monitor_printf(mon, rec);
> >>+        }
> >>+    }
> >>+}
> >>+
> >>+void do_info_all_trace_events(Monitor *mon)
> >>+{
> >>+    unsigned int i;
> >>+
> >>+    for (i = 0; i<  NR_TRACE_EVENTS; i++) {
> >>+        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> >>+                                trace_list[i].tp_name, i, trace_list[i].state);
> >>+    }
> >>+}
> >>  #endif
> >>
> >>  static void user_monitor_complete(void *opaque, QObject *ret_data)
> >>diff --git a/simpletrace.c b/simpletrace.c
> >>index 57c41fc..c7b1e7e 100644
> >>--- a/simpletrace.c
> >>+++ b/simpletrace.c
> >>@@ -1,8 +1,8 @@
> >>  #include<stdlib.h>
> >>  #include<stdio.h>
> >>-#include "monitor.h"
> >>  #include "trace.h"
> >>
> >>+/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */
> >
> >I can't see TRACE_REC_SIZE anywhere else in this patch.
> 
> Oops. This comment must go. The connotation was for
> MAX_TRACE_STR_LEN to be large enough to hold the formatted string,
> but I'm not sure if there is a way to test that.
> 
> >
> >>  typedef struct {
> >>      unsigned long event;
> >>      unsigned long x1;
> >>@@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
> >>      trace(event, x1, x2, x3, x4, x5);
> >>  }
> >>
> >>-void do_info_trace(Monitor *mon)
> >>-{
> >>-    unsigned int i;
> >>-
> >>-    for (i = 0; i<  trace_idx ; i++) {
> >>-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
> >>-    }
> >>-}
> >>-
> >>-void do_info_all_trace_events(Monitor *mon)
> >>-{
> >>-    unsigned int i;
> >>-
> >>-    for (i = 0; i<  NR_TRACE_EVENTS; i++) {
> >>-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> >>-                                trace_list[i].tp_name, i, trace_list[i].state);
> >>-    }
> >>-}
> >>-
> >>  static TraceEvent* find_trace_event_by_name(const char *tname)
> >>  {
> >>      unsigned int i;
> >>@@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate)
> >>          tp->state = tstate;
> >>      }
> >>  }
> >>+
> >>+/**
> >>+ * Return the current trace index.
> >>+ *
> >>+ */
> >>+unsigned int get_trace_idx(void)
> >>+{
> >>+    return trace_idx;
> >>+}
> >
> >format_trace_string() returns NULL if the index is beyond the last valid
> >trace record.  monitor.c doesn't need to know how many trace records
> >there are ahead of time, it can just keep printing until it gets NULL.
> >I don't feel strongly about this but wanted to mention it.
> 
> format_trace_string() returns NULL when the index passed exceeds the
> size of trace buffer. This function is meant for printing current
> contents of trace buffer, which may be less than the entire buffer
> size.

Sorry, you're right the patch will return NULL if the index exceeds the
size of the trace buffer.

The idea I was suggesting requires it to return NULL when the index >=
trace_idx.

> >
> >>+
> >>+/**
> >>+ * returns formatted TraceRecord at a given index in the trace buffer.
> >>+ * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
> >>+ *
> >>+ * @idx : index in the buffer for which trace record is returned.
> >>+ * @trace_str : output string passed.
> >>+ */
> >>+char* format_trace_string(unsigned int idx, char trace_str[])
> >>+{
> >>+    TraceRecord rec;
> >>+    if (idx>= TRACE_BUF_LEN || sizeof(trace_str)>= MAX_TRACE_STR_LEN) {
> >
> >sizeof(trace_str) == sizeof(char *), not the size of the caller's array in bytes.
> 
> Hmm, I'll need to scrap off this check.
> 
> >
> >The fixed size limit can be eliminated using asprintf(3), which
> >allocates a string of the right size while doing the string formatting.
> >The caller of format_trace_string() is then responsible for freeing the
> >string when they are done with it.
> >
> 
> I am somehow reluctant to allocate memory here and free it somewhere
> else. Calls for memory leaks quite easily in case it gets missed.
> I'd rather use stack-allocated arrays that clean up after the call
> to the handler is done.

Okay.

> 
> >>+        return NULL;
> >>+    }
> >>+    rec = trace_buf[idx];
> >
> >Is it necessary to copy the trace record here?
> 
> In my understanding, this would run in the context of monitor
> handlers, which are executed in a separate thread other than the
> main qemu execution loop. Since sprintf() is a longer operation,
> considering the trace_idx might get incremented in the meantime -- I
> had thought copying the TraceRecord would be achieved more quickly
> with lesser probability of index slipping away. Might be an
> over-optimization -- pls correct me if I'm wrong :-)

I haven't read the monitor code but I'd expect it to be executed in the
iothread like all other asynchronous IO handlers on file descriptors.  A
quick dig through monitor.c and qemu-char.c suggests that that is uses
qemu_set_fd_handler() and will therefore be called with the qemu mutex
held.

> 
> >
> >>+    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
> >>+                            rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5);
> >>+    return&trace_str[0];
> >>+}
> >>diff --git a/tracetool b/tracetool
> >>index c77280d..b7a0499 100755
> >>--- a/tracetool
> >>+++ b/tracetool
> >>@@ -125,6 +125,11 @@ typedef struct {
> >>      bool state;
> >>  } TraceEvent;
> >>
> >>+/* Max size of trace string to be displayed via the monitor.
> >>+ * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
> >>+ */
> >>+#define MAX_TRACE_STR_LEN 100
> >>+
> >>  void trace1(TraceEventID event, unsigned long x1);
> >>  void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
> >>  void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
> >>@@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
> >>  void do_info_trace(Monitor *mon);
> >>  void do_info_all_trace_events(Monitor *mon);
> >>  void change_trace_event_state(const char *tname, bool tstate);
> >>+unsigned int get_trace_idx(void);
> >>+char* format_trace_string(unsigned int idx, char *trace_str);
> >
> >I think we need to choose a prefix like simpletrace_*() or something
> >more concise (but not "strace_" because it's too confusing).  Other
> >subsystems tend to do this: pci_*(), ram_*(), etc.
> >
> 
> Agree, it is useful. However, simpletrace_ is too big for a prefix.
> Maybe st_ works, though it is slightly on the ambiguous side ?

Cool, st_ works for me.

> 
> >Perhaps let's do it as a separate patch to fix up all of the simple
> >trace backend.
> >
> 
> Will do.
> 
> Thanks,
> -- 
> Prerna Saxena
> 
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India

Stefan

[Qemu-devel] [RFC v4][PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

[PATCH] Separate monitor command handler interfaces and tracing internals.

Changelog from v3 :
1. Cleanups.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   52 ++++++++++++++++++++++++++++++----------------------
 tracetool     |    7 +++++++
 3 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/monitor.c b/monitor.c
index 433a3ec..1f89938 100644
--- a/monitor.c
+++ b/monitor.c
@@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
     bool new_state = qdict_get_bool(qdict, "option");
     change_trace_event_state(tp_name, new_state);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i;
+    char rec[MAX_TRACE_STR_LEN];
+    unsigned int trace_idx = get_trace_idx();
+
+    for (i = 0; i < trace_idx ; i++) {
+        if (format_trace_string(i, rec)) {
+            monitor_printf(mon, rec);
+        }
+    }
+}
+
+void do_info_all_trace_events(Monitor *mon)
+{
+    unsigned int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
+                                trace_list[i].tp_name, i, trace_list[i].state);
+    }
+}
 #endif
 
 static void user_monitor_complete(void *opaque, QObject *ret_data)
diff --git a/simpletrace.c b/simpletrace.c
index 57c41fc..78507ec 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -1,8 +1,8 @@
 #include <stdlib.h>
 #include <stdio.h>
-#include "monitor.h"
 #include "trace.h"
 
+/* Remember to update MAX_TRACE_STR_LEN when changing TraceRecord structure */
 typedef struct {
     unsigned long event;
     unsigned long x1;
@@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
     trace(event, x1, x2, x3, x4, x5);
 }
 
-void do_info_trace(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < trace_idx ; i++) {
-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
-    }
-}
-
-void do_info_all_trace_events(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < NR_TRACE_EVENTS; i++) {
-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
-                                trace_list[i].tp_name, i, trace_list[i].state);
-    }
-}
-
 static TraceEvent* find_trace_event_by_name(const char *tname)
 {
     unsigned int i;
@@ -115,3 +94,32 @@ void change_trace_event_state(const char *tname, bool tstate)
         tp->state = tstate;
     }
 }
+
+/**
+ * Return the current trace index.
+ *
+ */
+unsigned int get_trace_idx(void)
+{
+    return trace_idx;
+}
+
+/**
+ * returns formatted TraceRecord at a given index in the trace buffer.
+ * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
+ * 
+ * @idx : index in the buffer for which trace record is returned.
+ * @trace_str : output string passed.
+ */
+char* format_trace_string(unsigned int idx, char trace_str[])
+{
+    TraceRecord rec;
+    if (idx >= TRACE_BUF_LEN) {
+        return NULL;
+    }
+    rec = trace_buf[idx];
+    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
+                     trace_buf[idx].event, trace_buf[idx].x1, trace_buf[idx].x2,
+                       trace_buf[idx].x3, trace_buf[idx].x4, trace_buf[idx].x5);
+    return &trace_str[0];
+}
diff --git a/tracetool b/tracetool
index c77280d..b7a0499 100755
--- a/tracetool
+++ b/tracetool
@@ -125,6 +125,11 @@ typedef struct {
     bool state;
 } TraceEvent;
 
+/* Max size of trace string to be displayed via the monitor.
+ * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
+ */
+#define MAX_TRACE_STR_LEN 100
+
 void trace1(TraceEventID event, unsigned long x1);
 void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
 void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
@@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
 void do_info_trace(Monitor *mon);
 void do_info_all_trace_events(Monitor *mon);
 void change_trace_event_state(const char *tname, bool tstate);
+unsigned int get_trace_idx(void);
+char* format_trace_string(unsigned int idx, char *trace_str);
 EOF
 
     simple_event_num=0
-- 
1.6.2.5



-- 
Prerna Saxena

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

Re: [Qemu-devel] [RFC v4][PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi]

On Fri, Jul 9, 2010 at 12:30 PM, Prerna Saxena
<prerna@linux.vnet.ibm.com> wrote:
> [PATCH] Separate monitor command handler interfaces and tracing internals.
>
> Changelog from v3 :
> 1. Cleanups.
>
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  monitor.c     |   23 +++++++++++++++++++++++
>  simpletrace.c |   52 ++++++++++++++++++++++++++++++----------------------
>  tracetool     |    7 +++++++
>  3 files changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 433a3ec..1f89938 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
>     bool new_state = qdict_get_bool(qdict, "option");
>     change_trace_event_state(tp_name, new_state);
>  }
> +
> +void do_info_trace(Monitor *mon)
> +{
> +    unsigned int i;
> +    char rec[MAX_TRACE_STR_LEN];
> +    unsigned int trace_idx = get_trace_idx();
> +
> +    for (i = 0; i < trace_idx ; i++) {
> +        if (format_trace_string(i, rec)) {
> +            monitor_printf(mon, rec);
> +        }
> +    }
> +}
> +
> +void do_info_all_trace_events(Monitor *mon)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> +                                trace_list[i].tp_name, i, trace_list[i].state);
> +    }
> +}
>  #endif
>
>  static void user_monitor_complete(void *opaque, QObject *ret_data)
> diff --git a/simpletrace.c b/simpletrace.c
> index 57c41fc..78507ec 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -1,8 +1,8 @@
>  #include <stdlib.h>
>  #include <stdio.h>
> -#include "monitor.h"
>  #include "trace.h"
>
> +/* Remember to update MAX_TRACE_STR_LEN when changing TraceRecord structure */
>  typedef struct {
>     unsigned long event;
>     unsigned long x1;
> @@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>     trace(event, x1, x2, x3, x4, x5);
>  }
>
> -void do_info_trace(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < trace_idx ; i++) {
> -        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
> -    }
> -}
> -
> -void do_info_all_trace_events(Monitor *mon)
> -{
> -    unsigned int i;
> -
> -    for (i = 0; i < NR_TRACE_EVENTS; i++) {
> -        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> -                                trace_list[i].tp_name, i, trace_list[i].state);
> -    }
> -}
> -
>  static TraceEvent* find_trace_event_by_name(const char *tname)
>  {
>     unsigned int i;
> @@ -115,3 +94,32 @@ void change_trace_event_state(const char *tname, bool tstate)
>         tp->state = tstate;
>     }
>  }
> +
> +/**
> + * Return the current trace index.
> + *
> + */
> +unsigned int get_trace_idx(void)
> +{
> +    return trace_idx;
> +}
> +
> +/**
> + * returns formatted TraceRecord at a given index in the trace buffer.
> + * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
> + *
> + * @idx : index in the buffer for which trace record is returned.
> + * @trace_str : output string passed.
> + */
> +char* format_trace_string(unsigned int idx, char trace_str[])
> +{
> +    TraceRecord rec;
> +    if (idx >= TRACE_BUF_LEN) {
> +        return NULL;
> +    }
> +    rec = trace_buf[idx];

rec is unused.

> +    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
> +                     trace_buf[idx].event, trace_buf[idx].x1, trace_buf[idx].x2,
> +                       trace_buf[idx].x3, trace_buf[idx].x4, trace_buf[idx].x5);
> +    return &trace_str[0];
> +}
> diff --git a/tracetool b/tracetool
> index c77280d..b7a0499 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -125,6 +125,11 @@ typedef struct {
>     bool state;
>  } TraceEvent;
>
> +/* Max size of trace string to be displayed via the monitor.
> + * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
> + */
> +#define MAX_TRACE_STR_LEN 100
> +
>  void trace1(TraceEventID event, unsigned long x1);
>  void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
>  void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
> @@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>  void do_info_trace(Monitor *mon);
>  void do_info_all_trace_events(Monitor *mon);
>  void change_trace_event_state(const char *tname, bool tstate);
> +unsigned int get_trace_idx(void);
> +char* format_trace_string(unsigned int idx, char *trace_str);
>  EOF
>
>     simple_event_num=0
> --
> 1.6.2.5
>
>
>
> --
> Prerna Saxena
>
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
>
>
>

[Qemu-devel] [RFC v5[PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

[PATCH] Separate monitor command handler interfaces and tracing internals.

Changelog from v3:
 - cleanup ( removed unnecessary references to 'rec' )

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   50 ++++++++++++++++++++++++++++----------------------
 tracetool     |    7 +++++++
 3 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/monitor.c b/monitor.c
index 433a3ec..1f89938 100644
--- a/monitor.c
+++ b/monitor.c
@@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
     bool new_state = qdict_get_bool(qdict, "option");
     change_trace_event_state(tp_name, new_state);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i;
+    char rec[MAX_TRACE_STR_LEN];
+    unsigned int trace_idx = get_trace_idx();
+
+    for (i = 0; i < trace_idx ; i++) {
+        if (format_trace_string(i, rec)) {
+            monitor_printf(mon, rec);
+        }
+    }
+}
+
+void do_info_all_trace_events(Monitor *mon)
+{
+    unsigned int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
+                                trace_list[i].tp_name, i, trace_list[i].state);
+    }
+}
 #endif
 
 static void user_monitor_complete(void *opaque, QObject *ret_data)
diff --git a/simpletrace.c b/simpletrace.c
index 57c41fc..9e3b46c 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -1,8 +1,8 @@
 #include <stdlib.h>
 #include <stdio.h>
-#include "monitor.h"
 #include "trace.h"
 
+/* Remember to update MAX_TRACE_STR_LEN when changing TraceRecord structure */
 typedef struct {
     unsigned long event;
     unsigned long x1;
@@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
     trace(event, x1, x2, x3, x4, x5);
 }
 
-void do_info_trace(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < trace_idx ; i++) {
-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
-    }
-}
-
-void do_info_all_trace_events(Monitor *mon)
-{
-    unsigned int i;
-
-    for (i = 0; i < NR_TRACE_EVENTS; i++) {
-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
-                                trace_list[i].tp_name, i, trace_list[i].state);
-    }
-}
-
 static TraceEvent* find_trace_event_by_name(const char *tname)
 {
     unsigned int i;
@@ -115,3 +94,30 @@ void change_trace_event_state(const char *tname, bool tstate)
         tp->state = tstate;
     }
 }
+
+/**
+ * Return the current trace index.
+ *
+ */
+unsigned int get_trace_idx(void)
+{
+    return trace_idx;
+}
+
+/**
+ * returns formatted TraceRecord at a given index in the trace buffer.
+ * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
+ * 
+ * @idx : index in the buffer for which trace record is returned.
+ * @trace_str : output string passed.
+ */
+char* format_trace_string(unsigned int idx, char trace_str[])
+{
+    if (idx >= TRACE_BUF_LEN) {
+        return NULL;
+    }
+    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
+                     trace_buf[idx].event, trace_buf[idx].x1, trace_buf[idx].x2,
+                       trace_buf[idx].x3, trace_buf[idx].x4, trace_buf[idx].x5);
+    return &trace_str[0];
+}
diff --git a/tracetool b/tracetool
index c77280d..b7a0499 100755
--- a/tracetool
+++ b/tracetool
@@ -125,6 +125,11 @@ typedef struct {
     bool state;
 } TraceEvent;
 
+/* Max size of trace string to be displayed via the monitor.
+ * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
+ */
+#define MAX_TRACE_STR_LEN 100
+
 void trace1(TraceEventID event, unsigned long x1);
 void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
 void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
@@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
 void do_info_trace(Monitor *mon);
 void do_info_all_trace_events(Monitor *mon);
 void change_trace_event_state(const char *tname, bool tstate);
+unsigned int get_trace_idx(void);
+char* format_trace_string(unsigned int idx, char *trace_str);
 EOF
 
     simple_event_num=0
-- 
1.6.2.5



-- 
Prerna Saxena

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

[Qemu-devel] [PATCH] trace: Remove monitor.h dependency from simpletrace

[Stefan Hajnoczi]

User-mode targets don't have a monitor so the simple trace backend
currently does not build on those targets.  This patch abstracts the
monitor printing interface so there is no direct coupling between
simpletrace and the monitor.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
I started reading the monitor code today and realized that there is already a
solution to this problem.  The monitor printing interface uses FILE* instead of
Monitor*, allowing QEMU subsystems to contain monitor printing code that thinks
it is doing standard library stream I/O.  This idiom is used by
cpu_dump_state(), dump_exec_info(), and others.

 monitor.c     |   14 +++++++++++++-
 simpletrace.c |   17 ++++++++---------
 tracetool     |    4 ++--
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index 433a3ec..090e13c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -920,6 +920,18 @@ static void do_info_cpu_stats(Monitor *mon)
 }
 #endif
 
+#if defined(CONFIG_SIMPLE_TRACE)
+static void do_info_trace(Monitor *mon)
+{
+    st_print_trace((FILE *)mon, &monitor_fprintf);
+}
+
+static void do_info_trace_events(Monitor *mon)
+{
+    st_print_trace_events((FILE *)mon, &monitor_fprintf);
+}
+#endif
+
 /**
  * do_quit(): Quit QEMU execution
  */
@@ -2584,7 +2596,7 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show available trace-events & their state",
-        .mhandler.info = do_info_all_trace_events,
+        .mhandler.info = do_info_trace_events,
     },
 #endif
     {
diff --git a/simpletrace.c b/simpletrace.c
index 4a4203e..7094f4b 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -1,7 +1,6 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <time.h>
-#include "monitor.h"
 #include "trace.h"
 
 typedef struct {
@@ -95,24 +94,24 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
     trace(event, x1, x2, x3, x4, x5);
 }
 
-void do_info_trace(Monitor *mon)
+void st_print_trace(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
 {
     unsigned int i;
 
-    for (i = 0; i < trace_idx ; i++) {
-        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
+    for (i = 0; i < trace_idx; i++) {
+        stream_printf(stream, "Event %lu : %lx %lx %lx %lx %lx\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);
     }
 }
 
-void do_info_all_trace_events(Monitor *mon)
+void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
 {
     unsigned int i;
 
     for (i = 0; i < NR_TRACE_EVENTS; i++) {
-        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
-                                trace_list[i].tp_name, i, trace_list[i].state);
+        stream_printf(stream, "%s [Event ID %u] : state %u\n",
+                      trace_list[i].tp_name, i, trace_list[i].state);
     }
 }
 
diff --git a/tracetool b/tracetool
index 43757e3..8d8f27c 100755
--- a/tracetool
+++ b/tracetool
@@ -139,8 +139,8 @@ void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
 void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
 void trace4(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4);
 void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5);
-void do_info_trace(Monitor *mon);
-void do_info_all_trace_events(Monitor *mon);
+void st_print_trace(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...));
+void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...));
 void change_trace_event_state(const char *tname, bool tstate);
 EOF
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH] trace: Remove monitor.h dependency from simpletrace

[Stefan Hajnoczi]

I forgot to mention this patch is against the tracing branch, not qemu.git:

http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/tracing

Stefan

[Qemu-devel] Re: [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user

[Prerna Saxena]

On 07/08/2010 07:04 PM, Stefan Hajnoczi wrote:
> On Thu, Jul 08, 2010 at 04:50:52PM +0530, Prerna Saxena wrote:
>> On 07/08/2010 02:50 PM, Stefan Hajnoczi wrote:
>>> On Thu, Jul 08, 2010 at 10:58:58AM +0530, Prerna Saxena wrote:
>>>> [PATCH] Separate monitor command handler interfaces and tracing internals.
>>>>
>>>>
>>>> Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com>
>>>> ---
>>>>   monitor.c     |   23 +++++++++++++++++++++++
>>>>   simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
>>>>   tracetool     |    7 +++++++
>>>>   3 files changed, 59 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 433a3ec..1f89938 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
>>>>       bool new_state = qdict_get_bool(qdict, "option");
>>>>       change_trace_event_state(tp_name, new_state);
>>>>   }
>>>> +
>>>> +void do_info_trace(Monitor *mon)
>>>> +{
>>>> +    unsigned int i;
>>>> +    char rec[MAX_TRACE_STR_LEN];
>>>> +    unsigned int trace_idx = get_trace_idx();
>>>> +
>>>> +    for (i = 0; i<   trace_idx ; i++) {
>>>> +        if (format_trace_string(i, rec)) {
>>>> +            monitor_printf(mon, rec);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +void do_info_all_trace_events(Monitor *mon)
>>>> +{
>>>> +    unsigned int i;
>>>> +
>>>> +    for (i = 0; i<   NR_TRACE_EVENTS; i++) {
>>>> +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
>>>> +                                trace_list[i].tp_name, i, trace_list[i].state);
>>>> +    }
>>>> +}
>>>>   #endif
>>>>
>>>>   static void user_monitor_complete(void *opaque, QObject *ret_data)
>>>> diff --git a/simpletrace.c b/simpletrace.c
>>>> index 57c41fc..c7b1e7e 100644
>>>> --- a/simpletrace.c
>>>> +++ b/simpletrace.c
>>>> @@ -1,8 +1,8 @@
>>>>   #include<stdlib.h>
>>>>   #include<stdio.h>
>>>> -#include "monitor.h"
>>>>   #include "trace.h"
>>>>
>>>> +/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */
>>>
>>> I can't see TRACE_REC_SIZE anywhere else in this patch.
>>
>> Oops. This comment must go. The connotation was for
>> MAX_TRACE_STR_LEN to be large enough to hold the formatted string,
>> but I'm not sure if there is a way to test that.
>>

Done in v4.

>>>
>>>>   typedef struct {
>>>>       unsigned long event;
>>>>       unsigned long x1;
>>>> @@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>>>>       trace(event, x1, x2, x3, x4, x5);
>>>>   }
>>>>
>>>> -void do_info_trace(Monitor *mon)
>>>> -{
>>>> -    unsigned int i;
>>>> -
>>>> -    for (i = 0; i<   trace_idx ; i++) {
>>>> -        monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\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);
>>>> -    }
>>>> -}
>>>> -
>>>> -void do_info_all_trace_events(Monitor *mon)
>>>> -{
>>>> -    unsigned int i;
>>>> -
>>>> -    for (i = 0; i<   NR_TRACE_EVENTS; i++) {
>>>> -        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
>>>> -                                trace_list[i].tp_name, i, trace_list[i].state);
>>>> -    }
>>>> -}
>>>> -
>>>>   static TraceEvent* find_trace_event_by_name(const char *tname)
>>>>   {
>>>>       unsigned int i;
>>>> @@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate)
>>>>           tp->state = tstate;
>>>>       }
>>>>   }
>>>> +
>>>> +/**
>>>> + * Return the current trace index.
>>>> + *
>>>> + */
>>>> +unsigned int get_trace_idx(void)
>>>> +{
>>>> +    return trace_idx;
>>>> +}
>>>
>>> format_trace_string() returns NULL if the index is beyond the last valid
>>> trace record.  monitor.c doesn't need to know how many trace records
>>> there are ahead of time, it can just keep printing until it gets NULL.
>>> I don't feel strongly about this but wanted to mention it.
>>
>> format_trace_string() returns NULL when the index passed exceeds the
>> size of trace buffer. This function is meant for printing current
>> contents of trace buffer, which may be less than the entire buffer
>> size.
>
> Sorry, you're right the patch will return NULL if the index exceeds the
> size of the trace buffer.
>
> The idea I was suggesting requires it to return NULL when the index>=
> trace_idx.
>

I've tried to keep this as generic as possible. get_trace_idx() can be 
put to use to query state of trace buffer in different scenarios.

>>>
>>>> +
>>>> +/**
>>>> + * returns formatted TraceRecord at a given index in the trace buffer.
>>>> + * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n"
>>>> + *
>>>> + * @idx : index in the buffer for which trace record is returned.
>>>> + * @trace_str : output string passed.
>>>> + */
>>>> +char* format_trace_string(unsigned int idx, char trace_str[])
>>>> +{
>>>> +    TraceRecord rec;
>>>> +    if (idx>= TRACE_BUF_LEN || sizeof(trace_str)>= MAX_TRACE_STR_LEN) {
>>>
>>> sizeof(trace_str) == sizeof(char *), not the size of the caller's array in bytes.
>>
>> Hmm, I'll need to scrap off this check.
>>

Done.

>>>
>>> The fixed size limit can be eliminated using asprintf(3), which
>>> allocates a string of the right size while doing the string formatting.
>>> The caller of format_trace_string() is then responsible for freeing the
>>> string when they are done with it.
>>>
>>
>> I am somehow reluctant to allocate memory here and free it somewhere
>> else. Calls for memory leaks quite easily in case it gets missed.
>> I'd rather use stack-allocated arrays that clean up after the call
>> to the handler is done.
>
> Okay.
>
>>
>>>> +        return NULL;
>>>> +    }
>>>> +    rec = trace_buf[idx];
>>>
>>> Is it necessary to copy the trace record here?
>>
>> In my understanding, this would run in the context of monitor
>> handlers, which are executed in a separate thread other than the
>> main qemu execution loop. Since sprintf() is a longer operation,
>> considering the trace_idx might get incremented in the meantime -- I
>> had thought copying the TraceRecord would be achieved more quickly
>> with lesser probability of index slipping away. Might be an
>> over-optimization -- pls correct me if I'm wrong :-)
>
> I haven't read the monitor code but I'd expect it to be executed in the
> iothread like all other asynchronous IO handlers on file descriptors.  A
> quick dig through monitor.c and qemu-char.c suggests that that is uses
> qemu_set_fd_handler() and will therefore be called with the qemu mutex
> held.
>

Ok, removed this in v4.

>>
>>>
>>>> +    sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n",
>>>> +                            rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5);
>>>> +    return&trace_str[0];
>>>> +}
>>>> diff --git a/tracetool b/tracetool
>>>> index c77280d..b7a0499 100755
>>>> --- a/tracetool
>>>> +++ b/tracetool
>>>> @@ -125,6 +125,11 @@ typedef struct {
>>>>       bool state;
>>>>   } TraceEvent;
>>>>
>>>> +/* Max size of trace string to be displayed via the monitor.
>>>> + * Format : "Event %lu : %lx %lx %lx %lx %lx\n"
>>>> + */
>>>> +#define MAX_TRACE_STR_LEN 100
>>>> +
>>>>   void trace1(TraceEventID event, unsigned long x1);
>>>>   void trace2(TraceEventID event, unsigned long x1, unsigned long x2);
>>>>   void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3);
>>>> @@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon
>>>>   void do_info_trace(Monitor *mon);
>>>>   void do_info_all_trace_events(Monitor *mon);
>>>>   void change_trace_event_state(const char *tname, bool tstate);
>>>> +unsigned int get_trace_idx(void);
>>>> +char* format_trace_string(unsigned int idx, char *trace_str);
>>>
>>> I think we need to choose a prefix like simpletrace_*() or something
>>> more concise (but not "strace_" because it's too confusing).  Other
>>> subsystems tend to do this: pci_*(), ram_*(), etc.
>>>
>>
>> Agree, it is useful. However, simpletrace_ is too big for a prefix.
>> Maybe st_ works, though it is slightly on the ambiguous side ?
>
> Cool, st_ works for me.
>

Great. I'll post a separate patch to address the name changes.

>>
>>> Perhaps let's do it as a separate patch to fix up all of the simple
>>> trace backend.
>>>
>>

Regards,
-- 
Prerna Saxena

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