qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establis


From: Lluís Vilanova
Subject: Re: [Qemu-devel] [PATCH 10/12] trace: [tracetool] Automatically establish available backends and formats
Date: Tue, 20 Mar 2012 19:45:52 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.94 (gnu/linux)

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 :)


> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]