Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20171110100700.5870c21f@gandalf.local.home>
References: <1510207298-14828-1-git-send-email-laoar.shao@gmail.com>
 <20171109064326.skfvvohg7qhgl3lp@ast-mbp> <20171109131808.373c296f@gandalf.local.home>
 <30A95900-7378-439F-954E-BD5487012435@fb.com> <CFF85160-E239-4E15-8438-CB8C2795D4FD@fb.com>
 <20171109195712.47fe23b6@gandalf.local.home> <CALOAHbCM1d8rs1_KMo0MMCwgTG18TeiH_ZQJ2=sgPst2PoU9Xg@mail.gmail.com>
 <20171110100700.5870c21f@gandalf.local.home>
From:   Yafang Shao <laoar.shao@gmail.com>
Date:   Sat, 11 Nov 2017 02:06:00 +0000
Message-ID: <CALOAHbD16KNZ4JWTLudQLDgB72Ke7Twdb-bLYznH7voAUQvQog@mail.gmail.com>
Subject: Re: [PATCH] tcp: Export to userspace the TCP state names for the
 trace events
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     David Miller <davem@davemloft.net>,
        Song Liu <songliubraving@fb.com>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 =EF=BC=9F
>> 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 >=3D 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

From 1583701547807103322@xxx Fri Nov 10 17:38:10 +0000 2017
X-GM-THRID: 1583638629223849157
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread