From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49031) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAHrg-0006s7-QS for qemu-devel@nongnu.org; Wed, 21 Mar 2012 05:30:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SAHre-0004Or-DR for qemu-devel@nongnu.org; Wed, 21 Mar 2012 05:29:56 -0400 Received: from mail-lpp01m010-f45.google.com ([209.85.215.45]:49956) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAHrd-0004ON-Ui for qemu-devel@nongnu.org; Wed, 21 Mar 2012 05:29:54 -0400 Received: by lahe6 with SMTP id e6so740244lah.4 for ; Wed, 21 Mar 2012 02:29:51 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87pqc79km7.fsf@ginnungagap.bsc.es> References: <20120313200235.24179.63987.stgit@ginnungagap.bsc.es> <20120313200332.24179.78152.stgit@ginnungagap.bsc.es> <87obrrgyoc.fsf@ginnungagap.bsc.es> <87pqc79km7.fsf@ginnungagap.bsc.es> Date: Wed, 21 Mar 2012 09:29:51 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 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: =?ISO-8859-1?Q?Llu=EDs_Vilanova?= Cc: harsh@linux.vnet.ibm.com, qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com 2012/3/20 Llu=EDs Vilanova : > Stefan Hajnoczi writes: > >> 2012/3/20 Llu=EDs Vilanova : >>> Stefan Hajnoczi writes: >>> >>>> 2012/3/13 Llu=EDs Vilanova : >>>>> Adds decorators to establish which backend and/or format each routine= is meant >>>>> to process. >>>>> >>>>> With this, tables enumerating format and backend routines can be elim= inated and >>>>> part of the usage message can be computed in a more generic way. >>>>> >>>>> Signed-off-by: Llu=EDs Vilanova >>>>> Signed-off-by: Harsh Prateek Bora >>>>> --- >>>>> =A0Makefile.objs =A0 =A0 =A0 =A0| =A0 =A06 - >>>>> =A0Makefile.target =A0 =A0 =A0| =A0 =A03 >>>>> =A0scripts/tracetool.py | =A0320 ++++++++++++++++++++++++++++++++----= -------------- >>>>> =A03 files changed, 211 insertions(+), 118 deletions(-) >>> >>>> I find the decorators are overkill and we miss the chance to use more >>>> straightforward approaches that Python supports. =A0The decorators bui= ld >>>> structures behind the scenes instead of using classes in an open coded >>>> way. =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. >>> >>>> I've tried out an alternative approach which works very nicely for >>>> formats. =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: >>> >>>> https://github.com/stefanha/qemu/commit/3500eb43f80f3c9200107aa0ca19a1= b57300ef8a >>> >>>> What do you think? >>> >>> I don't like it: >>> >>> 1) Format and backend tables must be manually filled. >>> >>> 2) The base Backend class has empty methods for each possible format. >>> >>> 3) There is no control on format/backend compatibility. >>> >>> But I do like the idea of having a single per-backend class having meth= ods for >>> each possible format. >>> >>> The main reason for automatically establishing formats, backends and th= eir >>> compatibility is that the instrumentation patches add some extra format= s and >>> backends, which makes the process much more tedious if it's not automat= ed. >>> >>> Whether you use decorators or classes is not that important. >>> >>> 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.). >>> >>> Format/backend compatibility can be established by introspecting into t= he >>> methods on each backend child class, matched against the NAMEs of the r= egistered >>> formats. >>> >>> But going this way does not sound to me like it will be much clearer th= an >>> decorators. >>> >>> 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. =A0The 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 doe= sn't > matter that much :) Formats seem so simple to me that the cost of any infrastructure is higher than throwing a Format() instance in a dict. > Maybe this would work nice for everybody: > > tracetool.py =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# main program (just pars= e cmdline opts and call tracetool module) > tracetool/__init__.py =A0 =A0 =A0 =A0 # common boilerplate code (e.g., ev= ent parsing and call dispatching) > tracetool/format/__init__.py =A0# format auto-registration/lookup code > tracetool/format/h.py =A0 =A0 =A0 =A0 # code for the 'h' format > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# __doc__ =A0 = =A0 =A0 =A0 =A0 [mandatory, format description] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# def begin(ev= ents) [optional] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# def end(even= ts) =A0 [optional] > tracetool/backend/__init__.py # backend auto-registration/lookup code > tracetool/backend/simple.py =A0 # code for the 'simple' backend > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# __doc__ =A0 = =A0 =A0 =A0 =A0 =A0[mandatory, backend description] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# PUBLIC =3D [= True|False] [optional, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0defaults to False, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0indicates it's listed on --list-backends= ] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# def format(e= vents) [optional, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 backend-specific code for given format, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0# =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 implicitly indicates format compatibility] Let's call this function backend.generate(events) instead of "format" since we already use that term and it's a Python builtin too. This is a code generation function. > > 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. I think it's more convenient to drop the nop backend and introduce a nop() code generation function for each format. The .h format would nop() function would emit a static inline empty C function that does nothing. The other formats could probably do nothing in nop(). >> As the next step with this patch series we could drop this patch. =A0It >> doesn't make an external difference. =A0Then 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 = make it > happen. I like the organization you have proposed. In order to avoid rebasing and porting too many fixes from tracetool to tracetool.py I suggest you resend this series without the format/backend consolidation code. I can merge this series quickly and we can handle the format/backend consolidation code in a separate patch series. Stefan