lkml.org 
[lkml]   [2009]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] Fix tracing infrastructure to support multiple includes when defining CREATE_TRACE_POINTS
From
Date
On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:
> On Thu, Dec 03, 2009 at 09:15:26AM -0500, Steven Rostedt wrote:
> > On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> > > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > > be included with CREATE_TRACE_EVENTS defined
> > >
> > > I've been workingon adding a few tracepoints to the network stack, and in so
> > > doing it was convienient for me to add a second include file to
> > > net/core/net-traces.c with TRACE_EVENTS defined in them. net-traces.c defines
> > > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > > duplicate definitions of __tpstrtab_<name>. I tracked down the bug to find that
> > > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > > run through define_trace.h (which is included from skb.h, included in
> > > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > > to start a new cycle with the next header.
> > >
> > > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > > This places us back in the proper state to define a new set of tracepoints in a
> > > subsequent header file. Not sure if theres a better way to handle this, but
> > > this worked well for me, allowing me to include multiple headers with
> > > TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
> > >
> >
> > Nice, just some comments below.
> ><snip>
> >
> > You duplicate the DECLARE_TRACE from above and place it into an
> > unprotected area. Why not move it instead of duplicating it. This is
> > just a bug waiting to happen if we ever need to modify DECLARE_TRACE.
> >
> > If you change it to:
> >
> > #if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> >
> > #define DECLARE_TRACE(...) ....
> >
> > #endif
> >
> > Then you can remove the first definition of it. The first time
> > processing the file, when it hits the #ifndef DECLARE_TRACE in the
> > _LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
> > and then when it exits that conditional, it does.
>
>
> Yeah, that makes sense to me. Although I think you mean if
> defined(CONFIG_TRACEPOINTS) rather than if !defined(CONFIG_TRACEPOINTS). The

Yep, that's what I meant ;-)

> case I'm moving to outside the unprotected area is the one that we use if
> tracepoints are enabled. I just tested this new patch, and it works well for
> me. Thanks!
>
> Neil
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

OK, I'll give it some testing tonight.

-- Steve




\
 
 \ /
  Last update: 2009-12-03 20:45    [W:0.101 / U:41.328 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site