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

[Stefan Hajnoczi]

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.

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.

Stefan