On Sat, 11 Nov 2017 02:06:00 +0000 Yafang Shao <laoar.s...@gmail.com> wrote:
> 2017-11-10 15:07 GMT+00:00 Steven Rostedt <rost...@goodmis.org>: > > On Fri, 10 Nov 2017 12:56:06 +0800 > > Yafang Shao <laoar.s...@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