[PATCH mptcp-next v20 09/10] selftests: mptcp: pass fastclose to sflags

Geliang Tang posted 10 patches 2 years, 7 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>
[PATCH mptcp-next v20 09/10] selftests: mptcp: pass fastclose to sflags
Posted by Geliang Tang 2 years, 7 months ago
Pass fastclose flag to the sflags argument instead of addr_nr_ns2.
Then the fullmesh flag setting can be moved into pm_nl_set_endpoint()
from do_transfer().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c3c8471f9ad1..89379815b619 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -828,6 +828,12 @@ pm_nl_set_endpoint()
 	local connector_ns="$2"
 	local connect_addr="$3"
 
+	local flags="subflow"
+	if [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
+		flags="${flags},fullmesh"
+		addr_nr_ns2=${addr_nr_ns2:9}
+	fi
+
 	# let the mptcp subflow be established in background before
 	# do endpoint manipulation
 	if [ $addr_nr_ns1 != "0" ] || [ $addr_nr_ns2 != "0" ]; then
@@ -1015,11 +1021,10 @@ do_transfer()
 		extra_args="-r ${speed:6}"
 	fi
 
-	local flags="subflow"
 	local extra_cl_args=""
 	local extra_srv_args=""
 	local trunc_size=""
-	if [[ "${addr_nr_ns2}" = "fastclose_"* ]]; then
+	if [[ "${sflags}" = "fastclose_"* ]]; then
 		if [ ${test_linkfail} -le 1 ]; then
 			echo "fastclose tests need test_linkfail argument"
 			fail_test
@@ -1028,7 +1033,7 @@ do_transfer()
 
 		# disconnect
 		trunc_size=${test_linkfail}
-		local side=${addr_nr_ns2:10}
+		local side=${sflags:10}
 
 		if [ ${side} = "client" ]; then
 			extra_cl_args="-f ${test_linkfail}"
@@ -1041,10 +1046,6 @@ do_transfer()
 			fail_test
 			return 1
 		fi
-		addr_nr_ns2=0
-	elif [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
-		flags="${flags},fullmesh"
-		addr_nr_ns2=${addr_nr_ns2:9}
 	fi
 
 	extra_srv_args="$extra_args $extra_srv_args"
@@ -3186,7 +3187,7 @@ fullmesh_tests()
 fastclose_tests()
 {
 	if reset_check_counter "fastclose test" "MPTcpExtMPFastcloseTx"; then
-		test_linkfail=1024 addr_nr_ns2=fastclose_client \
+		test_linkfail=1024 sflags=fastclose_client \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
 		chk_fclose_nr 1 1
@@ -3194,7 +3195,7 @@ fastclose_tests()
 	fi
 
 	if reset_check_counter "fastclose server test" "MPTcpExtMPFastcloseRx"; then
-		test_linkfail=1024 addr_nr_ns2=fastclose_server \
+		test_linkfail=1024 sflags=fastclose_server \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
 		chk_fclose_nr 1 1 invert
-- 
2.35.3
Re: [PATCH mptcp-next v20 09/10] selftests: mptcp: pass fastclose to sflags
Posted by Matthieu Baerts 2 years, 7 months ago
Hi Geliang,

On 16/06/2023 09:31, Geliang Tang wrote:
> Pass fastclose flag to the sflags argument instead of addr_nr_ns2.

Please *always* explain the reason(s) the commit message.
Here I guess you do that because it is clearer, right?

> Then the fullmesh flag setting can be moved into pm_nl_set_endpoint()
> from do_transfer().

Just to know, do you plan to simplify this part as well? e.g. to be able
to do something like:

  fullmesh=2 (...) \
      run_tests (...)

  if [ -n "${fullmesh}" ]; then
      flags="${flags},fullmesh"
      addr_nr_ns2=${fullmesh}
  fi

or:

  sflags=fullmesh addr_nr_ns2=2 (...) \
      run_tests (...)

and stop doing stuffs like:

  if [[ "${foo}" = "bar_"* ]]; then
      extracted_var=${foo:42}

>  		extra_args="-r ${speed:6}"
>  	fi
>  
> -	local flags="subflow"
>  	local extra_cl_args=""
>  	local extra_srv_args=""
>  	local trunc_size=""
> -	if [[ "${addr_nr_ns2}" = "fastclose_"* ]]; then
> +	if [[ "${sflags}" = "fastclose_"* ]]; then
>  		if [ ${test_linkfail} -le 1 ]; then
>  			echo "fastclose tests need test_linkfail argument"
>  			fail_test

Why not using a new "key"? e.g.

  fastclose=client \
      run_tests (...)

You would have here something clearer:

  if [ -n "${fastclose}" ]; then
      local side=${fastclose}

(not directly related but it is the same for speed_XX (e.g. speed_10),
we could pass speed=10 and even speed=slow but that's for a different
commit if you are interested to change that too ;) )

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net