From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RkxK6-0003Kz-PI for qemu-devel@nongnu.org; Wed, 11 Jan 2012 07:30:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RkxK5-00022t-Fs for qemu-devel@nongnu.org; Wed, 11 Jan 2012 07:30:34 -0500 Received: from mail-wi0-f173.google.com ([209.85.212.173]:59586) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RkxK5-00022o-37 for qemu-devel@nongnu.org; Wed, 11 Jan 2012 07:30:33 -0500 Received: by wibhm11 with SMTP id hm11so389489wib.4 for ; Wed, 11 Jan 2012 04:30:32 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1326193179-19677-4-git-send-email-harsh@linux.vnet.ibm.com> References: <1326193179-19677-1-git-send-email-harsh@linux.vnet.ibm.com> <1326193179-19677-4-git-send-email-harsh@linux.vnet.ibm.com> Date: Wed, 11 Jan 2012 12:30:32 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v3 3/3] simpletrace.py: updated log reader script to handle new log format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Harsh Prateek Bora Cc: vilanova@ac.upc.edu, mathieu.desnoyers@efficios.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, aneesh.kumar@linux.vnet.ibm.com On Tue, Jan 10, 2012 at 10:59 AM, Harsh Prateek Bora 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): > + =A0 =A0"""Parse a trace-events file into {event_num: (name, type arg, .= ..)}.""" > + > + =A0 =A0events =3D {dropped_event_id: ('name', 'args')} > + =A0 =A0event_num =3D 0 > + =A0 =A0for line in fobj: > + =A0 =A0 =A0 =A0m =3D event_re.match(line.strip()) > + =A0 =A0 =A0 =A0if m is None: > + =A0 =A0 =A0 =A0 =A0 =A0continue > + > + =A0 =A0 =A0 =A0disable, name, args =3D m.groups() > + =A0 =A0 =A0 =A0events[event_num] =3D tuple(args.split(',')) > + =A0 =A0 =A0 =A0event_num +=3D 1 > + =A0 =A0return 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): > + =A0 =A0params =3D etypes[event_id] > + =A0 =A0for elem in params: > + =A0 =A0 =A0 =A0if is_string(elem): > + =A0 =A0 =A0 =A0 =A0 =A0l =3D fobj.read(4) > + =A0 =A0 =A0 =A0 =A0 =A0(len,) =3D struct.unpack('=3DL', l) > + =A0 =A0 =A0 =A0 =A0 =A0s =3D fobj.read(len) > + =A0 =A0 =A0 =A0 =A0 =A0sfmt =3D `len`+'s' > + =A0 =A0 =A0 =A0 =A0 =A0(str,) =3D struct.unpack_from(sfmt, s) s =3D=3D str, there's no need to unpack > + =A0 =A0 =A0 =A0 =A0 =A0type, sep, var =3D elem.rpartition('*') Event should handle trace-events parsing, we shouldn't sprinkle the parsing into all code that uses trace-events. > + =A0 =A0 =A0 =A0 =A0 =A0print '%(arg)s=3D%(val)s' % { 'arg': var.lstrip(= ), 'val': str }, > + =A0 =A0 =A0 =A0elif '*' in elem: > + =A0 =A0 =A0 =A0 =A0 =A0(value,) =3D struct.unpack('=3DQ', fobj.read(8)) > + =A0 =A0 =A0 =A0 =A0 =A0type, sep, var =3D elem.rpartition('*') > + =A0 =A0 =A0 =A0 =A0 =A0print '%(arg)s=3D0x%(val)u' % { 'arg': var.lstri= p(), 'val': value }, > + =A0 =A0 =A0 =A0else: > + =A0 =A0 =A0 =A0 =A0 =A0(value,) =3D struct.unpack('=3DQ', fobj.read(8)) > + =A0 =A0 =A0 =A0 =A0 =A0type, sep, var =3D elem.rpartition(' ') > + =A0 =A0 =A0 =A0 =A0 =A0print '%(arg)s=3D%(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'). > + =A0 =A0print > + =A0 =A0return > + > +etypes =3D get_event_argtypes(open(sys.argv[1], 'r')) Parsing trace-events... > +def processv2(fobj): > + =A0 =A0'''Process simpletrace v2 log trace records''' > + =A0 =A0events =3D 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. > + =A0 =A0#print events > + =A0 =A0max_events =3D len(events) - 1 > + =A0 =A0last_timestamp =3D None > + =A0 =A0while True: > + =A0 =A0 =A0 =A0# read record header > + =A0 =A0 =A0 =A0rechdr =3D read_header(fobj) > + =A0 =A0 =A0 =A0if rechdr is None: > + =A0 =A0 =A0 =A0 =A0 =A0print "No more records" debugging? > + =A0 =A0 =A0 =A0 =A0 =A0break > + > + =A0 =A0 =A0 =A0if rechdr[0] >=3D max_events: > + =A0 =A0 =A0 =A0 =A0 =A0print "Invalid Event ID found, Trace Log may be = corrupted !!!" > + =A0 =A0 =A0 =A0 =A0 =A0sys.exit(1) > + > + =A0 =A0 =A0 =A0if last_timestamp is None: > + =A0 =A0 =A0 =A0 =A0 =A0last_timestamp =3D rechdr[1] > + =A0 =A0 =A0 =A0delta_ns =3D rechdr[1] - last_timestamp > + =A0 =A0 =A0 =A0last_timestamp =3D rechdr[1] > + > + =A0 =A0 =A0 =A0print events[rechdr[0]][0], =A0'%0.3f' % (delta_ns / 100= 0.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. > + =A0 =A0 =A0 =A0# read data > + =A0 =A0 =A0 =A0# process_event(rec[0], fobj) > + =A0 =A0 =A0 =A0# rechdr[2] is record length (including header) > + =A0 =A0 =A0 =A0#print etypes[rechdr[0]] > + =A0 =A0 =A0 =A0#data =3D fobj.read(rechdr[2] - header_len) > + =A0 =A0 =A0 =A0process_event(rechdr[0], fobj) > + > =A0class Analyzer(object): > =A0 =A0 """A trace file analyzer which processes trace records. > > @@ -90,8 +165,6 @@ def process(events, log, analyzer): > =A0 =A0 """Invoke an analyzer on each event in a log.""" > =A0 =A0 if isinstance(events, str): > =A0 =A0 =A0 =A0 events =3D parse_events(open(events, 'r')) > - =A0 =A0if isinstance(log, str): > - =A0 =A0 =A0 =A0log =3D 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.