Re: [MPTCP] [RFC 1/3] mptcp: Slightly refactor mptcp_established_options_xxx()

[Peter Krystad <peter.krystad at linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

On Thu, 2019-05-23 at 11:00 +0200, Paolo Abeni wrote:
> Hi,
> 
> Thank you for doing this and sharing the code early, very appreciated!
> 
> On Wed, 2019-05-22 at 14:38 -0700, Peter Krystad wrote:
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 1cd915890ce5..68a2311b56d4 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -797,20 +797,20 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
> >  		unsigned int remaining = MAX_TCP_OPTION_SPACE - size;
> >  		unsigned int opt_size;
> >  
> > -		if (mptcp_established_options(sk, &opt_size, &opts->mptcp)) {
> > -			if (remaining >= opt_size) {
> > -				opts->options |= OPTION_MPTCP;
> > -				size += opt_size;
> > -			}
> > -		} else {
> > -			unsigned int dss_size;
> > +		if ((sk, skb, &opt_size,
> > +					      remaining, &opts->mptcp)) {
> > +			opts->options |= OPTION_MPTCP;
> > +			size += opt_size;
> > +			remaining -= opt_size;
> > +		}
> > +
> > +		if (mptcp_established_options_dss(sk, skb, &opt_size,
> > +						  remaining, &opts->mptcp)) {
> > +			opts->options |= OPTION_MPTCP;
> > +			size += opt_size;
> > +			remaining -= opt_size;
> > +		}
> >  
> > -			if (mptcp_established_options_dss(sk, skb, &dss_size,
> > -							  remaining,
> > -							  &opts->mptcp)) {
> > -				opts->options |= OPTION_MPTCP;
> > -				size += dss_size;
> > -			}
> >  		}
> >  	}
> > 
> 
> [Not strictly related to MP_JOIN] I think it could be possibly cleaner
> to expose to the TCP code a single mptcp_established_option() hook and
> handle there all the relevant cases ('plain', _dss() _addr() added by
> the next patch).
> 
> The mptcp code should not change much, and possibly it would remove a
> bunch of duplicate checks (about subflow->mp_capable).

I agree a single hook is cleaner, this ended up like this to make the
individual commits stand alone better as features were added. I will revise
this.

Peter.
 
> Cheers,
> 
> Paolo
>