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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 11 Nov 2017 02:06:00 +0000
From:   Yafang Shao <laoar.shao@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     David Miller <davem@...emloft.net>,
        Song Liu <songliubraving@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tcp: Export to userspace the TCP state names for the
 trace events

2017-11-10 15:07 GMT+00:00 Steven Rostedt <rostedt@...dmis.org>:
> On Fri, 10 Nov 2017 12:56:06 +0800
> Yafang Shao <laoar.shao@...il.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
include/linux/bpf_trace.h
fs/f2fs/trace.h
fs/afs/internal.h
arch/x86/include/asm/mmu_context.h
...

Are these files doing properly ?
Should we fix them ?

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.

>> 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.

> 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.

Thanks
Yafang