[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establis
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats |
Date: |
Wed, 21 Mar 2012 09:29:51 +0000 |
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
- [Qemu-devel] [PATCH 07/12] trace: [tracetool] Process the "disable" event property, (continued)
- [Qemu-devel] [PATCH 07/12] trace: [tracetool] Process the "disable" event property, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 03/12] trace: [tracetool] Do not rebuild event list in backend code, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 04/12] trace: [tracetool] Simplify event line parsing, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 08/12] trace: [tracetool] Rewrite event argument parsing, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 09/12] trace: [tracetool] Make format-specific code optional and with access to events, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/13
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Stefan Hajnoczi, 2012/03/20
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/20
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Stefan Hajnoczi, 2012/03/20
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/20
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/21
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Stefan Hajnoczi, 2012/03/22
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/21
- Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats, Lluís Vilanova, 2012/03/21
[Qemu-devel] [PATCH 11/12] trace: Provide a per-event status define for conditional compilation, Lluís Vilanova, 2012/03/13
[Qemu-devel] [PATCH 12/12] trace: [tracetool] Add error-reporting functions, Lluís Vilanova, 2012/03/13
Re: [Qemu-devel] [PATCH 00/12] Rewrite tracetool using python, Stefan Hajnoczi, 2012/03/20