| Message ID | 20200121170542.2762865-1-matthieu.baerts@tessares.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: uniform the use of CONFIG_MPTCP_IPV6 | expand |
On Tue, 21 Jan 2020, Matthieu Baerts wrote: > We should not check CONFIG_IPV6 but CONFIG_MPTCP_IPV6. > And better to always use: > > #if IS_ENABLED(CONFIG_MPTCP_IPV6) > > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > --- > > Notes: > to be squashed in "mptcp: Create SUBFLOW socket for incoming connections" > > net/mptcp/protocol.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 7f1efe1aae5e..84799a5212a2 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -227,7 +227,7 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk) > inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr; > inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr; > > -#if IS_ENABLED(CONFIG_IPV6) > +#if IS_ENABLED(CONFIG_MPTCP_IPV6) > msk->sk_v6_daddr = ssk->sk_v6_daddr; > msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr; I noticed that the declarations for msk6 and ssk6 at the beginning of this function are not #ifdef'd either - I suggest moving all the IPv6 code to the beginning of the function so the declaration and the assignments can all be in one ifdef. Mat > > @@ -474,7 +474,7 @@ static int mptcp_listen(struct socket *sock, int backlog) > > static bool is_tcp_proto(const struct proto *p) > { > -#ifdef CONFIG_MPTCP_IPV6 > +#if IS_ENABLED(CONFIG_MPTCP_IPV6) > return p == &tcp_prot || p == &tcpv6_prot; > #else > return p == &tcp_prot; > -- > 2.24.0 -- Mat Martineau Intel
Hi Mat, On 21/01/2020 18:51, Mat Martineau wrote: > > On Tue, 21 Jan 2020, Matthieu Baerts wrote: > >> We should not check CONFIG_IPV6 but CONFIG_MPTCP_IPV6. >> And better to always use: >> >> #if IS_ENABLED(CONFIG_MPTCP_IPV6) >> >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> >> --- >> >> Notes: >> to be squashed in "mptcp: Create SUBFLOW socket for incoming >> connections" >> >> net/mptcp/protocol.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 7f1efe1aae5e..84799a5212a2 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -227,7 +227,7 @@ static void mptcp_copy_inaddrs(struct sock *msk, >> const struct sock *ssk) >> inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr; >> inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr; >> >> -#if IS_ENABLED(CONFIG_IPV6) >> +#if IS_ENABLED(CONFIG_MPTCP_IPV6) >> msk->sk_v6_daddr = ssk->sk_v6_daddr; >> msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr; > > I noticed that the declarations for msk6 and ssk6 at the beginning of > this function are not #ifdef'd either - I suggest moving all the IPv6 > code to the beginning of the function so the declaration and the > assignments can all be in one ifdef. Thank you for the review! I see that inet6_sk() is an inline function which will return NULL if CONFIG_IPV6 is not enabled. Also "struct ipv6_pinfo" is always available. Do we need to add #ifdef then? Or to avoid declaring variables that are not used if IPV6 is not enabled? Cheers, Matt
Hi Mat, On 21/01/2020 22:10, Matthieu Baerts wrote: > Hi Mat, > > On 21/01/2020 18:51, Mat Martineau wrote: >> >> On Tue, 21 Jan 2020, Matthieu Baerts wrote: >> >>> We should not check CONFIG_IPV6 but CONFIG_MPTCP_IPV6. >>> And better to always use: >>> >>> #if IS_ENABLED(CONFIG_MPTCP_IPV6) >>> >>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> >>> --- >>> >>> Notes: >>> to be squashed in "mptcp: Create SUBFLOW socket for incoming >>> connections" >>> >>> net/mptcp/protocol.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >>> index 7f1efe1aae5e..84799a5212a2 100644 >>> --- a/net/mptcp/protocol.c >>> +++ b/net/mptcp/protocol.c >>> @@ -227,7 +227,7 @@ static void mptcp_copy_inaddrs(struct sock *msk, >>> const struct sock *ssk) >>> inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr; >>> inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr; >>> >>> -#if IS_ENABLED(CONFIG_IPV6) >>> +#if IS_ENABLED(CONFIG_MPTCP_IPV6) >>> msk->sk_v6_daddr = ssk->sk_v6_daddr; >>> msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr; >> >> I noticed that the declarations for msk6 and ssk6 at the beginning of >> this function are not #ifdef'd either - I suggest moving all the IPv6 >> code to the beginning of the function so the declaration and the >> assignments can all be in one ifdef. > > Thank you for the review! > > I see that inet6_sk() is an inline function which will return NULL if > CONFIG_IPV6 is not enabled. > Also "struct ipv6_pinfo" is always available. > > Do we need to add #ifdef then? Or to avoid declaring variables that are > not used if IPV6 is not enabled? - bc1f4ea1bcdb: "squashed" in "mptcp: Create SUBFLOW socket for incoming connections" Tests and export are in progress! Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 7f1efe1aae5e..84799a5212a2 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -227,7 +227,7 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk) inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr; inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr; -#if IS_ENABLED(CONFIG_IPV6) +#if IS_ENABLED(CONFIG_MPTCP_IPV6) msk->sk_v6_daddr = ssk->sk_v6_daddr; msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr; @@ -474,7 +474,7 @@ static int mptcp_listen(struct socket *sock, int backlog) static bool is_tcp_proto(const struct proto *p) { -#ifdef CONFIG_MPTCP_IPV6 +#if IS_ENABLED(CONFIG_MPTCP_IPV6) return p == &tcp_prot || p == &tcpv6_prot; #else return p == &tcp_prot;
We should not check CONFIG_IPV6 but CONFIG_MPTCP_IPV6. And better to always use: #if IS_ENABLED(CONFIG_MPTCP_IPV6) Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> --- Notes: to be squashed in "mptcp: Create SUBFLOW socket for incoming connections" net/mptcp/protocol.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)