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

[Matthieu Baerts <matttbe@kernel.org>]

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.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.