2017-11-11 3:32 GMT+00:00 Steven Rostedt <rost...@goodmis.org>: > 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. >
Understood. Thanks for explanation. >> >> >> >> 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. > Got it. These two issues should be fixed in two different patches :-) Thanks Yafang