Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend

[Stefan Hajnoczi <stefanha@redhat.com>]

On Tue, Feb 04, 2014 at 02:26:13PM +0900, Kazuya Saito wrote:
> (2014/01/31 19:37), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
> >  I hope this
> > change can be dropped in the next revision of the patch since the "ust"
> > backend will no longer be different.
> 
> OK.  I'll try it.
> However, the above should be changed because of "events" backend even if
> it supports "ust" backend.  I think there is two solutions for this.
> The first is to define common.events_h() and common.events_c().
> The other is to branch it for "events" backend as below.
> 
>     if backends == ["nop"]:
>         func = tracetool.try_import("tracetool.format." + format,
>                                     "nop", _empty)[1]
>         func(events)
>     elif backends == ["events"]:
>         func = tracetool.try_import("tracetool.backend.events",
>                                     format, None)[1]
>         func(events)
>     else:
>         func = tracetool.try_import("tracetool.backend.common",
>                                     format, None)[1]
>         func(backends, events)
> 
> I think the first is better because backend/__init__.py shouldn't care
> "events" backend.  Do you have any suggestions?

Yes, I prefer less special cases too.

> >>  def h(events):
> >> +    pass
> >
> > I thought all code generation now happens in backends.common.h(), so
> > this function will not be called anymore?
> 
> It is called in tracetool.backend.compatible().  So, it is required when
> selecting only dtrace backend.
> 
> > The same is true for c() defined in this file.
> 
> It is also required for the same reason as dtrace.h().

tracetool.backend.compatible() is testing for the existence of a
function that is not used anywhere else.  Backend code doesn't make it
obvious why this is necessary.

We should either make compatible() work (e.g. by testing body_<format>
and ensuring all formats use the "body_" function prefix) or with
something explicit like a <backend>.supported_formats = ['c', 'h'] list.

> >> +util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
> >
> > How about adding multi.o to util-obj-y just like control.o below?  All
> > these object files are added to libqemuutil.a.  The linker will only
> > pull in object files that are needed (based on symbol dependencies) so
> > there is no harm in uncoditionally building multi.o.
> 
> If adding multi.o to util-obj-y, compile error occurs when selecting
> only dtrace backend.  This is because the function trace_print_events(),
> trace_event_set_state_dynamic_backend() and trace_backend_init() are
> declared doubly in multi.o and default.o.
> So, I'm going to leave it.  Do you have any suggestions?

I guess it should be:

ifeq ($(CONFIG_TRACE_DEFAULT),y)
util-obj-y += default.o
else
util-obj-y += multi.o
endif

> >> +bool trace_backend_init(const char *events, const char *file)
> >> +{
> >> +    bool retval = true;
> >> +
> >> +#ifndef CONFIG_TRACE_SIMPLE
> >> +    if (file) {
> >> +        fprintf(stderr, "error: -trace file=...: "
> >> +                "option not supported by the selected tracing backend\n");
> >> +        return false;
> >> +    }
> >> +#endif
> >
> > Not sure if this is right.  We may need to use -trace file=... for
> > simple or ftrace.  stderr should just ignore it.
> 
> The error message is output if the argument "file" is set when using
> "stderr", "ftrace" or "dtrace" backend.  In other words, only "simple"
> backend uses -trace file=...
> This is the reason why I implemented it as seen above.

oops, I misread the code.  I thought #ifdef instead of #ifndef.