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
