Fix build errors for target i386-linux-user

[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 <[email protected]>

---
 monitor.c     |   21 +++++++++++++++++++++
 simpletrace.c |   39 ++-------------------------------------
 tracetool     |   16 ++++++++++++++++
 3 files changed, 39 insertions(+), 37 deletions(-)

monitor.c +21 -0

index 433a3ec..9b5d65a 100644
@@ -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)

simpletrace.c +2 -37

index 57c41fc..834b4c1 100644
@@ -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;

tracetool +16 -0

index c77280d..c7a690d 100755
@@ -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);

Signature · 10 lines

1.6.2.5



-- 
Prerna Saxena

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

Quoted 4 lines · 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.

Quoted 3 lines

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

Quoted 5 lines · On Thu, 1 Jul 2010 10:18:41 +0100 Stefan Hajnoczi…

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

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 <[email protected]>

---
 Makefile.target |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Makefile.target +0 -0

index 493233a..b05e37e 100644

Makefile.target +2 -2

 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
 

Signature · 6 lines

-- 
Prerna Saxena

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

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

Signed-off-by Prerna Saxena <[email protected]>

---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
 tracetool     |    7 +++++++
 3 files changed, 59 insertions(+), 22 deletions(-)

monitor.c +23 -0

index 433a3ec..1f89938 100644
@@ -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)

simpletrace.c +0 -0

index 57c41fc..c7b1e7e 100644

simpletrace.c +0 -21

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

simpletrace.c +4 -0

+    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];
+}

tracetool +5 -0

index c77280d..b7a0499 100755
@@ -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);

tracetool +0 -0

 EOF
 
     simple_event_num=0

Signature · 10 lines

1.6.2.5



-- 
Prerna Saxena

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

Quoted 54 lines · Prerna Saxena

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

Signed-off-by Prerna Saxena<[email protected]>

---
  monitor.c     |   23 +++++++++++++++++++++++
  simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
  tracetool     |    7 +++++++
  3 files changed, 59 insertions(+), 22 deletions(-)

monitor.c +23 -0

index 433a3ec..1f89938 100644
@@ -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)

simpletrace.c +0 -1

index 57c41fc..c7b1e7e 100644
@@ -1,8 +1,8 @@
  #include<stdlib.h>
  #include<stdio.h>
-#include "monitor.h"
  #include "trace.h"
 

simpletrace.c line 5

Quoted 1 line

simpletrace.c +1 -0

+/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */

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

Quoted 42 lines

simpletrace.c +6 -21

  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)

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 105

Quoted 3 lines

simpletrace.c +3 -0

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

simpletrace.c void change_trace_event_state(const char *tname, bool tstate)

Quoted 13 lines

simpletrace.c +12 -0

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

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 120

Quoted 3 lines

simpletrace.c +3 -0

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

Is it necessary to copy the trace record here?

Quoted 23 lines

simpletrace.c +4 -0

+    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];
+}

tracetool +5 -0

index c77280d..b7a0499 100755
@@ -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);

tracetool void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon · line 142

Quoted 4 lines

tracetool +2 -0

  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.

Quoted 55 lines · Stefan Hajnoczi

Quoted 54 lines · Prerna Saxena

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

Signed-off-by Prerna Saxena<[email protected]>

---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
 tracetool     |    7 +++++++
 3 files changed, 59 insertions(+), 22 deletions(-)

monitor.c +23 -0

index 433a3ec..1f89938 100644
@@ -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)

simpletrace.c +0 -1

index 57c41fc..c7b1e7e 100644
@@ -1,8 +1,8 @@
 #include<stdlib.h>
 #include<stdio.h>
-#include "monitor.h"
 #include "trace.h"
 

Quoted 3 lines

simpletrace.c line 5

Quoted 1 line

simpletrace.c +1 -0

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

Quoted 42 lines

Quoted 42 lines

simpletrace.c +6 -21

 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)

Quoted 8 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 105

Quoted 3 lines

simpletrace.c +3 -0

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

Quoted 15 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate)

Quoted 13 lines

simpletrace.c +12 -0

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

Quoted 6 lines

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.

Quoted 6 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 120

Quoted 4 lines

simpletrace.c +3 -0

+        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 :-)

Quoted 24 lines

Quoted 24 lines

simpletrace.c +4 -0

+    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];
+}

tracetool +5 -0

index c77280d..b7a0499 100755
@@ -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);

Quoted 9 lines

tracetool void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon · line 142

Quoted 4 lines

tracetool +2 -0

 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 ?

Quoted 4 lines

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

Will do.

Thanks,

Signature · 5 lines

Prerna Saxena

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

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

Changelog from v3 :
1. Cleanups.

Signed-off-by Prerna Saxena <[email protected]>

---
 monitor.c     |   23 +++++++++++++++++++++++
 simpletrace.c |   52 ++++++++++++++++++++++++++++++---------------------- tracetool     |    7 +++++++

 3 files changed, 60 insertions(+), 22 deletions(-)

monitor.c +23 -0

index 433a3ec..1f89938 100644
@@ -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)

simpletrace.c +15 -22

index 57c41fc..78507ec 100644
@@ -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"
+ *

simpletrace.c +5 -0

+    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];
+}

tracetool +7 -0

index c77280d..b7a0499 100755
@@ -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

Quoted 56 lines · Prerna Saxena

Quoted 55 lines · Stefan Hajnoczi

Quoted 54 lines · Prerna Saxena

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

Signed-off-by Prerna Saxena<[email protected]>

---
  monitor.c     |   23 +++++++++++++++++++++++
  simpletrace.c |   51 +++++++++++++++++++++++++++++----------------------
  tracetool     |    7 +++++++
  3 files changed, 59 insertions(+), 22 deletions(-)

monitor.c +23 -0

index 433a3ec..1f89938 100644
@@ -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)

simpletrace.c +0 -1

index 57c41fc..c7b1e7e 100644
@@ -1,8 +1,8 @@
  #include<stdlib.h>
  #include<stdio.h>
-#include "monitor.h"
  #include "trace.h"
 

Quoted 8 lines

Quoted 3 lines

simpletrace.c line 5

Quoted 1 line

simpletrace.c +1 -0

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

Quoted 51 lines

Quoted 42 lines

Quoted 42 lines

simpletrace.c +6 -21

  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)

Quoted 8 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 105

Quoted 3 lines

simpletrace.c +3 -0

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

Quoted 4 lines

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.

Quoted 16 lines

Quoted 15 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate)

Quoted 13 lines

simpletrace.c +12 -0

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

Quoted 2 lines

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

Quoted 11 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 6 lines

simpletrace.c void change_trace_event_state(const char *tname, bool tstate) · line 120

Quoted 4 lines

simpletrace.c +3 -0

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

Is it necessary to copy the trace record here?

Quoted 7 lines

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.

Quoted 34 lines

Quoted 33 lines

Quoted 23 lines

simpletrace.c +4 -0

+    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];
+}

tracetool +5 -0

index c77280d..b7a0499 100755
@@ -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);

tracetool void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon · line 142

Quoted 5 lines

tracetool +2 -0

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

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

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