Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params

[Geliang Tang <geliang@kernel.org>]

On Thu, 2024-08-08 at 12:22 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/08/2024 05:28, Geliang Tang wrote:
> > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > chk_join_nr() currently takes 9 positional parameters, 6 of them
> > > are
> > > optional. It makes it hard to read:
> > > 
> > >   chk_join_nr 1 1 1 1 0 1 1 0 4
> > > 
> > > Naming these vars helps to make it easier to read:
> > > 
> > >   join_csum_ns1=1 join_csum_ns2=0 \
> > >     join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
> > >     join_corrupted_pkts=4 \
> > >     chk_join_nr 1 1 1
> > > 
> > > It will then be easier to add new optional parameters.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 31
> > > ++++++++++++++++++-------
> > >  1 file changed, 22 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index a1f80dac59a7..0401ba1aaf1b 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -61,6 +61,12 @@ unset sflags
> > >  unset fastclose
> > >  unset fullmesh
> > >  unset speed
> > > +unset join_csum_ns1
> > > +unset join_csum_ns2
> > > +unset join_fail_nr
> > > +unset join_rst_nr
> > > +unset join_infi_nr
> > > +unset join_corrupted_pkts
> > >  
> > >  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) ==
> > > 0x30) ||
> > >  #				  (ip6 && (ip6[74] & 0xf0) ==
> > > 0x30)'"
> > > @@ -1314,12 +1320,12 @@ chk_join_nr()
> > >  	local syn_nr=$1
> > >  	local syn_ack_nr=$2
> > >  	local ack_nr=$3
> > > -	local csum_ns1=${4:-0}
> > > -	local csum_ns2=${5:-0}
> > > -	local fail_nr=${6:-0}
> > > -	local rst_nr=${7:-0}
> > > -	local infi_nr=${8:-0}
> > > -	local corrupted_pkts=${9:-0}
> > > +	local csum_ns1=${join_csum_ns1:-0}
> > > +	local csum_ns2=${join_csum_ns2:-0}
> > > +	local fail_nr=${join_fail_nr:-0}
> > > +	local rst_nr=${join_rst_nr:-0}
> > > +	local infi_nr=${join_infi_nr:-0}
> > > +	local corrupted_pkts=${join_corrupted_pkts:-0}
> > >  	local count
> > >  	local with_cookie
> > >  
> > > @@ -3138,7 +3144,8 @@ fastclose_tests()
> > >  		MPTCP_LIB_SUBTEST_FLAKY=1
> > >  		test_linkfail=1024 fastclose=server \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > > -		chk_join_nr 0 0 0 0 0 0 1
> > > +		join_rst_nr=1 \
> > > +			chk_join_nr 0 0 0
> > >  		chk_fclose_nr 1 1 invert
> > >  		chk_rst_nr 1 1
> > >  	fi
> > > @@ -3157,7 +3164,10 @@ fail_tests()
> > >  		MPTCP_LIB_SUBTEST_FLAKY=1
> > >  		test_linkfail=128 \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > > -		chk_join_nr 0 0 0 +1 +0 1 0 1
> > > "$(pedit_action_pkts)"
> > > +		join_csum_ns1=+1 join_csum_ns2=+0 \
> > > +			join_fail_nr=1 join_rst_nr=0
> > > join_infi_nr=1
> > > \
> > 
> > Can we drop this "join_rst_nr=0"?
> 
> We could, but I prefer not: I think it is better to specify all
> variables here, to make it clear we expect no RST, compared to the
> next
> test where instead we expect an infinite mapping. It is easier to
> compare the expectations from the two tests.
> 
> It is different from the 'fastclose_tests()' test, where it is just
> an
> exception there, just one test where there is a rst.

Sure! thanks for the explanation.

-Geliang

> 
> Cheers,
> Matt