| Message ID | 20100708105858.7b456295@zephyr |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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,
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
[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(-)