qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup


From: Lluís Vilanova
Subject: Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
Date: Thu, 20 Feb 2014 14:39:27 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Stefan Hajnoczi writes:

> On Wed, Feb 19, 2014 at 04:19:10PM +0100, Lluís Vilanova wrote:
>> Stefan Hajnoczi writes:
>> 
>> > On Mon, Feb 17, 2014 at 08:36:19PM +0100, Lluís Vilanova wrote:
>> >> Minimizes the amount of backend code, making it simpler to add 
>> >> new/different
>> >> backends.
>> >> 
>> >> Also performs other cleanups all around.
>> >> 
>> >> Signed-off-by: Lluís Vilanova <address@hidden>
>> >> ---
>> >> 
>> >> Lluís Vilanova (4):
>> >> trace: [tracetool] Add method 'Event.api' to build event names
>> >> trace: [tracetool,trivial] Style changes
>> >> trace: [tracetool] Identify formats directly used by QEMU
>> >> trace: [tracetool] Minimize the amount of per-backend code
>> 
>> > I think we stretched the concepts of backends and formats too far.
>> > There are formats that only work with one backend (like 'stap').  And
>> > there are backends that behave differently from all other backends.
>> 
>> > As a result we're trying to abstract and make common a bunch of stuff
>> > that isn't really common.  This problem existed before this patch
>> > series, but I feel we're going further down a direction that
>> > increasingly seems to be wrong.
>> 
>> > It's simpler if we turn the design inside-out.  Instead of making
>> > backends export all sorts of interfaces and flags, tracetool should just
>> > parse trace-events and hand the list over to the backend.
>> 
>> > Let the backend do whatever it wants.  The format option simply becomes
>> > an option telling the backend which type of output to generate
>> > (events.h, events.c, .stp, etc).
>> 
>> > Common behavior should live in plain old Python modules/functions.
>> 
>> > TL;DR moving to a library design would simplify and clean up more than
>> > trying to improve the current framework design
>> 
>> > What do you think?
>> 
>> This series moves into that direction; some of the formats are simply not
>> handled by backends. For example, the "stap", "events_c" and "events_h" 
>> formats
>> have no backend-specific contents.
>> 
>> Also, having common code for the "format", and then relying on backends for a
>> small piece of the contents simplifies later patches like the multi-backend
>> tracing.
>> 
>> The thing here is that maybe we should change format to "file", since it
>> actually refers to a specific output file.

> You have a point.  tracetool needs to output a particular file (e.g.
> generated-events.c, generated-tracers.h), which is kind of what a
> "format" has become.

> So the "format" is the primary piece of code that emits output.  But if
> it needs to do something backend-specific (like generated-tracers.h)
> then it should call into backend modules.

Exactly.


> I am still concerned about the weird and wonderful interfaces that we're
> creating (like the "API" field in this patch series).  They make it
> harder to understand the code and add new backends.  Will think about
> this more when reviewing the next revision of this series.

Right. To be honest, the "API" sounded like a nice idea when I wrote it, but the
only thing it buys you is that non-API formats will only receive non-disabled
events. A really small benefit for adding more inter-module semantics. I will
probably remove it after rebasing the series.


Thanks,
  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]