Messages in this thread | | | Date | Fri, 10 Nov 2017 22:32:06 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH] tcp: Export to userspace the TCP state names for the trace events |
| |
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
| |