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 :) > 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 with 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 registration 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 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] 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 make it happen. Lluis -- "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