From: Stefan Hajnoczi <stefanha@gmail.com>
To: Stefan Hajnoczi <stefanha@redhat.com>,
qemu-devel@nongnu.org, Kazuya Saito <saito.kazuya@jp.fujitsu.com>
Subject: Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
Date: Thu, 20 Feb 2014 11:20:58 +0100 [thread overview]
Message-ID: <20140220102058.GD1272@stefanha-thinkpad.redhat.com> (raw)
In-Reply-To: <87eh2zw2oh.fsf@fimbulvetr.bsc.es>
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 <vilanova@ac.upc.edu>
> >> ---
> >>
> >> 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
next prev parent reply other threads:[~2014-02-20 10:21 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-17 19:36 [Qemu-devel] [PATCH 0/4] Tracetool cleanup Lluís Vilanova
2014-02-17 19:36 ` [Qemu-devel] [PATCH 1/4] trace: [tracetool] Add method 'Event.api' to build event names Lluís Vilanova
2014-02-17 19:36 ` [Qemu-devel] [PATCH 2/4] trace: [tracetool,trivial] Style changes Lluís Vilanova
2014-02-17 19:36 ` [Qemu-devel] [PATCH 3/4] trace: [tracetool] Identify formats directly used by QEMU Lluís Vilanova
2014-02-17 19:36 ` [Qemu-devel] [PATCH 4/4] trace: [tracetool] Minimize the amount of per-backend code Lluís Vilanova
2014-02-19 13:37 ` Stefan Hajnoczi
2014-02-19 13:48 ` [Qemu-devel] [PATCH 0/4] Tracetool cleanup Stefan Hajnoczi
2014-02-19 15:19 ` Lluís Vilanova
2014-02-20 10:20 ` Stefan Hajnoczi [this message]
2014-02-20 13:39 ` Lluís Vilanova
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140220102058.GD1272@stefanha-thinkpad.redhat.com \
--to=stefanha@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=saito.kazuya@jp.fujitsu.com \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).