2012/3/20 Lluís Vilanova <vilan...@ac.upc.edu>: > Stefan Hajnoczi writes: > >> 2012/3/20 Lluís Vilanova <vilan...@ac.upc.edu>: >>> Stefan Hajnoczi writes: >>> >>>> 2012/3/13 Lluís Vilanova <vilan...@ac.upc.edu>: >>>>> 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 >>>>> eliminated and >>>>> part of the usage message can be computed in a more generic way. >>>>> >>>>> Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu> >>>>> Signed-off-by: Harsh Prateek Bora <ha...@linux.vnet.ibm.com> >>>>> --- >>>>> Makefile.objs | 6 - >>>>> Makefile.target | 3 >>>>> scripts/tracetool.py | 320 >>>>> ++++++++++++++++++++++++++++++++------------------ >>>>> 3 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. The decorators build >>>> structures behind the scenes instead of using classes in an open coded >>>> way. I 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. For 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/3500eb43f80f3c9200107aa0ca19a1b57300ef8a >>> >>>> 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 methods >>> for >>> each possible format. >>> >>> The main reason for automatically establishing formats, backends and their >>> compatibility is that the instrumentation patches add some extra formats and >>> backends, which makes the process much more tedious if it's not automated. >>> >>> 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 the >>> methods on each backend child class, matched against the NAMEs of the >>> registered >>> formats. >>> >>> But going this way does not sound to me like it will be much clearer than >>> 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. 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 :)
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 # main program (just parse cmdline opts and > call tracetool module) > tracetool/__init__.py # common boilerplate code (e.g., event parsing > and call dispatching) > tracetool/format/__init__.py # format auto-registration/lookup code > tracetool/format/h.py # code for the 'h' format > # __doc__ [mandatory, format > description] > # 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 > description] > # PUBLIC = [True|False] [optional, > # defaults to False, > # indicates it's listed > on --list-backends] > # def format(events) [optional, > # backend-specific code for > given format, > # 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. 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 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