Re: [Qemu-devel] [RFC PATCH v3 3/3] simpletrace.py: updated log reader script to handle new log format

[Stefan Hajnoczi]

On Tue, Jan 10, 2012 at 10:59 AM, Harsh Prateek Bora
<address@hidden> wrote:
> diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py

This file is primarily a Python module for writing trace analysis
scripts.  The Formatter is a useful code example that shows how to use
the module.  This change breaks the module API and turns this file
basically just into the Formatter.  Please don't do that.

When processing v2 files the trace record arguments passed to
Analyzer.catchall() or dedicated event handler methods can now also be
strings, not just ints.  That's the only API change that is required.

> +def get_event_argtypes(fobj):
> +    """Parse a trace-events file into {event_num: (name, type arg, ...)}."""
> +
> +    events = {dropped_event_id: ('name', 'args')}
> +    event_num = 0
> +    for line in fobj:
> +        m = event_re.match(line.strip())
> +        if m is None:
> +            continue
> +
> +        disable, name, args = m.groups()
> +        events[event_num] = tuple(args.split(','))
> +        event_num += 1
> +    return events

It would be nice to share Event with tracetool.py instead of
duplicating trace-events parsing.  If you want Event can live in its
own traceevent.py module.

> +def process_event(event_id, fobj):
> +    params = etypes[event_id]
> +    for elem in params:
> +        if is_string(elem):
> +            l = fobj.read(4)
> +            (len,) = struct.unpack('=L', l)
> +            s = fobj.read(len)
> +            sfmt = `len`+'s'
> +            (str,) = struct.unpack_from(sfmt, s)

s == str, there's no need to unpack

> +            type, sep, var = elem.rpartition('*')

Event should handle trace-events parsing, we shouldn't sprinkle the
parsing into all code that uses trace-events.

> +            print '%(arg)s=%(val)s' % { 'arg': var.lstrip(), 'val': str },
> +        elif '*' in elem:
> +            (value,) = struct.unpack('=Q', fobj.read(8))
> +            type, sep, var = elem.rpartition('*')
> +            print '%(arg)s=0x%(val)u' % { 'arg': var.lstrip(), 'val': value 
> },
> +        else:
> +            (value,) = struct.unpack('=Q', fobj.read(8))
> +            type, sep, var = elem.rpartition(' ')
> +            print '%(arg)s=%(val)u' % { 'arg': var.lstrip(), 'val': value },

The existing simpletrace.py Formatter will use 0x%x for all fields.  I
suggest keeping it that way unless you want to use the event's format
string, which is might be doable (though you'd need to convert '%p' to
'%#x').

> +    print
> +    return
> +
> +etypes = get_event_argtypes(open(sys.argv[1], 'r'))

Parsing trace-events...

> +def processv2(fobj):
> +    '''Process simpletrace v2 log trace records'''
> +    events = parse_events(open(sys.argv[1], 'r'))

...multiple times in different ways is not good.  Please implement an
Event class that handles the file format parsing and is shared between
simpletrace.py and tracetool.py.

> +    #print events
> +    max_events = len(events) - 1
> +    last_timestamp = None
> +    while True:
> +        # read record header
> +        rechdr = read_header(fobj)
> +        if rechdr is None:
> +            print "No more records"

debugging?

> +            break
> +
> +        if rechdr[0] >= max_events:
> +            print "Invalid Event ID found, Trace Log may be corrupted !!!"
> +            sys.exit(1)
> +
> +        if last_timestamp is None:
> +            last_timestamp = rechdr[1]
> +        delta_ns = rechdr[1] - last_timestamp
> +        last_timestamp = rechdr[1]
> +
> +        print events[rechdr[0]][0],  '%0.3f' % (delta_ns / 1000.0),

simpletrace is a module that analysis scripts use, it should not print
anything.  The prints belong in the Formatter() class which does
pretty-printing and is executed when simpletrace.py is executed as a
program.

> +        # read data
> +        # process_event(rec[0], fobj)
> +        # rechdr[2] is record length (including header)
> +        #print etypes[rechdr[0]]
> +        #data = fobj.read(rechdr[2] - header_len)
> +        process_event(rechdr[0], fobj)
> +
>  class Analyzer(object):
>     """A trace file analyzer which processes trace records.
>
> @@ -90,8 +165,6 @@ def process(events, log, analyzer):
>     """Invoke an analyzer on each event in a log."""
>     if isinstance(events, str):
>         events = parse_events(open(events, 'r'))
> -    if isinstance(log, str):
> -        log = open(log, 'rb')

Removing this breaks existing analysis scripts that use process().
Parsing the header should be part of read_trace_file(), we can raise
an exception if the file format is unsupported.