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

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

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