Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats

[Stefan Hajnoczi]

2012/3/20 Lluís Vilanova <address@hidden>:
> Stefan Hajnoczi writes:
>
>> 2012/3/13 Lluís Vilanova <address@hidden>:
>>> 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 <address@hidden>
>>> Signed-off-by: Harsh Prateek Bora <address@hidden>
>>> ---
>>>  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.

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?

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.

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.

Stefan