| Message ID | 9e790506ede2adbc601e5f3f576c023118d87280.1579624298.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | Squash-to: "mptcp: cope with later TCP fallback" | expand |
On Tue, 21 Jan 2020, Paolo Abeni wrote: > kbuildbot reported a build breakage with IPV6 disabled. > > Signed-off-by: Paolo Abeni <pabeni@redha.com> > --- > net/mptcp/protocol.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 6da609791c7a..4e289ff748ad 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -26,6 +26,15 @@ > > static void __mptcp_close(struct sock *sk, long timeout); > > +static const struct proto_ops * tcp_proto_ops(struct sock *sk) > +{ > +#if IS_ENABLED(IPV6) Does this need to be IS_ENABLED(CONFIG_IPV6) ? > + if (sk->sk_family == AF_INET6) > + return &inet6_stream_ops; > +#endif > + return &inet_stream_ops; > +} > + > /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing > * socket->sk and stream ops and destroying msk > * return the msk socket, as we can't access msk anymore after this function > @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk, > subflow->conn = NULL; > } > release_sock(ssk); > - sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops : > - &inet_stream_ops; > + sock->ops = tcp_proto_ops(ssk); > > /* destroy the left-over msk sock */ > __mptcp_close(sk, 0); > -- > 2.21.0 -- Mat Martineau Intel
On Tue, 21 Jan 2020, Mat Martineau wrote: > > On Tue, 21 Jan 2020, Paolo Abeni wrote: > >> kbuildbot reported a build breakage with IPV6 disabled. >> >> Signed-off-by: Paolo Abeni <pabeni@redha.com> One more thing - typo in signoff ^^^^^ >> --- >> net/mptcp/protocol.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 6da609791c7a..4e289ff748ad 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -26,6 +26,15 @@ >> >> static void __mptcp_close(struct sock *sk, long timeout); >> >> +static const struct proto_ops * tcp_proto_ops(struct sock *sk) >> +{ >> +#if IS_ENABLED(IPV6) > > Does this need to be IS_ENABLED(CONFIG_IPV6) ? > >> + if (sk->sk_family == AF_INET6) >> + return &inet6_stream_ops; >> +#endif >> + return &inet_stream_ops; >> +} >> + >> /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing >> * socket->sk and stream ops and destroying msk >> * return the msk socket, as we can't access msk anymore after this >> function >> @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct >> mptcp_sock *msk, >> subflow->conn = NULL; >> } >> release_sock(ssk); >> - sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops : >> - &inet_stream_ops; >> + sock->ops = tcp_proto_ops(ssk); >> >> /* destroy the left-over msk sock */ >> __mptcp_close(sk, 0); >> -- >> 2.21.0 > > -- > Mat Martineau > Intel > -- Mat Martineau Intel
On Tue, 2020-01-21 at 08:49 -0800, Mat Martineau wrote: > On Tue, 21 Jan 2020, Paolo Abeni wrote: > > > kbuildbot reported a build breakage with IPV6 disabled. > > > > Signed-off-by: Paolo Abeni <pabeni@redha.com> > > --- > > net/mptcp/protocol.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 6da609791c7a..4e289ff748ad 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -26,6 +26,15 @@ > > > > static void __mptcp_close(struct sock *sk, long timeout); > > > > +static const struct proto_ops * tcp_proto_ops(struct sock *sk) > > +{ > > +#if IS_ENABLED(IPV6) > > Does this need to be IS_ENABLED(CONFIG_IPV6) ? Or IS_ENABLED(CONFIG_MPTCP_IPV6)? Peter. > > + if (sk->sk_family == AF_INET6) > > + return &inet6_stream_ops; > > +#endif > > + return &inet_stream_ops; > > +} > > + > > /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing > > * socket->sk and stream ops and destroying msk > > * return the msk socket, as we can't access msk anymore after this function > > @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk, > > subflow->conn = NULL; > > } > > release_sock(ssk); > > - sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops : > > - &inet_stream_ops; > > + sock->ops = tcp_proto_ops(ssk); > > > > /* destroy the left-over msk sock */ > > __mptcp_close(sk, 0); > > -- > > 2.21.0 > > -- > Mat Martineau > Intel > _______________________________________________ > mptcp mailing list -- mptcp@lists.01.org > To unsubscribe send an email to mptcp-leave@lists.01.org
Hi Paolo, On 21/01/2020 17:51, Mat Martineau wrote: > On Tue, 21 Jan 2020, Mat Martineau wrote: > >> >> On Tue, 21 Jan 2020, Paolo Abeni wrote: >> >>> kbuildbot reported a build breakage with IPV6 disabled. >>> >>> Signed-off-by: Paolo Abeni <pabeni@redha.com> > > One more thing - typo in signoff ^^^^^ If "redhat.com" is too long, I guess you can also write "ibm.com"! ... :-D Cheers, Matt
Hi Paolo, Mat,
On 21/01/2020 17:32, Paolo Abeni wrote:
> kbuildbot reported a build breakage with IPV6 disabled.
Do you know why my CI didn't get the issue?
It runs:
make defconfig
echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc.
echo | scripts/config -e MPTCP -e MPTCP_IPV6
echo | scripts/config -d IPV6 -d MPTCP_IPV6
make
MPTCP is enabled and IPV6 is not.
Which combination did I miss?
Cheers,
Matt
On Tue, 2020-01-21 at 08:51 -0800, Mat Martineau wrote: > On Tue, 21 Jan 2020, Mat Martineau wrote: > > > On Tue, 21 Jan 2020, Paolo Abeni wrote: > > > > > kbuildbot reported a build breakage with IPV6 disabled. > > > > > > Signed-off-by: Paolo Abeni <pabeni@redha.com> > > One more thing - typo in signoff ^^^^^ Thanks, will fix. > > > --- > > > net/mptcp/protocol.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > > index 6da609791c7a..4e289ff748ad 100644 > > > --- a/net/mptcp/protocol.c > > > +++ b/net/mptcp/protocol.c > > > @@ -26,6 +26,15 @@ > > > > > > static void __mptcp_close(struct sock *sk, long timeout); > > > > > > +static const struct proto_ops * tcp_proto_ops(struct sock *sk) > > > +{ > > > +#if IS_ENABLED(IPV6) > > > > Does this need to be IS_ENABLED(CONFIG_IPV6) ? yep, CONFIG_IPV6 is the thing. v2 is coming. Cheers, Paolo
On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote: > Hi Paolo, Mat, > > On 21/01/2020 17:32, Paolo Abeni wrote: > > kbuildbot reported a build breakage with IPV6 disabled. > > Do you know why my CI didn't get the issue? > > It runs: > > make defconfig > echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc. > echo | scripts/config -e MPTCP -e MPTCP_IPV6 > echo | scripts/config -d IPV6 -d MPTCP_IPV6 > make > > MPTCP is enabled and IPV6 is not. > > Which combination did I miss? weird. I can reproduce the build failure with CONFIG_MPTCP=y # CONFIG_IPV6 is not set # CONFIG_MPTCP_IPV6 is not set net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp': /home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to `inet6_stream_ops' the inet6_stream_ops struct is unconditionally referenced by the mptcp code. /P
Hi Paolo, On 21/01/2020 21:40, Paolo Abeni wrote: > On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote: >> Hi Paolo, Mat, >> >> On 21/01/2020 17:32, Paolo Abeni wrote: >>> kbuildbot reported a build breakage with IPV6 disabled. >> >> Do you know why my CI didn't get the issue? >> >> It runs: >> >> make defconfig >> echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc. >> echo | scripts/config -e MPTCP -e MPTCP_IPV6 >> echo | scripts/config -d IPV6 -d MPTCP_IPV6 >> make >> >> MPTCP is enabled and IPV6 is not. >> >> Which combination did I miss? > > weird. I can reproduce the build failure with > > CONFIG_MPTCP=y > # CONFIG_IPV6 is not set > # CONFIG_MPTCP_IPV6 is not set > > > net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp': > /home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to > `inet6_stream_ops' > > the inet6_stream_ops struct is unconditionally referenced by the mptcp > code. I don't understand why but my CI didn't catch errors when compiling without IPv6 :-/ Here is the script I use: https://github.com/multipath-tcp/mptcp_net-next/blob/d77caf0c96ed3825c6c750445a7a5e772da27477/ci/update-tg-tree.sh#L278 And here is what I have in the logs: + tg checkout next + break + is_tg_top t/upstream + '[' t/upstream = t/upstream ']' + check_compilation_no_ipv6 + generate_config_mptcp + generate_config_no_mptcp + make defconfig *** Default configuration is based on 'x86_64_defconfig' # # configuration written to .config # + echo + scripts/config --disable DRM --disable PCCARD --disable ATA --disable MD --disable PPS --disable SOUND --disable USB --disable IOMMU_SUPPORT --disable INPUT_LEDS --disable AGP --disable VGA_ARB --disable EFI --disable WLAN --disable WIRELESS --disable LOGO --disable NFS_FS --disable XFRM_USER --disable INET6_AH --disable INET6_ESP --disable NETDEVICES + echo + scripts/config -e MPTCP -e MPTCP_IPV6 + echo + scripts/config -d IPV6 -d MPTCP_IPV6 + compile_kernel 'without IPv6 and with CONFIG_MPTCP' ++ nproc ++ nproc + KCFLAGS=-Werror + make -j8 -l8 scripts/kconfig/conf --syncconfig Kconfig DESCEND objtool CALL scripts/atomic/check-atomics.sh CALL scripts/checksyscalls.sh CHK include/generated/compile.h [...] CC net/mptcp/protocol.o CC net/mptcp/subflow.o CC kernel/sched/pelt.o CC kernel/sched/stats.o CC kernel/auditfilter.o CC net/core/dev_ioctl.o CC kernel/sched/cpuacct.o CC net/ipv4/tcp_ulp.o net/mptcp/protocol.c: In function 'mptcp_copy_inaddrs': net/mptcp/protocol.c:1133:21: error: unused variable 'msk6' [-Werror=unused-variable] struct ipv6_pinfo *msk6 = inet6_sk(msk); ^~~~ net/mptcp/protocol.c:1132:27: error: unused variable 'ssk6' [-Werror=unused-variable] const struct ipv6_pinfo *ssk6 = inet6_sk(ssk); ^~~~ cc1: all warnings being treated as errors scripts/Makefile.build:265: recipe for target 'net/mptcp/protocol.o' failed make[2]: *** [net/mptcp/protocol.o] Error 1 make[2]: *** Waiting for unfinished jobs.... CC net/ipv4/tcp_offload.o CC net/ipv4/datagram.o scripts/Makefile.build:503: recipe for target 'net/mptcp' failed make[1]: *** [net/mptcp] Error 2 make[1]: *** Waiting for unfinished jobs.... CC net/ipv4/raw.o [...] AR net/ipv4/built-in.a make: *** [net] Error 2 Makefile:1693: recipe for target 'net' failed + err 'Unable to compile without IPv6 and with CONFIG_MPTCP' + echo 'ERROR: Unable to compile without IPv6 and with CONFIG_MPTCP' ERROR: Unable to compile without IPv6 and with CONFIG_MPTCP + return 1 + return 1 + check_compilation_i386 [...] I have no idea why "check_compilation_i386" is executed while "set -e" is launched at the beginning of the script (even set by the caller: "bash -ehxB patches/update-tg-tree.sh") and it should prevent that. If you have any idea why, I would be very interested by the explanation :-) Cheers, Matt
On 21/01/2020 23:13, Matthieu Baerts wrote: > Hi Paolo, > > On 21/01/2020 21:40, Paolo Abeni wrote: >> On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote: >>> Hi Paolo, Mat, >>> >>> On 21/01/2020 17:32, Paolo Abeni wrote: >>>> kbuildbot reported a build breakage with IPV6 disabled. >>> >>> Do you know why my CI didn't get the issue? >>> >>> It runs: >>> >>> make defconfig >>> echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc. >>> echo | scripts/config -e MPTCP -e MPTCP_IPV6 >>> echo | scripts/config -d IPV6 -d MPTCP_IPV6 >>> make >>> >>> MPTCP is enabled and IPV6 is not. >>> >>> Which combination did I miss? >> >> weird. I can reproduce the build failure with >> >> CONFIG_MPTCP=y >> # CONFIG_IPV6 is not set >> # CONFIG_MPTCP_IPV6 is not set >> >> >> net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp': >> /home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to >> `inet6_stream_ops' >> >> the inet6_stream_ops struct is unconditionally referenced by the mptcp >> code. > > I don't understand why but my CI didn't catch errors when compiling > without IPv6 :-/ Mat found the issue: In short if you have this script: === #!/bin/bash set -e ff() { echo before false echo after } ff === only "before" will be printed. But with this one: === #!/bin/bash set -e ff() { echo before false echo after } ff || echo "error" === both "before" and "after" are printed. Mat also found the explanation in the manual: Exit immediately unless the command is "part of any command executed in a && or || list except the command following the final && or ||" In other words, better to always use 'if ! X; then Y; fi' then 'X || Y'. Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 6da609791c7a..4e289ff748ad 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -26,6 +26,15 @@ static void __mptcp_close(struct sock *sk, long timeout); +static const struct proto_ops * tcp_proto_ops(struct sock *sk) +{ +#if IS_ENABLED(IPV6) + if (sk->sk_family == AF_INET6) + return &inet6_stream_ops; +#endif + return &inet_stream_ops; +} + /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing * socket->sk and stream ops and destroying msk * return the msk socket, as we can't access msk anymore after this function @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk, subflow->conn = NULL; } release_sock(ssk); - sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops : - &inet_stream_ops; + sock->ops = tcp_proto_ops(ssk); /* destroy the left-over msk sock */ __mptcp_close(sk, 0);
kbuildbot reported a build breakage with IPV6 disabled. Signed-off-by: Paolo Abeni <pabeni@redha.com> --- net/mptcp/protocol.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)