[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: |
Tue, 20 Mar 2012 16:33:30 +0000 |
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
- [Qemu-devel] [PATCH 05/12] trace: [tracetool] Do not precompute the event number, (continued)
- [Qemu-devel] [PATCH 05/12] trace: [tracetool] Do not precompute the event number, Lluís Vilanova, 2012/03/13
- [Qemu-devel] [PATCH 06/12] trace: [tracetool] Add support for event properties, Lluís Vilanova, 2012/03/13
- [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 <=
- 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/21
- 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