From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SA44n-0006LO-D2 for qemu-devel@nongnu.org; Tue, 20 Mar 2012 14:46:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SA44h-0002ek-Qc for qemu-devel@nongnu.org; Tue, 20 Mar 2012 14:46:32 -0400 Received: from roura.ac.upc.edu ([147.83.33.10]:48119 helo=roura.ac.upc.es) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SA44h-0002dx-BN for qemu-devel@nongnu.org; Tue, 20 Mar 2012 14:46:27 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <20120313200235.24179.63987.stgit@ginnungagap.bsc.es> <20120313200332.24179.78152.stgit@ginnungagap.bsc.es> <87obrrgyoc.fsf@ginnungagap.bsc.es> Date: Tue, 20 Mar 2012 19:45:52 +0100 In-Reply-To: (Stefan Hajnoczi's message of "Tue, 20 Mar 2012 16:33:30 +0000") Message-ID: <87pqc79km7.fsf@ginnungagap.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: harsh@linux.vnet.ibm.com, qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com Stefan Hajnoczi writes: > 2012/3/20 Llu=C3=ADs Vilanova : >> Stefan Hajnoczi writes: >>=20 >>> 2012/3/13 Llu=C3=ADs Vilanova : >>>> Adds decorators to establish which backend and/or format each routine = is meant >>>> to process. >>>>=20 >>>> With this, tables enumerating format and backend routines can be elimi= nated and >>>> part of the usage message can be computed in a more generic way. >>>>=20 >>>> Signed-off-by: Llu=C3=ADs Vilanova >>>> Signed-off-by: Harsh Prateek Bora >>>> --- >>>> =C2=A0Makefile.objs =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A06 - >>>> =C2=A0Makefile.target =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A03 >>>> =C2=A0scripts/tracetool.py | =C2=A0320 +++++++++++++++++++++++++++++++= +------------------ >>>> =C2=A03 files changed, 211 insertions(+), 118 deletions(-) >>=20 >>> I find the decorators are overkill and we miss the chance to use more >>> straightforward approaches that Python supports. =C2=A0The decorators b= uild >>> structures behind the scenes instead of using classes in an open coded >>> way. =C2=A0I think this makes it more difficult for people to modify the >>> code - they will need to dig in to what exactly the decorators do - >>> what they do is pretty similar to what you get from a class. >>=20 >>> I've tried out an alternative approach which works very nicely for >>> formats. =C2=A0For backends it's not a perfect fit because it creates >>> instances when we don't really need them, but I think it's still an >>> overall cleaner approach: >>=20 >>> https://github.com/stefanha/qemu/commit/3500eb43f80f3c9200107aa0ca19a1b= 57300ef8a >>=20 >>> What do you think? >>=20 >> I don't like it: >>=20 >> 1) Format and backend tables must be manually filled. >>=20 >> 2) The base Backend class has empty methods for each possible format. >>=20 >> 3) There is no control on format/backend compatibility. >>=20 >> But I do like the idea of having a single per-backend class having metho= ds for >> each possible format. >>=20 >> The main reason for automatically establishing formats, backends and the= ir >> compatibility is that the instrumentation patches add some extra formats= and >> backends, which makes the process much more tedious if it's not automate= d. >>=20 >> Whether you use decorators or classes is not that important. >>=20 >> Auto-registration can be accomplished using metaclasses and special >> per-format/backend "special" attributes the metaclasses will look for (e= .g. NAME >> to set the commandline-visible name of a format/backend.). >>=20 >> Format/backend compatibility can be established by introspecting into the >> methods on each backend child class, matched against the NAMEs of the re= gistered >> formats. >>=20 >> But going this way does not sound to me like it will be much clearer than >> decorators. >>=20 >> Do you agree? (I'll wait on solving this before fixing the rest of your = concerns >> in this series). Do you still prefer classes over decorators? > For formats the Format class plus a dict approach is much nicer than > decorators. The code is short and easy to understand. Well, I prefer to automate this kind of things so that new features get automatically registered and the changes are localized; but it really doesn= 't matter that much :) > For backends it becomes a little tougher and I was wondering whether > splitting the code into modules would buy us something. The fact that > you've added '####...' section delimeters shows that tracetool.py is > growing to long and we're putting too many concepts into one file. If > each backend is a module then we have a natural way of containing > backend-specific code. Perhaps the module can register itself when > tracetool.py wildcard imports them all. I think this will approach > the level of magic that decorators involve but with the bonus that we > begin to split the code instead of growing a blob. What do you think > of putting each backend in its own module? Sure, the script is getting too large. I just tried to get what I needed wi= th minimal changes on top of the existing code. > Do you have a link to your latest code that adds formats/backends? > I'd like to take a quick look to make sure I understand where you're > going with this - I've only been thinking of the current set of > formats/backends. There's no public repo, sorry. Still, some of "my backends" need registrati= on of intermediate backends *and* formats (e.g., not available through --list-backends) that are specific to instrumentation. Maybe this would work nice for everybody: tracetool.py # main program (just parse cmdline opts and c= all tracetool module) tracetool/__init__.py # common boilerplate code (e.g., event parsin= g and call dispatching) tracetool/format/__init__.py # format auto-registration/lookup code tracetool/format/h.py # code for the 'h' format # __doc__ [mandatory, format descri= ption] # def begin(events) [optional] # def end(events) [optional] tracetool/backend/__init__.py # backend auto-registration/lookup code tracetool/backend/simple.py # code for the 'simple' backend # __doc__ [mandatory, backend desc= ription] # PUBLIC =3D [True|False] [optional,=20 # defaults to False, # indicates it's liste= d on --list-backends] # def format(events) [optional,=20 # backend-specific code f= or given format, # implicitly indicates fo= rmat compatibility] Note that new formats will need to add new format routines in 'tracetool/backend/nop.py' to accomodate whatever action has to be taken on disabled events. > As the next step with this patch series we could drop this patch. It > doesn't make an external difference. Then we can continue to discuss > how to best handle backends as a separate patch. WDYT of the organization above? Given the current code it's pretty simple to split it into different modules. If everybody agrees on the above, I can ma= ke it happen. Lluis --=20 "And it's much the same thing with knowledge, for whenever you learn something new, the whole world becomes that much richer." -- The Princess of Pure Reason, as told by Norton Juster in The Phantom Tollbooth