On Fri, Aug 23, 2024 at 03:34:10PM +0200, Aperence wrote: > Currently, the TCP_MAXSEG socket option doesn't seem to be supported > with MPTCP. This results in a warning when trying to set the MSS of > sockets in proto_tcp:tcp_bind_listener. > > This can be resolved by adding two new variables: > sock_inet(6)_mptcp_maxseg_default that will hold the default > value of the TCP_MAXSEG option. Note that for the moment, this > will always be -1 as the option isn't supported. However, in the > future, when the support for this option will be added, it should > contain the correct value for the MSS, allowing to correctly > set the TCP_MAXSEG option.
I'm perfectly fine with the whole patch except that it's not logical to add it as a bug fix after the first one which raises the API incompatibility. I think it should instead just be part of the first one (unless you figure a way to introduce it before, which I doubt) and you can simply append the commit message to the first one explaining this specific case. A few nits below: > diff --git a/src/sock_inet.c b/src/sock_inet.c > index 028ffaa68..9f10c5654 100644 > --- a/src/sock_inet.c > +++ b/src/sock_inet.c > @@ -77,6 +77,10 @@ int sock_inet6_v6only_default = 0; > int sock_inet_tcp_maxseg_default = -1; > int sock_inet6_tcp_maxseg_default = -1; > > +/* Default MPTCPv4/MPTCPv6 MSS settings. -1=unknown. */ > +int sock_inet_mptcp_maxseg_default = -1; > +int sock_inet6_mptcp_maxseg_default = -1; > + Maybe these should be conditioned to the use of MPTCP (just saving 8 bytes but...). > /* Compares two AF_INET sockaddr addresses. Returns 0 if they match or > non-zero > * if they do not match. > */ > @@ -494,6 +498,30 @@ static void sock_inet_prepare() > #endif > close(fd); > } > + > +#ifdef __linux__ Here I think a short comment is deserved to explain why __linux__, because it's the same choice that was made in proto_tcp.c. Or better, you could probably do that in compat.h: #ifdef __linux__ # define HA_HAVE_MPTCP 1 # ifndef IPPROTO_TCP # define IPPROTO_TCP ... # endif #endif and use #ifdef HA_HAVE_MPTCP here and around the variables above. That would make it quite clear about what we're really checking for here. Thanks! Willy
