1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v3 0/8] mptcp: MIB counters for sent MP_JOIN
  4. View patch

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

Matthieu Baerts (NGI0) posted 8 patches 4 months ago
There is a newer version of this series
[PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params
Posted by Matthieu Baerts (NGI0) 4 months ago 
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 \
+			join_corrupted_pkts="$(pedit_action_pkts)" \
+			chk_join_nr 0 0 0
 		chk_fail_nr 1 -1 invert
 	fi
 
@@ -3170,7 +3180,10 @@ fail_tests()
 		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
 		test_linkfail=1024 \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1 1 0 1 1 0 "$(pedit_action_pkts)"
+		join_csum_ns1=1 join_csum_ns2=0 \
+			join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
+			join_corrupted_pkts="$(pedit_action_pkts)" \
+			chk_join_nr 1 1 1
 	fi
 }
 

-- 
2.45.2
Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params
Posted by Geliang Tang 3 months, 4 weeks ago 
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"?

> +			join_corrupted_pkts="$(pedit_action_pkts)" \
> +			chk_join_nr 0 0 0
>  		chk_fail_nr 1 -1 invert
>  	fi
>  
> @@ -3170,7 +3180,10 @@ fail_tests()
>  		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags
> subflow
>  		test_linkfail=1024 \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1 1 0 1 1 0 "$(pedit_action_pkts)"
> +		join_csum_ns1=1 join_csum_ns2=0 \
> +			join_fail_nr=1 join_rst_nr=1 join_infi_nr=0
> \

And this "join_infi_nr=0".

> +			join_corrupted_pkts="$(pedit_action_pkts)" \
> +			chk_join_nr 1 1 1
>  	fi
>  }
>  
>
Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params
Posted by Matthieu Baerts 3 months, 4 weeks ago 
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.
Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params
Posted by Geliang Tang 3 months, 3 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.