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/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.
>
> 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