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

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

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