Re: [PATCH] tcp: Export to userspace the TCP state names for the trace events

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 11 Nov 2017 02:06:00 +0000
Yafang Shao <laoar.shao@gmail.com> wrote:

> 2017-11-10 15:07 GMT+00:00 Steven Rostedt <rostedt@goodmis.org>:
> > On Fri, 10 Nov 2017 12:56:06 +0800
> > Yafang Shao <laoar.shao@gmail.com> wrote:
> >  
> >> Could the macro tcp_state_name() be renamed ？
> >> If <trace/events/tcp.h> is included in include/net/tcp.h, it will  
> >
> > Ideally, you don't want to include trace/events/*.h headers in other
> > headers, as they can have side effects if those headers are included in
> > other trace/events/*.h headers.
> >  
> 
> Actually I find trace/events/*.h is included in lots of other headers,
> for example,
> 
> net/rxrpc/ar-internal.h

This is an internal header, so it's not that likely to be used where it
shouldn't be.

> include/linux/bpf_trace.h
> fs/f2fs/trace.h

The above two are actually headers specifically used to pull in the
trace/events/*.h headers.

> fs/afs/internal.h

another internal header. Unlikely to be misused.

> arch/x86/include/asm/mmu_context.h

This one, hmm, probably should be fixed.

> ...
> 
> Are these files doing properly ?

Most yes, some probably not.

> Should we fix them ?

Probably, but if they are used incorrectly, it would usually fail on
build (The same global functions and variables would be defined).

> 
> But per my understanding, it is ok to include  trace/events/*.h in
> other headers because we defined TRACE_SYSTEM as well, as a
> consequence those headers should not included in trace/events/*.h. If
> that happens, it may means that one of the these two TRACE_SYSTEM is
> not defined properly. Maybe these two TRACE_SYSTEM should be merged to
> one TRACE_SYSTEM.

Two different files may have the same TRACE_SYSTEM defined. That's not
an issue.

The issue is, if you have a trace/events/*.h header in a popular file
(like it use to be in include/linux/slab.h), then it can cause issues
if another trace/events/*.h header includes it. That's because each
trace/events/*.h header must be included with CREATE_TRACE_POINTS only
once.

> 
> 
> >> cause compile error, because there's another function tcp_state_name()
> >> defined in net/netfilter/ipvs/ip_vs_proto_tcp.c.
> >>     static const char * tcp_state_name(int state)
> >>     {
> >>
> >>         if (state >= IP_VS_TCP_S_LAST)
> >>
> >>             return "ERR!";
> >>
> >>         return tcp_state_name_table[state] ? tcp_state_name_table[state] : "?";
> >>
> >>     }  
> >
> > But that said, I didn't make up the trace_state_name(), it was already
> > there in net-next before this patch.
> >  
> 
> I know that is not your fault.

:-)

> But as you are modifying this file, it is better to modify it in your
> patch as well.
> So we need not submit another new patch to fix it.

I could whip up a patch 2.

> 
> > But yeah, in actuality, I would have just done:
> >
> > #define EM(a)   { a, #a },
> > #define EMe(a)  { a, #a }
> >
> > directly. Which we can still do.
> >
> > -- Steve
> >  
> 
> The suggestion from Song is good to fix it.

Song's suggestion seems like it can simple be a patch added on top of
mine. As it is somewhat agnostic to the fix I'm making. That is, it's a
different problem, and thus should be a different patch.

-- Steve