On Tue, Jan 10, 2012 at 10:59 AM, Harsh Prateek Bora <ha...@linux.vnet.ibm.com> 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.