From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAbtk-0005MK-O0 for qemu-devel@nongnu.org; Tue, 04 Feb 2014 04:02:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WAbte-0007Sv-JP for qemu-devel@nongnu.org; Tue, 04 Feb 2014 04:02:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:29902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAbte-0007SK-Bj for qemu-devel@nongnu.org; Tue, 04 Feb 2014 04:02:22 -0500 Date: Tue, 4 Feb 2014 10:02:14 +0100 From: Stefan Hajnoczi Message-ID: <20140204090214.GC6919@stefanha-thinkpad.redhat.com> References: <52E73388.90601@jp.fujitsu.com> <20140131103750.GF30735@stefanha-thinkpad.redhat.com> <52F079F5.1040009@jp.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52F079F5.1040009@jp.fujitsu.com> Subject: Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kazuya Saito Cc: "qemu-devel@nongnu.org" 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_ and ensuring all formats use the "body_" function prefix) or with something explicit like a .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.