[PATCH mptcp-next v5 00/12] add helpers and vars in mptcp_lib.sh, part 3

[PATCH mptcp-next v5 00/12] add helpers and vars in mptcp_lib.sh, part 3

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v5:
 - don't use mptcp_lib_print_test_counter in mptcp_join.sh, it breaks
   skip_test().

v4:
 - set test_cnt to 0, use ++counter in mptcp_lib_print_test_counter() to
   fix the following mismatched test counters:

# 012 userspace pm server fullmesh
#       syn                                 [ OK ]
#       synack                              [ OK ]
#       ack                                 [ OK ]
#       add                                 [ OK ]
#       echo                                [FAIL] got 1 ADD_ADDR echo[s] expected 2
# Server ns stats
# TcpPassiveOpens                 5                  0.0
# TcpInSegs                       25                 0.0
... ...
#
# 1 failure(s) has(ve) been detected:
#       - 13: userspace pm server fullmesh

v3:
 - fix shellcheck errors in v2

v2:
 - fix shellcheck errors in v1
 - print test results with counters

Geliang Tang (12):
  selftests: mptcp: capitalize ok/fail/skip
  selftests: mptcp: sockopt: print every test result
  selftests: mptcp: connect: fix misaligned OK/FAIL
  selftests: mptcp: print test results with colors
  selftests: mptcp: connect: add dedicated port counter
  selftests: mptcp: connect: print out ping tests info
  selftests: mptcp: print test results with counters
  selftests: mptcp: move test_fail out of check_expected_one
  selftests: mptcp: extract mptcp_lib_check_expected
  selftests: mptcp: export event macros in mptcp_lib
  selftests: mptcp: add mptcp_lib_verify_listener_events
  selftests: mptcp: use KSFT_SKIP/KSFT_PASS/KSFT_FAIL

 tools/testing/selftests/net/mptcp/diag.sh     |  20 +-
 .../selftests/net/mptcp/mptcp_connect.sh      |  85 ++++---
 .../testing/selftests/net/mptcp/mptcp_join.sh |  67 ++----
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  94 +++++++-
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  34 ++-
 .../testing/selftests/net/mptcp/pm_netlink.sh |  11 +-
 .../selftests/net/mptcp/simult_flows.sh       |  15 +-
 .../selftests/net/mptcp/userspace_pm.sh       | 210 +++++++-----------
 8 files changed, 295 insertions(+), 241 deletions(-)

-- 
2.40.1

[PATCH mptcp-next v5 01/12] selftests: mptcp: capitalize ok/fail/skip

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Most scripts print uppercase [ OK ], [FAIL] and [SKIP] as test results,
but lowercase ones are used in diag.sh, mptcp_join.sh and simult_flows.sh.
To maintain consistency with other scripts, this patch capitalizes these
lowercase [ ok ], [ fail ] and [ skip ]:

    [ ok ]   -> [ OK ]   	in diag.sh, mptcp_join.sh
    [ fail ] -> [FAIL]		in diag.sh, mptcp_join.sh, simult_flows.sh
    [ skip ] -> [SKIP]		in diag.sh, mptcp_join.sh

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh        | 12 ++++++------
 .../testing/selftests/net/mptcp/mptcp_connect.sh | 16 ++++++++--------
 tools/testing/selftests/net/mptcp/mptcp_join.sh  |  6 +++---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh   |  2 +-
 .../testing/selftests/net/mptcp/simult_flows.sh  |  2 +-
 5 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index d8f080f934ac..aa3fb808fc6d 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -58,15 +58,15 @@ __chk_nr()
 	printf "%-50s" "$msg"
 	if [ "$nr" != "$expected" ]; then
 		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
-			echo "[ skip ] Feature probably not supported"
+			echo "[SKIP] Feature probably not supported"
 			mptcp_lib_result_skip "${msg}"
 		else
-			echo "[ fail ] expected $expected found $nr"
+			echo "[FAIL] expected $expected found $nr"
 			mptcp_lib_result_fail "${msg}"
 			ret=$test_cnt
 		fi
 	else
-		echo "[  ok  ]"
+		echo "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
 	test_cnt=$((test_cnt+1))
@@ -116,15 +116,15 @@ wait_msk_nr()
 
 	printf "%-50s" "$msg"
 	if [ $i -ge $timeout ]; then
-		echo "[ fail ] timeout while expecting $expected max $max last $nr"
+		echo "[FAIL] timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
 		ret=$test_cnt
 	elif [ $nr != $expected ]; then
-		echo "[ fail ] expected $expected found $nr"
+		echo "[FAIL] expected $expected found $nr"
 		mptcp_lib_result_fail "${msg} # unexpected result"
 		ret=$test_cnt
 	else
-		echo "[  ok  ]"
+		echo "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
 	test_cnt=$((test_cnt+1))
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 0ca2960c9099..ab3bb3b17522 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -254,7 +254,7 @@ check_mptcp_disabled()
 
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
-		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[ FAIL ]"
+		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[FAIL]"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
 		ret=1
 		return 1
@@ -267,7 +267,7 @@ check_mptcp_disabled()
 	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
-		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[ FAIL ]"
+		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[FAIL]"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
 		ret=1
 		return 1
@@ -294,7 +294,7 @@ do_ping()
 	ip netns exec ${connector_ns} ping ${ping_args} $connect_addr >/dev/null || rc=1
 
 	if [ $rc -ne 0 ] ; then
-		echo "$listener_ns -> $connect_addr connectivity [ FAIL ]" 1>&2
+		echo "$listener_ns -> $connect_addr connectivity [FAIL]" 1>&2
 		ret=1
 
 		return 1
@@ -427,7 +427,7 @@ do_transfer()
 	result_msg+=" # time=${duration}ms"
 	printf "(duration %05sms) " "${duration}"
 	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
-		echo "[ FAIL ] client exit code $retc, server $rets" 1>&2
+		echo "[FAIL] client exit code $retc, server $rets" 1>&2
 		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
 		ip netns exec ${listener_ns} ss -Menita 1>&2 -o "sport = :$port"
 		cat /tmp/${listener_ns}.out
@@ -469,13 +469,13 @@ do_transfer()
 	fi
 
 	if [ ${stat_synrx_now_l} -lt ${expect_synrx} ]; then
-		printf "[ FAIL ] lower MPC SYN rx (%d) than expected (%d)\n" \
+		printf "[FAIL] lower MPC SYN rx (%d) than expected (%d)\n" \
 			"${stat_synrx_now_l}" "${expect_synrx}" 1>&2
 		retc=1
 	fi
 	if [ ${stat_ackrx_now_l} -lt ${expect_ackrx} ] && [ ${stat_ooo_now} -eq 0 ]; then
 		if [ ${stat_ooo_now} -eq 0 ]; then
-			printf "[ FAIL ] lower MPC ACK rx (%d) than expected (%d)\n" \
+			printf "[FAIL] lower MPC ACK rx (%d) than expected (%d)\n" \
 				"${stat_ackrx_now_l}" "${expect_ackrx}" 1>&2
 			rets=1
 		else
@@ -491,13 +491,13 @@ do_transfer()
 
 		local csum_err_s_nr=$((csum_err_s - stat_csum_err_s))
 		if [ $csum_err_s_nr -gt 0 ]; then
-			printf "[ FAIL ]\nserver got %d data checksum error[s]" ${csum_err_s_nr}
+			printf "[FAIL]\nserver got %d data checksum error[s]" ${csum_err_s_nr}
 			rets=1
 		fi
 
 		local csum_err_c_nr=$((csum_err_c - stat_csum_err_c))
 		if [ $csum_err_c_nr -gt 0 ]; then
-			printf "[ FAIL ]\nclient got %d data checksum error[s]" ${csum_err_c_nr}
+			printf "[FAIL]\nclient got %d data checksum error[s]" ${csum_err_c_nr}
 			retc=1
 		fi
 	fi
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 1df2d24979a0..8e8977031fa0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -190,17 +190,17 @@ print_info()
 
 print_ok()
 {
-	mptcp_lib_print_ok "[ ok ]${1:+ ${*}}"
+	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
 }
 
 print_fail()
 {
-	mptcp_lib_print_err "[fail]${1:+ ${*}}"
+	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
 }
 
 print_skip()
 {
-	mptcp_lib_print_warn "[skip]${1:+ ${*}}"
+	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
 }
 
 # [ $1: fail msg ]
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 763a2989ca6d..7e309493eda2 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -292,7 +292,7 @@ mptcp_lib_check_transfer() {
 	local what="${3}"
 
 	if ! cmp "$in" "$out" > /dev/null 2>&1; then
-		echo "[ FAIL ] $what does not match (in, out):"
+		echo "[FAIL] $what does not match (in, out):"
 		mptcp_lib_print_file_err "$in"
 		mptcp_lib_print_file_err "$out"
 
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 467feb17e07b..58cfdaf1db25 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -192,7 +192,7 @@ do_transfer()
 		return 0
 	fi
 
-	echo " [ fail ]"
+	echo " [FAIL]"
 	echo "client exit code $retc, server $rets" 1>&2
 	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
 	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
-- 
2.40.1

Re: [PATCH mptcp-next v5 01/12] selftests: mptcp: capitalize ok/fail/skip

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Most scripts print uppercase [ OK ], [FAIL] and [SKIP] as test results,
> but lowercase ones are used in diag.sh, mptcp_join.sh and simult_flows.sh.
> To maintain consistency with other scripts, this patch capitalizes these
> lowercase [ ok ], [ fail ] and [ skip ]:
> 
>     [ ok ]   -> [ OK ]   	in diag.sh, mptcp_join.sh
>     [ fail ] -> [FAIL]		in diag.sh, mptcp_join.sh, simult_flows.sh
>     [ skip ] -> [SKIP]		in diag.sh, mptcp_join.sh

I think this could be squashed with patch 4/12: OK the behaviour is
changed, but that's still the same info being printed. This can be
justified with a simple "we uniform how the results are presented with
colours".

If you change that, I think it would be better to add new helpers to
print OK/FAIL/SKIP string from one place, like it is done in
mptcp_join.sh. So if needed, we can easily change how the results are
presented later instead of having to change it in different places.

We could even have these helpers in the lib:


 mptcp_lib_print_success()
 {
	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
 }

 mptcp_lib_print_fail()
 {
	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
 }

 mptcp_lib_print_skip()
 {
	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
 }

WDYT?

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

Re: [PATCH mptcp-next v5 01/12] selftests: mptcp: capitalize ok/fail/skip

[Geliang Tang]

Hi Matt,

On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Most scripts print uppercase [ OK ], [FAIL] and [SKIP] as test
> > results,
> > but lowercase ones are used in diag.sh, mptcp_join.sh and
> > simult_flows.sh.
> > To maintain consistency with other scripts, this patch capitalizes
> > these
> > lowercase [ ok ], [ fail ] and [ skip ]:
> > 
> >     [ ok ]   -> [ OK ]   	in diag.sh, mptcp_join.sh
> >     [ fail ] -> [FAIL]		in diag.sh, mptcp_join.sh,
> > simult_flows.sh
> >     [ skip ] -> [SKIP]		in diag.sh, mptcp_join.sh
> 
> I think this could be squashed with patch 4/12: OK the behaviour is
> changed, but that's still the same info being printed. This can be
> justified with a simple "we uniform how the results are presented
> with
> colours".
> 
> If you change that, I think it would be better to add new helpers to
> print OK/FAIL/SKIP string from one place, like it is done in
> mptcp_join.sh. So if needed, we can easily change how the results are
> presented later instead of having to change it in different places.
> 
> We could even have these helpers in the lib:
> 
> 
>  mptcp_lib_print_success()
>  {
> 	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
>  }
> 
>  mptcp_lib_print_fail()
>  {
> 	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
>  }
> 
>  mptcp_lib_print_skip()
>  {
> 	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
>  }
> 
> WDYT?

I agree, added mptcp_lib_pr_ok(), mptcp_lib_pr_fail() and
mptcp_lib_pr_skip() in v6.

> 
> (...)
> Cheers,
> Matt

[PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Only total test results are printed out in mptcp_sockopt.sh:

 PASS: all packets had packet mark set
 PASS: SOL_MPTCP getsockopt has expected information
 PASS: TCP_INQ cmsg/ioctl -t tcp
 PASS: TCP_INQ cmsg/ioctl -6 -t tcp
 PASS: TCP_INQ cmsg/ioctl -r tcp
 PASS: TCP_INQ cmsg/ioctl -6 -r tcp

This patch prints more info for every test result in each test
group:

 transfer ipv4                                                [ OK ]
 mark ipv4                                                    [ OK ]
 transfer ipv6                                                [ OK ]
 mark ipv6                                                    [ OK ]
 PASS: all packets had packet mark set
 sockopt v4                                                   [ OK ]
 sockopt v6                                                   [ OK ]
 PASS: SOL_MPTCP getsockopt has expected information
 TCP_INQ: -t tcp                                              [ OK ]
 PASS: TCP_INQ cmsg/ioctl -t tcp
 TCP_INQ: -6 -t tcp                                           [ OK ]
 PASS: TCP_INQ cmsg/ioctl -6 -t tcp
 TCP_INQ: -r tcp                                              [ OK ]
 PASS: TCP_INQ cmsg/ioctl -r tcp
 TCP_INQ: -6 -r tcp                                           [ OK ]
 PASS: TCP_INQ cmsg/ioctl -6 -r tcp
 TCP_INQ: -r tcp -t tcp                                       [ OK ]

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 6ed4aa32222f..f84185b5dc9f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -161,6 +161,7 @@ do_transfer()
 	wait $spid
 	local rets=$?
 
+	printf "%-50s" "transfer ${ip}"
 	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
 		echo " client exit code $retc, server $rets" 1>&2
 		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
@@ -169,12 +170,15 @@ do_transfer()
 		echo -e "\nnetns ${connector_ns} socket stat for ${port}:" 1>&2
 		ip netns exec ${connector_ns} ss -Menita 1>&2 -o "dport = :$port"
 
+		mptcp_lib_print_err "[FAIL]"
 		mptcp_lib_result_fail "transfer ${ip}"
 
 		ret=1
 		return 1
 	fi
+	mptcp_lib_print_ok "[ OK ]"
 
+	printf "%-50s" "mark ${ip}"
 	if [ $local_addr = "::" ];then
 		check_mark $listener_ns 6 || retc=1
 		check_mark $connector_ns 6 || retc=1
@@ -190,8 +194,10 @@ do_transfer()
 	mptcp_lib_result_code "${rets}" "transfer ${ip}"
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
+		mptcp_lib_print_ok "[ OK ]"
 		return 0
 	fi
+	mptcp_lib_print_err "[FAIL]"
 
 	return 1
 }
@@ -220,23 +226,27 @@ do_mptcp_sockopt_tests()
 	ip netns exec "$ns_sbox" ./mptcp_sockopt
 	lret=$?
 
+	printf "%-50s" "sockopt v4"
 	if [ $lret -ne 0 ]; then
 		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
 		mptcp_lib_result_fail "sockopt v4"
 		ret=$lret
 		return
 	fi
+	mptcp_lib_print_ok "[ OK ]"
 	mptcp_lib_result_pass "sockopt v4"
 
 	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
 	lret=$?
 
+	printf "%-50s" "sockopt v6"
 	if [ $lret -ne 0 ]; then
 		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
 		mptcp_lib_result_fail "sockopt v6"
 		ret=$lret
 		return
 	fi
+	mptcp_lib_print_ok "[ OK ]"
 	mptcp_lib_result_pass "sockopt v6"
 }
 
@@ -259,6 +269,7 @@ run_tests()
 
 do_tcpinq_test()
 {
+	printf "%-50s" "TCP_INQ: $*"
 	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
 	local lret=$?
 	if [ $lret -ne 0 ];then
@@ -267,6 +278,7 @@ do_tcpinq_test()
 		mptcp_lib_result_fail "TCP_INQ: $*"
 		return $lret
 	fi
+	mptcp_lib_print_ok "[ OK ]"
 
 	echo "PASS: TCP_INQ cmsg/ioctl $*"
 	mptcp_lib_result_pass "TCP_INQ: $*"
-- 
2.40.1

Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Only total test results are printed out in mptcp_sockopt.sh:
> 
>  PASS: all packets had packet mark set
>  PASS: SOL_MPTCP getsockopt has expected information
>  PASS: TCP_INQ cmsg/ioctl -t tcp
>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>  PASS: TCP_INQ cmsg/ioctl -r tcp
>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> 
> This patch prints more info for every test result in each test
> group:
> 
>  transfer ipv4                                                [ OK ]
>  mark ipv4                                                    [ OK ]
>  transfer ipv6                                                [ OK ]
>  mark ipv6                                                    [ OK ]
>  PASS: all packets had packet mark set
>  sockopt v4                                                   [ OK ]
>  sockopt v6                                                   [ OK ]
>  PASS: SOL_MPTCP getsockopt has expected information
>  TCP_INQ: -t tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -t tcp
>  TCP_INQ: -6 -t tcp                                           [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>  TCP_INQ: -r tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -r tcp
>  TCP_INQ: -6 -r tcp                                           [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>  TCP_INQ: -r tcp -t tcp                                       [ OK ]

Please clearly explain why this is interesting, even if it looks obvious.

Here, I don't know if this patch makes sense or not: to me, the output
is now confusing because there is a mix of '[ OK ]' and 'PASS'. Maybe:

- remove all the "PASS: xxx"? → I don't see what it brings more
- (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK ]"? But
there are fewer details)

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 6ed4aa32222f..f84185b5dc9f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -161,6 +161,7 @@ do_transfer()
>  	wait $spid
>  	local rets=$?
>  
> +	printf "%-50s" "transfer ${ip}"

Best to use a helper to avoid repeating '"%-50s"', and to be able to
change how the titles are displayed from a single place. (same below of
course)

Also, probably looking better if you use 'Transfer' instead of 'transfer'.

>  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
>  		echo " client exit code $retc, server $rets" 1>&2
>  		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
> @@ -169,12 +170,15 @@ do_transfer()
>  		echo -e "\nnetns ${connector_ns} socket stat for ${port}:" 1>&2
>  		ip netns exec ${connector_ns} ss -Menita 1>&2 -o "dport = :$port"
>  
> +		mptcp_lib_print_err "[FAIL]"
>  		mptcp_lib_result_fail "transfer ${ip}"
>  
>  		ret=1
>  		return 1
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"

(here as well, we could use 'mptcp_lib_print_success/fail/skip'
suggested in patch 1/12)

>  
> +	printf "%-50s" "mark ${ip}"

Same here: 'Mark' vs 'mark'.

>  	if [ $local_addr = "::" ];then
>  		check_mark $listener_ns 6 || retc=1
>  		check_mark $connector_ns 6 || retc=1
> @@ -190,8 +194,10 @@ do_transfer()
>  	mptcp_lib_result_code "${rets}" "transfer ${ip}"
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
> +		mptcp_lib_print_ok "[ OK ]"
>  		return 0
>  	fi
> +	mptcp_lib_print_err "[FAIL]"
>  
>  	return 1
>  }
> @@ -220,23 +226,27 @@ do_mptcp_sockopt_tests()
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt
>  	lret=$?
>  
> +	printf "%-50s" "sockopt v4"

Maybe 'SOL_MPTCP getsockopt v4'?

>  	if [ $lret -ne 0 ]; then
>  		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
>  		mptcp_lib_result_fail "sockopt v4"
>  		ret=$lret
>  		return
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "sockopt v4"
>  
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
>  	lret=$?
>  
> +	printf "%-50s" "sockopt v6"
>  	if [ $lret -ne 0 ]; then
>  		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
>  		mptcp_lib_result_fail "sockopt v6"
>  		ret=$lret
>  		return
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "sockopt v6"
>  }
>  
> @@ -259,6 +269,7 @@ run_tests()
>  
>  do_tcpinq_test()
>  {
> +	printf "%-50s" "TCP_INQ: $*"
>  	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
>  	local lret=$?
>  	if [ $lret -ne 0 ];then
> @@ -267,6 +278,7 @@ do_tcpinq_test()
>  		mptcp_lib_result_fail "TCP_INQ: $*"
>  		return $lret
>  	fi
> +	mptcp_lib_print_ok "[ OK ]"
>  
>  	echo "PASS: TCP_INQ cmsg/ioctl $*"
>  	mptcp_lib_result_pass "TCP_INQ: $*"

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

Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Geliang Tang]

On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Only total test results are printed out in mptcp_sockopt.sh:
> > 
> >  PASS: all packets had packet mark set
> >  PASS: SOL_MPTCP getsockopt has expected information
> >  PASS: TCP_INQ cmsg/ioctl -t tcp
> >  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> >  PASS: TCP_INQ cmsg/ioctl -r tcp
> >  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> > 
> > This patch prints more info for every test result in each test
> > group:
> > 
> > transfer ipv4                                                [ OK ]
> > mark ipv4                                                    [ OK ]
> > transfer ipv6                                                [ OK ]
> > mark ipv6                                                    [ OK ]
> > PASS: all packets had packet mark set
> > sockopt v4                                                   [ OK ]
> > sockopt v6                                                   [ OK ]
> > PASS: SOL_MPTCP getsockopt has expected information
> > TCP_INQ: -t tcp                                              [ OK ]
> > PASS: TCP_INQ cmsg/ioctl -t tcp
> > TCP_INQ: -6 -t tcp                                           [ OK ]
> > PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> > TCP_INQ: -r tcp                                              [ OK ]
> > PASS: TCP_INQ cmsg/ioctl -r tcp
> > TCP_INQ: -6 -r tcp                                           [ OK ]
> > PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> > TCP_INQ: -r tcp -t tcp                                       [ OK ]
> 
> Please clearly explain why this is interesting, even if it looks
> obvious.
> 
> Here, I don't know if this patch makes sense or not: to me, the
> output
> is now confusing because there is a mix of '[ OK ]' and 'PASS'.
> Maybe:
> 
> - remove all the "PASS: xxx"? → I don't see what it brings more
> - (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK ]"?
> But
> there are fewer details)

This patch uses to match the output of this script:

INFO: PASS: all packets had packet mark set
INFO: PASS: SOL_MPTCP getsockopt has expected information
INFO: PASS: TCP_INQ cmsg/ioctl -t tcp
INFO: PASS: TCP_INQ cmsg/ioctl -6 -t tcp
INFO: PASS: TCP_INQ cmsg/ioctl -r tcp
INFO: PASS: TCP_INQ cmsg/ioctl -6 -r tcp
INFO: PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp

with the test results of it:

ok 1 - mptcp_sockopt: mark ipv4
ok 2 - mptcp_sockopt: transfer ipv4
ok 3 - mptcp_sockopt: mark ipv6
ok 4 - mptcp_sockopt: transfer ipv6
ok 5 - mptcp_sockopt: sockopt v4
ok 6 - mptcp_sockopt: sockopt v6
ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp

This patch is prepared for the coming printing counter patch, otherwise
the two output results may have different serial numbers, which can be
confusing.

> 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 12
> > ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > index 6ed4aa32222f..f84185b5dc9f 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > @@ -161,6 +161,7 @@ do_transfer()
> >  	wait $spid
> >  	local rets=$?
> >  
> > +	printf "%-50s" "transfer ${ip}"
> 
> Best to use a helper to avoid repeating '"%-50s"', and to be able to
> change how the titles are displayed from a single place. (same below
> of
> course)

Add a new helper print_title() to do this.

> 
> Also, probably looking better if you use 'Transfer' instead of
> 'transfer'.

Updated in v6.

> 
> >  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
> >  		echo " client exit code $retc, server $rets" 1>&2
> >  		echo -e "\nnetns ${listener_ns} socket stat for
> > ${port}:" 1>&2
> > @@ -169,12 +170,15 @@ do_transfer()
> >  		echo -e "\nnetns ${connector_ns} socket stat for
> > ${port}:" 1>&2
> >  		ip netns exec ${connector_ns} ss -Menita 1>&2 -o
> > "dport = :$port"
> >  
> > +		mptcp_lib_print_err "[FAIL]"
> >  		mptcp_lib_result_fail "transfer ${ip}"
> >  
> >  		ret=1
> >  		return 1
> >  	fi
> > +	mptcp_lib_print_ok "[ OK ]"
> 
> (here as well, we could use 'mptcp_lib_print_success/fail/skip'
> suggested in patch 1/12)


Replaced by mptcp_lib_pr_ok.

> 
> >  
> > +	printf "%-50s" "mark ${ip}"
> 
> Same here: 'Mark' vs 'mark'.

Yes, updated.

> 
> >  	if [ $local_addr = "::" ];then
> >  		check_mark $listener_ns 6 || retc=1
> >  		check_mark $connector_ns 6 || retc=1
> > @@ -190,8 +194,10 @@ do_transfer()
> >  	mptcp_lib_result_code "${rets}" "transfer ${ip}"
> >  
> >  	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
> > +		mptcp_lib_print_ok "[ OK ]"
> >  		return 0
> >  	fi
> > +	mptcp_lib_print_err "[FAIL]"
> >  
> >  	return 1
> >  }
> > @@ -220,23 +226,27 @@ do_mptcp_sockopt_tests()
> >  	ip netns exec "$ns_sbox" ./mptcp_sockopt
> >  	lret=$?
> >  
> > +	printf "%-50s" "sockopt v4"
> 
> Maybe 'SOL_MPTCP getsockopt v4'?

Updated.

> 
> >  	if [ $lret -ne 0 ]; then
> >  		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
> >  		mptcp_lib_result_fail "sockopt v4"
> >  		ret=$lret
> >  		return
> >  	fi
> > +	mptcp_lib_print_ok "[ OK ]"
> >  	mptcp_lib_result_pass "sockopt v4"
> >  
> >  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
> >  	lret=$?
> >  
> > +	printf "%-50s" "sockopt v6"
> >  	if [ $lret -ne 0 ]; then
> >  		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
> >  		mptcp_lib_result_fail "sockopt v6"
> >  		ret=$lret
> >  		return
> >  	fi
> > +	mptcp_lib_print_ok "[ OK ]"
> >  	mptcp_lib_result_pass "sockopt v6"
> >  }
> >  
> > @@ -259,6 +269,7 @@ run_tests()
> >  
> >  do_tcpinq_test()
> >  {
> > +	printf "%-50s" "TCP_INQ: $*"
> >  	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
> >  	local lret=$?
> >  	if [ $lret -ne 0 ];then
> > @@ -267,6 +278,7 @@ do_tcpinq_test()
> >  		mptcp_lib_result_fail "TCP_INQ: $*"
> >  		return $lret
> >  	fi
> > +	mptcp_lib_print_ok "[ OK ]"
> >  
> >  	echo "PASS: TCP_INQ cmsg/ioctl $*"
> >  	mptcp_lib_result_pass "TCP_INQ: $*"
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Matthieu Baerts]

Hi Geliang,

On 28/02/2024 08:57, Geliang Tang wrote:
> On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 26/02/2024 10:43, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> Only total test results are printed out in mptcp_sockopt.sh:
>>>
>>>  PASS: all packets had packet mark set
>>>  PASS: SOL_MPTCP getsockopt has expected information
>>>  PASS: TCP_INQ cmsg/ioctl -t tcp
>>>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>>>  PASS: TCP_INQ cmsg/ioctl -r tcp
>>>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>>>
>>> This patch prints more info for every test result in each test
>>> group:
>>>
>>> transfer ipv4                                                [ OK ]
>>> mark ipv4                                                    [ OK ]
>>> transfer ipv6                                                [ OK ]
>>> mark ipv6                                                    [ OK ]
>>> PASS: all packets had packet mark set
>>> sockopt v4                                                   [ OK ]
>>> sockopt v6                                                   [ OK ]
>>> PASS: SOL_MPTCP getsockopt has expected information
>>> TCP_INQ: -t tcp                                              [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -t tcp
>>> TCP_INQ: -6 -t tcp                                           [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>>> TCP_INQ: -r tcp                                              [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -r tcp
>>> TCP_INQ: -6 -r tcp                                           [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>>> TCP_INQ: -r tcp -t tcp                                       [ OK ]
>>
>> Please clearly explain why this is interesting, even if it looks
>> obvious.
>>
>> Here, I don't know if this patch makes sense or not: to me, the
>> output
>> is now confusing because there is a mix of '[ OK ]' and 'PASS'.
>> Maybe:
>>
>> - remove all the "PASS: xxx"? → I don't see what it brings more
>> - (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK ]"?
>> But
>> there are fewer details)
> 
> This patch uses to match the output of this script:
> 
> INFO: PASS: all packets had packet mark set
> INFO: PASS: SOL_MPTCP getsockopt has expected information
> INFO: PASS: TCP_INQ cmsg/ioctl -t tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -r tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp
> 
> with the test results of it:
> 
> ok 1 - mptcp_sockopt: mark ipv4
> ok 2 - mptcp_sockopt: transfer ipv4
> ok 3 - mptcp_sockopt: mark ipv6
> ok 4 - mptcp_sockopt: transfer ipv6
> ok 5 - mptcp_sockopt: sockopt v4
> ok 6 - mptcp_sockopt: sockopt v6
> ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
> ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
> ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
> ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
> ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp
> 
> This patch is prepared for the coming printing counter patch, otherwise
> the two output results may have different serial numbers, which can be
> confusing.

Yes, I agree that it makes sense to print the individual results. But
what I wanted to say is that maybe we no longer need these "INFO: PASS:"
lines in the output?

e.g. only printing this:

# 01 Transfer ipv4                                     [ OK ]
# 02 Mark ipv4                                         [ OK ]
# 03 Transfer ipv6                                     [ OK ]
# 04 Mark ipv6                                         [ OK ]
# 05 SOL_MPTCP sockopt v4                              [ OK ]
# 06 SOL_MPTCP sockopt v6                              [ OK ]
# 07 TCP_INQ: -t tcp                                   [ OK ]
# 08 TCP_INQ: -6 -t tcp                                [ OK ]
# 09 TCP_INQ: -r tcp                                   [ OK ]
# 10 TCP_INQ: -6 -r tcp                                [ OK ]
# 11 TCP_INQ: -r tcp -t tcp                            [ OK ]

I think these 'INFO: PASS:' lines don't bring more info, no? If they do,
we can always modify what is printed above.

(While at it, it might look nicer to do: s/ipv4/IPv4/ (or s/ipv4/v4) if
we easily control that, I didn't check)


BTW, thank you for the replies on the different patches, it helps here
because there were a lot of comments on this v5.

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

Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Geliang Tang]

On Wed, 2024-02-28 at 10:13 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/02/2024 08:57, Geliang Tang wrote:
> > On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 26/02/2024 10:43, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > Only total test results are printed out in mptcp_sockopt.sh:
> > > > 
> > > >  PASS: all packets had packet mark set
> > > >  PASS: SOL_MPTCP getsockopt has expected information
> > > >  PASS: TCP_INQ cmsg/ioctl -t tcp
> > > >  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> > > >  PASS: TCP_INQ cmsg/ioctl -r tcp
> > > >  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> > > > 
> > > > This patch prints more info for every test result in each test
> > > > group:
> > > > 
> > > > transfer ipv4                                                [
> > > > OK ]
> > > > mark ipv4                                                    [
> > > > OK ]
> > > > transfer ipv6                                                [
> > > > OK ]
> > > > mark ipv6                                                    [
> > > > OK ]
> > > > PASS: all packets had packet mark set
> > > > sockopt v4                                                   [
> > > > OK ]
> > > > sockopt v6                                                   [
> > > > OK ]
> > > > PASS: SOL_MPTCP getsockopt has expected information
> > > > TCP_INQ: -t tcp                                              [
> > > > OK ]
> > > > PASS: TCP_INQ cmsg/ioctl -t tcp
> > > > TCP_INQ: -6 -t tcp                                           [
> > > > OK ]
> > > > PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> > > > TCP_INQ: -r tcp                                              [
> > > > OK ]
> > > > PASS: TCP_INQ cmsg/ioctl -r tcp
> > > > TCP_INQ: -6 -r tcp                                           [
> > > > OK ]
> > > > PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> > > > TCP_INQ: -r tcp -t tcp                                       [
> > > > OK ]
> > > 
> > > Please clearly explain why this is interesting, even if it looks
> > > obvious.
> > > 
> > > Here, I don't know if this patch makes sense or not: to me, the
> > > output
> > > is now confusing because there is a mix of '[ OK ]' and 'PASS'.
> > > Maybe:
> > > 
> > > - remove all the "PASS: xxx"? → I don't see what it brings more
> > > - (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK
> > > ]"?
> > > But
> > > there are fewer details)
> > 
> > This patch uses to match the output of this script:
> > 
> > INFO: PASS: all packets had packet mark set
> > INFO: PASS: SOL_MPTCP getsockopt has expected information
> > INFO: PASS: TCP_INQ cmsg/ioctl -t tcp
> > INFO: PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> > INFO: PASS: TCP_INQ cmsg/ioctl -r tcp
> > INFO: PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> > INFO: PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp
> > 
> > with the test results of it:
> > 
> > ok 1 - mptcp_sockopt: mark ipv4
> > ok 2 - mptcp_sockopt: transfer ipv4
> > ok 3 - mptcp_sockopt: mark ipv6
> > ok 4 - mptcp_sockopt: transfer ipv6
> > ok 5 - mptcp_sockopt: sockopt v4
> > ok 6 - mptcp_sockopt: sockopt v6
> > ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
> > ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
> > ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
> > ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
> > ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp
> > 
> > This patch is prepared for the coming printing counter patch,
> > otherwise
> > the two output results may have different serial numbers, which can
> > be
> > confusing.
> 
> Yes, I agree that it makes sense to print the individual results. But
> what I wanted to say is that maybe we no longer need these "INFO:
> PASS:"
> lines in the output?
> 
> e.g. only printing this:
> 
> # 01 Transfer ipv4                                     [ OK ]
> # 02 Mark ipv4                                         [ OK ]
> # 03 Transfer ipv6                                     [ OK ]
> # 04 Mark ipv6                                         [ OK ]
> # 05 SOL_MPTCP sockopt v4                              [ OK ]
> # 06 SOL_MPTCP sockopt v6                              [ OK ]
> # 07 TCP_INQ: -t tcp                                   [ OK ]
> # 08 TCP_INQ: -6 -t tcp                                [ OK ]
> # 09 TCP_INQ: -r tcp                                   [ OK ]
> # 10 TCP_INQ: -6 -r tcp                                [ OK ]
> # 11 TCP_INQ: -r tcp -t tcp                            [ OK ]

Yes, this is much better. Will update it in next version.

> 
> I think these 'INFO: PASS:' lines don't bring more info, no? If they
> do,
> we can always modify what is printed above.
> 
> (While at it, it might look nicer to do: s/ipv4/IPv4/ (or s/ipv4/v4)
> if
> we easily control that, I didn't check)

's/ipv4/v4' is better, will update them.

Thanks,
-Geliang

> 
> 
> BTW, thank you for the replies on the different patches, it helps
> here
> because there were a lot of comments on this v5.
> 
> Cheers,
> Matt

[PATCH mptcp-next v5 03/12] selftests: mptcp: connect: fix misaligned OK/FAIL

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The first [ OK ] and [FAIL] in the output of mptcp_connect.sh misalign
with the others:

New MPTCP socket can be blocked via sysctl		[ OK ]
INFO: validating network environment with pings
INFO: Using loss of 0.85% delay 16 ms reorder 95% 70% with delay 4ms on
ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration   184ms) [ OK ]
ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    50ms) [ OK ]
ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP	(duration    55ms) [ OK ]

This patch fixes them.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index ab3bb3b17522..72be905bab1f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -254,7 +254,7 @@ check_mptcp_disabled()
 
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
-		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[FAIL]"
+		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
 		ret=1
 		return 1
@@ -267,13 +267,13 @@ check_mptcp_disabled()
 	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
-		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[FAIL]"
+		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
 		ret=1
 		return 1
 	fi
 
-	echo -e "New MPTCP socket can be blocked via sysctl\t\t[ OK ]"
+	echo -e "New MPTCP socket can be blocked via sysctl\t\t\t   [ OK ]"
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
 }
-- 
2.40.1

Re: [PATCH mptcp-next v5 03/12] selftests: mptcp: connect: fix misaligned OK/FAIL

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The first [ OK ] and [FAIL] in the output of mptcp_connect.sh misalign
> with the others:
> 
> New MPTCP socket can be blocked via sysctl		[ OK ]
> INFO: validating network environment with pings
> INFO: Using loss of 0.85% delay 16 ms reorder 95% 70% with delay 4ms on
> ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration   184ms) [ OK ]
> ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    50ms) [ OK ]
> ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP	(duration    55ms) [ OK ]
> 
> This patch fixes them.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index ab3bb3b17522..72be905bab1f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -254,7 +254,7 @@ check_mptcp_disabled()
>  
>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> -		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[FAIL]"
> +		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t\t   [FAIL]"

Best to use one new helper using 'printf %-XXXs', instead of playing
with tabs and spaces.

That will also help to add colours later, with just:

  mptcp_lib_print_success/fail/warn

>  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
>  		ret=1
>  		return 1
> @@ -267,13 +267,13 @@ check_mptcp_disabled()
>  	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
> -		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[FAIL]"
> +		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
>  		ret=1
>  		return 1
>  	fi
>  
> -	echo -e "New MPTCP socket can be blocked via sysctl\t\t[ OK ]"
> +	echo -e "New MPTCP socket can be blocked via sysctl\t\t\t   [ OK ]"
>  	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
>  	return 0
>  }

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

Re: [PATCH mptcp-next v5 03/12] selftests: mptcp: connect: fix misaligned OK/FAIL

[Geliang Tang]

On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The first [ OK ] and [FAIL] in the output of mptcp_connect.sh
> > misalign
> > with the others:
> > 
> > New MPTCP socket can be blocked via sysctl		[ OK ]
> > INFO: validating network environment with pings
> > INFO: Using loss of 0.85% delay 16 ms reorder 95% 70% with delay
> > 4ms on
> > ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration   184ms)
> > [ OK ]
> > ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    50ms)
> > [ OK ]
> > ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP	(duration    55ms)
> > [ OK ]
> > 
> > This patch fixes them.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index ab3bb3b17522..72be905bab1f 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -254,7 +254,7 @@ check_mptcp_disabled()
> >  
> >  	# net.mptcp.enabled should be enabled by default
> >  	if [ "$(ip netns exec ${disabled_ns} sysctl
> > net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> > -		echo -e "net.mptcp.enabled sysctl is not 1 by
> > default\t\t[FAIL]"
> > +		echo -e "net.mptcp.enabled sysctl is not 1 by
> > default\t\t\t   [FAIL]"
> 
> Best to use one new helper using 'printf %-XXXs', instead of playing
> with tabs and spaces.

Updated.

> 
> That will also help to add colours later, with just:
> 
>   mptcp_lib_print_success/fail/warn

Updated.

> 
> >  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is
> > not 1 by default"
> >  		ret=1
> >  		return 1
> > @@ -267,13 +267,13 @@ check_mptcp_disabled()
> >  	mptcp_lib_ns_exit "${disabled_ns}"
> >  
> >  	if [ ${err} -eq 0 ]; then
> > -		echo -e "New MPTCP socket cannot be blocked via
> > sysctl\t\t[FAIL]"
> > +		echo -e "New MPTCP socket cannot be blocked via
> > sysctl\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "New MPTCP socket cannot be
> > blocked via sysctl"
> >  		ret=1
> >  		return 1
> >  	fi
> >  
> > -	echo -e "New MPTCP socket can be blocked via sysctl\t\t[
> > OK ]"
> > +	echo -e "New MPTCP socket can be blocked via
> > sysctl\t\t\t   [ OK ]"
> >  	mptcp_lib_result_pass "New MPTCP socket can be blocked via
> > sysctl"
> >  	return 0
> >  }
> 
> Cheers,
> Matt

[PATCH mptcp-next v5 04/12] selftests: mptcp: print test results with colors

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper mptcp_lib_verify_listener_events() will be added latter in
mptcp_lib.sh, and be used by mptcp_join.sh and userspace_pm.sh. The
former prints colored output while the latter is not. It makes sense
to unify them.

This patch uses mptcp_lib_print_ok(), _warn(), _err() and _info() helpers
in scripts diag.sh, mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh,
simult_flows.sh and userspace_pm.sh to print test results with colors.

Having colors helps to quickly identify issues when looking at a long
list of output logs and results.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh     | 12 ++++----
 .../selftests/net/mptcp/mptcp_connect.sh      | 28 ++++++++++---------
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 16 +++++------
 .../testing/selftests/net/mptcp/pm_netlink.sh |  3 +-
 .../selftests/net/mptcp/simult_flows.sh       |  4 +--
 .../selftests/net/mptcp/userspace_pm.sh       | 13 +++------
 6 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index aa3fb808fc6d..f9f62a8f41e3 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -58,15 +58,15 @@ __chk_nr()
 	printf "%-50s" "$msg"
 	if [ "$nr" != "$expected" ]; then
 		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
-			echo "[SKIP] Feature probably not supported"
+			mptcp_lib_print_warn "[SKIP] Feature probably not supported"
 			mptcp_lib_result_skip "${msg}"
 		else
-			echo "[FAIL] expected $expected found $nr"
+			mptcp_lib_print_err "[FAIL] expected $expected found $nr"
 			mptcp_lib_result_fail "${msg}"
 			ret=$test_cnt
 		fi
 	else
-		echo "[ OK ]"
+		mptcp_lib_print_ok "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
 	test_cnt=$((test_cnt+1))
@@ -116,15 +116,15 @@ wait_msk_nr()
 
 	printf "%-50s" "$msg"
 	if [ $i -ge $timeout ]; then
-		echo "[FAIL] timeout while expecting $expected max $max last $nr"
+		mptcp_lib_print_err "[FAIL] timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
 		ret=$test_cnt
 	elif [ $nr != $expected ]; then
-		echo "[FAIL] expected $expected found $nr"
+		mptcp_lib_print_err "[FAIL] expected $expected found $nr"
 		mptcp_lib_result_fail "${msg} # unexpected result"
 		ret=$test_cnt
 	else
-		echo "[ OK ]"
+		mptcp_lib_print_ok "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
 	test_cnt=$((test_cnt+1))
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 72be905bab1f..c7483902026b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -254,7 +254,8 @@ check_mptcp_disabled()
 
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
-		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t\t   [FAIL]"
+		echo -n -e "net.mptcp.enabled sysctl is not 1 by default"
+		mptcp_lib_print_err "\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
 		ret=1
 		return 1
@@ -267,13 +268,15 @@ check_mptcp_disabled()
 	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
-		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t\t   [FAIL]"
+		echo -n -e "New MPTCP socket cannot be blocked via sysctl"
+		mptcp_lib_print_err "\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
 		ret=1
 		return 1
 	fi
 
-	echo -e "New MPTCP socket can be blocked via sysctl\t\t\t   [ OK ]"
+	echo -n -e "New MPTCP socket can be blocked via sysctl"
+	mptcp_lib_print_ok "\t\t\t   [ OK ]"
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
 }
@@ -503,7 +506,7 @@ do_transfer()
 	fi
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
-		printf "[ OK ]"
+		mptcp_lib_print_ok "[ OK ]"
 		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
 	else
 		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
@@ -534,7 +537,6 @@ do_transfer()
 			"${expect_ackrx}" "${stat_ackrx_now_l}"
 	fi
 
-	echo
 	cat "$capout"
 	[ $retc -eq 0 ] && [ $rets -eq 0 ]
 }
@@ -708,7 +710,7 @@ EOF
 		return
 	fi
 
-	echo "INFO: test $msg"
+	mptcp_lib_print_info "INFO: test $msg"
 
 	TEST_COUNT=10000
 	local extra_args="-o TRANSPARENT"
@@ -721,12 +723,12 @@ EOF
 	ip -net "$listener_ns" route del local $local_addr/0 dev lo table 100
 
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: $msg, mptcp connection error" 1>&2
+		mptcp_lib_print_err "FAIL: $msg, mptcp connection error" 1>&2
 		ret=$lret
 		return 1
 	fi
 
-	echo "PASS: $msg"
+	mptcp_lib_print_ok "PASS: $msg"
 	return 0
 }
 
@@ -735,7 +737,7 @@ run_tests_peekmode()
 	local peekmode="$1"
 
 	TEST_GROUP="peek mode: ${peekmode}"
-	echo "INFO: with peek mode: ${peekmode}"
+	mptcp_lib_print_info "INFO: with peek mode: ${peekmode}"
 	run_tests_lo "$ns1" "$ns1" 10.0.1.1 1 "-P ${peekmode}"
 	run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1 "-P ${peekmode}"
 }
@@ -750,7 +752,7 @@ run_tests_mptfo()
 		return
 	fi
 
-	echo "INFO: with MPTFO start"
+	mptcp_lib_print_info "INFO: with MPTFO start"
 	ip netns exec "$ns1" sysctl -q net.ipv4.tcp_fastopen=2
 	ip netns exec "$ns2" sysctl -q net.ipv4.tcp_fastopen=1
 
@@ -762,7 +764,7 @@ run_tests_mptfo()
 
 	ip netns exec "$ns1" sysctl -q net.ipv4.tcp_fastopen=0
 	ip netns exec "$ns2" sysctl -q net.ipv4.tcp_fastopen=0
-	echo "INFO: with MPTFO end"
+	mptcp_lib_print_info "INFO: with MPTFO end"
 }
 
 run_tests_disconnect()
@@ -786,7 +788,7 @@ run_tests_disconnect()
 	cin_disconnect="$old_cin"
 	connect_per_transfer=3
 
-	echo "INFO: disconnect"
+	mptcp_lib_print_info "INFO: disconnect"
 	run_tests_lo "$ns1" "$ns1" 10.0.1.1 1 "-I 3 -i $old_cin"
 	run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1 "-I 3 -i $old_cin"
 
@@ -835,7 +837,7 @@ check_mptcp_disabled
 
 stop_if_error "The kernel configuration is not valid for MPTCP"
 
-echo "INFO: validating network environment with pings"
+mptcp_lib_print_info "INFO: validating network environment with pings"
 for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
 	do_ping "$ns1" $sender 10.0.1.1
 	do_ping "$ns1" $sender dead:beef:1::1
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index f84185b5dc9f..cfa0cfb918f4 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -218,7 +218,7 @@ do_mptcp_sockopt_tests()
 	local lret=0
 
 	if ! mptcp_lib_kallsyms_has "mptcp_diag_fill_info$"; then
-		echo "INFO: MPTCP sockopt not supported: SKIP"
+		mptcp_lib_print_warn "INFO: MPTCP sockopt not supported: SKIP"
 		mptcp_lib_result_skip "sockopt"
 		return
 	fi
@@ -228,7 +228,7 @@ do_mptcp_sockopt_tests()
 
 	printf "%-50s" "sockopt v4"
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
+		mptcp_lib_print_err "FAIL: SOL_MPTCP getsockopt"
 		mptcp_lib_result_fail "sockopt v4"
 		ret=$lret
 		return
@@ -241,7 +241,7 @@ do_mptcp_sockopt_tests()
 
 	printf "%-50s" "sockopt v6"
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
+		mptcp_lib_print_err "FAIL: SOL_MPTCP getsockopt (ipv6)"
 		mptcp_lib_result_fail "sockopt v6"
 		ret=$lret
 		return
@@ -274,13 +274,13 @@ do_tcpinq_test()
 	local lret=$?
 	if [ $lret -ne 0 ];then
 		ret=$lret
-		echo "FAIL: mptcp_inq $*" 1>&2
+		mptcp_lib_print_err "FAIL: mptcp_inq $*" 1>&2
 		mptcp_lib_result_fail "TCP_INQ: $*"
 		return $lret
 	fi
 	mptcp_lib_print_ok "[ OK ]"
 
-	echo "PASS: TCP_INQ cmsg/ioctl $*"
+	mptcp_lib_print_info "PASS: TCP_INQ cmsg/ioctl $*"
 	mptcp_lib_result_pass "TCP_INQ: $*"
 	return $lret
 }
@@ -290,7 +290,7 @@ do_tcpinq_tests()
 	local lret=0
 
 	if ! mptcp_lib_kallsyms_has "mptcp_ioctl$"; then
-		echo "INFO: TCP_INQ not supported: SKIP"
+		mptcp_lib_print_warn "INFO: TCP_INQ not supported: SKIP"
 		mptcp_lib_result_skip "TCP_INQ"
 		return
 	fi
@@ -327,12 +327,12 @@ run_tests $ns1 $ns2 10.0.1.1
 run_tests $ns1 $ns2 dead:beef:1::1
 
 if [ $ret -eq 0 ];then
-	echo "PASS: all packets had packet mark set"
+	mptcp_lib_print_info "PASS: all packets had packet mark set"
 fi
 
 do_mptcp_sockopt_tests
 if [ $ret -eq 0 ];then
-	echo "PASS: SOL_MPTCP getsockopt has expected information"
+	mptcp_lib_print_info "PASS: SOL_MPTCP getsockopt has expected information"
 fi
 
 do_tcpinq_tests
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 427fc5c70b3c..68fad278ac59 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -189,7 +189,8 @@ subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
 else
 	for st in fullmesh nofullmesh backup,fullmesh; do
 		st="          (${st})"
-		printf "%-50s%s\n" "${st}" "[SKIP]"
+		printf "%-50s" "${st}"
+		mptcp_lib_print_warn "[SKIP]"
 		mptcp_lib_result_skip "${st}"
 	done
 fi
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 58cfdaf1db25..79cb377ee0bd 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -187,12 +187,12 @@ do_transfer()
 	printf "%-16s" " max $max_time "
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
 	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
-		echo "[ OK ]"
+		mptcp_lib_print_ok "[ OK ]"
 		cat "$capout"
 		return 0
 	fi
 
-	echo " [FAIL]"
+	mptcp_lib_print_err " [FAIL]"
 	echo "client exit code $retc, server $rets" 1>&2
 	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
 	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index b0cce8f065d8..33bbb0d5807f 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -61,7 +61,7 @@ _printf() {
 
 print_title()
 {
-	_printf "INFO: %s\n" "${1}"
+	mptcp_lib_print_info "INFO: ${1}"
 }
 
 # $1: test name
@@ -72,27 +72,22 @@ print_test()
 	_printf "%-68s" "${test_name}"
 }
 
-print_results()
-{
-	_printf "[%s]\n" "${1}"
-}
-
 test_pass()
 {
-	print_results " OK "
+	mptcp_lib_print_ok "[ OK ]"
 	mptcp_lib_result_pass "${test_name}"
 }
 
 test_skip()
 {
-	print_results "SKIP"
+	mptcp_lib_print_warn "[SKIP]"
 	mptcp_lib_result_skip "${test_name}"
 }
 
 # $1: msg
 test_fail()
 {
-	print_results "FAIL"
+	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
 	ret=1
 
 	if [ -n "${1}" ]; then
-- 
2.40.1

Re: [PATCH mptcp-next v5 04/12] selftests: mptcp: print test results with colors

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_lib_verify_listener_events() will be added latter in
> mptcp_lib.sh, and be used by mptcp_join.sh and userspace_pm.sh. The
> former prints colored output while the latter is not. It makes sense
> to unify them.

(not sure if we need this explanation about
mptcp_lib_verify_listener_events)
> 
> This patch uses mptcp_lib_print_ok(), _warn(), _err() and _info() helpers
> in scripts diag.sh, mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh,
> simult_flows.sh and userspace_pm.sh to print test results with colors.
> 
> Having colors helps to quickly identify issues when looking at a long
> list of output logs and results.

I think this patch will look better if mptcp_lib_print_success/fail/skip
helpers are used.

In this case, you should not need to use mptcp_lib_print_ok/err/warn,
only _info, right?

If you still need to use '_ok/err/warn' in the new version, please
justify it. But I think these should be "internal" functions, or for
specific need.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 72be905bab1f..c7483902026b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -254,7 +254,8 @@ check_mptcp_disabled()
>  
>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> -		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t\t   [FAIL]"
> +		echo -n -e "net.mptcp.enabled sysctl is not 1 by default"

See the comments on previous patches, would be better with a helper to
print ↑ without 'echo -n -e', and ↓, we would just have
'mptcp_lib_print_fail'.

Same below

> +		mptcp_lib_print_err "\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
>  		ret=1
>  		return 1
> @@ -267,13 +268,15 @@ check_mptcp_disabled()
>  	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
> -		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t\t   [FAIL]"
> +		echo -n -e "New MPTCP socket cannot be blocked via sysctl"
> +		mptcp_lib_print_err "\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
>  		ret=1
>  		return 1
>  	fi
>  
> -	echo -e "New MPTCP socket can be blocked via sysctl\t\t\t   [ OK ]"
> +	echo -n -e "New MPTCP socket can be blocked via sysctl"
> +	mptcp_lib_print_ok "\t\t\t   [ OK ]"
>  	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
>  	return 0
>  }
> @@ -503,7 +506,7 @@ do_transfer()
>  	fi
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
> -		printf "[ OK ]"
> +		mptcp_lib_print_ok "[ OK ]"
>  		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
>  	else

Maybe good to add a comment here, e.g.

  # "FAIL" message has been printed before.

>  		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
> @@ -534,7 +537,6 @@ do_transfer()
>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>  	fi
>  
> -	echo
>  	cat "$capout"
>  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
>  }
> @@ -708,7 +710,7 @@ EOF
>  		return
>  	fi
>  
> -	echo "INFO: test $msg"
> +	mptcp_lib_print_info "INFO: test $msg"
>  
>  	TEST_COUNT=10000
>  	local extra_args="-o TRANSPARENT"
> @@ -721,12 +723,12 @@ EOF
>  	ip -net "$listener_ns" route del local $local_addr/0 dev lo table 100
>  
>  	if [ $lret -ne 0 ]; then
> -		echo "FAIL: $msg, mptcp connection error" 1>&2
> +		mptcp_lib_print_err "FAIL: $msg, mptcp connection error" 1>&2

(I guess it is fine here to print this to stdout, like all the others,
that's part of the unification I think -- but as mentioned in patch
2/12, we can probably remove this FAIL message, and the PASS one here as
well)

>  		ret=$lret
>  		return 1
>  	fi
>  
> -	echo "PASS: $msg"
> +	mptcp_lib_print_ok "PASS: $msg"
>  	return 0
>  }
>  

(...)

> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 58cfdaf1db25..79cb377ee0bd 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -187,12 +187,12 @@ do_transfer()
>  	printf "%-16s" " max $max_time "
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
>  	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
> -		echo "[ OK ]"
> +		mptcp_lib_print_ok "[ OK ]"
>  		cat "$capout"
>  		return 0
>  	fi
>  
> -	echo " [FAIL]"
> +	mptcp_lib_print_err " [FAIL]"

I guess we don't need the extra space before '[FAIL]', right?

>  	echo "client exit code $retc, server $rets" 1>&2
>  	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
>  	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index b0cce8f065d8..33bbb0d5807f 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -61,7 +61,7 @@ _printf() {
>  
>  print_title()
>  {
> -	_printf "INFO: %s\n" "${1}"
> +	mptcp_lib_print_info "INFO: ${1}"
>  }
>  
>  # $1: test name
> @@ -72,27 +72,22 @@ print_test()
>  	_printf "%-68s" "${test_name}"
>  }
>  
> -print_results()
> -{
> -	_printf "[%s]\n" "${1}"
> -}
> -
>  test_pass()
>  {
> -	print_results " OK "
> +	mptcp_lib_print_ok "[ OK ]"
>  	mptcp_lib_result_pass "${test_name}"
>  }
>  
>  test_skip()
>  {
> -	print_results "SKIP"
> +	mptcp_lib_print_warn "[SKIP]"
>  	mptcp_lib_result_skip "${test_name}"
>  }
>  
>  # $1: msg
>  test_fail()
>  {
> -	print_results "FAIL"
> +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"

The error will be repeated twice then, no? ...

>  	ret=1
>  
>  	if [ -n "${1}" ]; then

... maybe you can remove this block?

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

Re: [PATCH mptcp-next v5 04/12] selftests: mptcp: print test results with colors

[Geliang Tang]

On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The helper mptcp_lib_verify_listener_events() will be added latter
> > in
> > mptcp_lib.sh, and be used by mptcp_join.sh and userspace_pm.sh. The
> > former prints colored output while the latter is not. It makes
> > sense
> > to unify them.
> 
> (not sure if we need this explanation about
> mptcp_lib_verify_listener_events)

Dropped.

> > 
> > This patch uses mptcp_lib_print_ok(), _warn(), _err() and _info()
> > helpers
> > in scripts diag.sh, mptcp_connect.sh, mptcp_sockopt.sh,
> > pm_netlink.sh,
> > simult_flows.sh and userspace_pm.sh to print test results with
> > colors.
> > 
> > Having colors helps to quickly identify issues when looking at a
> > long
> > list of output logs and results.
> 
> I think this patch will look better if
> mptcp_lib_print_success/fail/skip
> helpers are used.
> 
> In this case, you should not need to use mptcp_lib_print_ok/err/warn,
> only _info, right?
> 
> If you still need to use '_ok/err/warn' in the new version, please
> justify it. But I think these should be "internal" functions, or for
> specific need.

Updated.

> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 72be905bab1f..c7483902026b 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -254,7 +254,8 @@ check_mptcp_disabled()
> >  
> >  	# net.mptcp.enabled should be enabled by default
> >  	if [ "$(ip netns exec ${disabled_ns} sysctl
> > net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> > -		echo -e "net.mptcp.enabled sysctl is not 1 by
> > default\t\t\t   [FAIL]"
> > +		echo -n -e "net.mptcp.enabled sysctl is not 1 by
> > default"
> 
> See the comments on previous patches, would be better with a helper
> to
> print ↑ without 'echo -n -e', and ↓, we would just have
> 'mptcp_lib_print_fail'.

Add a helper print_title.

> 
> Same below
> 
> > +		mptcp_lib_print_err "\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is
> > not 1 by default"
> >  		ret=1
> >  		return 1
> > @@ -267,13 +268,15 @@ check_mptcp_disabled()
> >  	mptcp_lib_ns_exit "${disabled_ns}"
> >  
> >  	if [ ${err} -eq 0 ]; then
> > -		echo -e "New MPTCP socket cannot be blocked via
> > sysctl\t\t\t   [FAIL]"
> > +		echo -n -e "New MPTCP socket cannot be blocked via
> > sysctl"
> > +		mptcp_lib_print_err "\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "New MPTCP socket cannot be
> > blocked via sysctl"
> >  		ret=1
> >  		return 1
> >  	fi
> >  
> > -	echo -e "New MPTCP socket can be blocked via
> > sysctl\t\t\t   [ OK ]"
> > +	echo -n -e "New MPTCP socket can be blocked via sysctl"
> > +	mptcp_lib_print_ok "\t\t\t   [ OK ]"
> >  	mptcp_lib_result_pass "New MPTCP socket can be blocked via
> > sysctl"
> >  	return 0
> >  }
> > @@ -503,7 +506,7 @@ do_transfer()
> >  	fi
> >  
> >  	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
> > -		printf "[ OK ]"
> > +		mptcp_lib_print_ok "[ OK ]"
> >  		mptcp_lib_result_pass "${TEST_GROUP}:
> > ${result_msg}"
> >  	else
> 
> Maybe good to add a comment here, e.g.
> 
>   # "FAIL" message has been printed before.
> 
> >  		mptcp_lib_result_fail "${TEST_GROUP}:
> > ${result_msg}"
> > @@ -534,7 +537,6 @@ do_transfer()
> >  			"${expect_ackrx}" "${stat_ackrx_now_l}"
> >  	fi
> >  
> > -	echo
> >  	cat "$capout"
> >  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
> >  }
> > @@ -708,7 +710,7 @@ EOF
> >  		return
> >  	fi
> >  
> > -	echo "INFO: test $msg"
> > +	mptcp_lib_print_info "INFO: test $msg"
> >  
> >  	TEST_COUNT=10000
> >  	local extra_args="-o TRANSPARENT"
> > @@ -721,12 +723,12 @@ EOF
> >  	ip -net "$listener_ns" route del local $local_addr/0 dev
> > lo table 100
> >  
> >  	if [ $lret -ne 0 ]; then
> > -		echo "FAIL: $msg, mptcp connection error" 1>&2
> > +		mptcp_lib_print_err "FAIL: $msg, mptcp connection
> > error" 1>&2
> 
> (I guess it is fine here to print this to stdout, like all the
> others,
> that's part of the unification I think -- but as mentioned in patch
> 2/12, we can probably remove this FAIL message, and the PASS one here
> as
> well)

"FAIL:" is removed, keep '1>&2' unchanged.

> 
> >  		ret=$lret
> >  		return 1
> >  	fi
> >  
> > -	echo "PASS: $msg"
> > +	mptcp_lib_print_ok "PASS: $msg"
> >  	return 0
> >  }
> >  
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > index 58cfdaf1db25..79cb377ee0bd 100755
> > --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > @@ -187,12 +187,12 @@ do_transfer()
> >  	printf "%-16s" " max $max_time "
> >  	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
> >  	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
> > -		echo "[ OK ]"
> > +		mptcp_lib_print_ok "[ OK ]"
> >  		cat "$capout"
> >  		return 0
> >  	fi
> >  
> > -	echo " [FAIL]"
> > +	mptcp_lib_print_err " [FAIL]"
> 
> I guess we don't need the extra space before '[FAIL]', right?

Yes. Replaced by mptcp_lib_pr_fail.

> 
> >  	echo "client exit code $retc, server $rets" 1>&2
> >  	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
> >  	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
> > diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > index b0cce8f065d8..33bbb0d5807f 100755
> > --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > @@ -61,7 +61,7 @@ _printf() {
> >  
> >  print_title()
> >  {
> > -	_printf "INFO: %s\n" "${1}"
> > +	mptcp_lib_print_info "INFO: ${1}"
> >  }
> >  
> >  # $1: test name
> > @@ -72,27 +72,22 @@ print_test()
> >  	_printf "%-68s" "${test_name}"
> >  }
> >  
> > -print_results()
> > -{
> > -	_printf "[%s]\n" "${1}"
> > -}
> > -
> >  test_pass()
> >  {
> > -	print_results " OK "
> > +	mptcp_lib_print_ok "[ OK ]"
> >  	mptcp_lib_result_pass "${test_name}"
> >  }
> >  
> >  test_skip()
> >  {
> > -	print_results "SKIP"
> > +	mptcp_lib_print_warn "[SKIP]"
> >  	mptcp_lib_result_skip "${test_name}"
> >  }
> >  
> >  # $1: msg
> >  test_fail()
> >  {
> > -	print_results "FAIL"
> > +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
> 
> The error will be repeated twice then, no? ...

"[FAIL]" is dropped here in v6.

> 
> >  	ret=1
> >  
> >  	if [ -n "${1}" ]; then
> 
> ... maybe you can remove this block?
> 
> Cheers,
> Matt

[PATCH mptcp-next v5 05/12] selftests: mptcp: connect: add dedicated port counter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
increase port numbers in mptcp_connect.sh.

This can avoid outputting discontinuous test counters.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index c7483902026b..68073b255983 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -33,6 +33,7 @@ do_tcp=0
 checksum=false
 filesize=0
 connect_per_transfer=1
+PORT=0
 
 if [ $tc_loss -eq 100 ];then
 	tc_loss=1%
@@ -317,7 +318,7 @@ do_transfer()
 	local extra_args="$7"
 
 	local port
-	port=$((10000+TEST_COUNT))
+	port=$((10000+PORT++))
 	TEST_COUNT=$((TEST_COUNT+1))
 
 	if [ "$rcvbuf" -gt 0 ]; then
@@ -712,7 +713,7 @@ EOF
 
 	mptcp_lib_print_info "INFO: test $msg"
 
-	TEST_COUNT=10000
+	PORT=10000
 	local extra_args="-o TRANSPARENT"
 	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
 		    ${connect_addr} ${local_addr} "${extra_args}"
-- 
2.40.1

Re: [PATCH mptcp-next v5 05/12] selftests: mptcp: connect: add dedicated port counter

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
> increase port numbers in mptcp_connect.sh.
> 
> This can avoid outputting discontinuous test counters.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index c7483902026b..68073b255983 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -33,6 +33,7 @@ do_tcp=0
>  checksum=false
>  filesize=0
>  connect_per_transfer=1
> +PORT=0

Can you not initialise it her at '10000'...

>  
>  if [ $tc_loss -eq 100 ];then
>  	tc_loss=1%
> @@ -317,7 +318,7 @@ do_transfer()
>  	local extra_args="$7"
>  
>  	local port
> -	port=$((10000+TEST_COUNT))
> +	port=$((10000+PORT++))

... and here, have:

  port=$((PORT + TEST_COUNT))

(or PORT++ if TEST_COUNT is removed later)

>  	TEST_COUNT=$((TEST_COUNT+1))
>  
>  	if [ "$rcvbuf" -gt 0 ]; then
> @@ -712,7 +713,7 @@ EOF
>  
>  	mptcp_lib_print_info "INFO: test $msg"
>  
> -	TEST_COUNT=10000
> +	PORT=10000

So here you don't need to override the value, no?

>  	local extra_args="-o TRANSPARENT"
>  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
>  		    ${connect_addr} ${local_addr} "${extra_args}"

While at it, maybe use ${PORT} in check_mptcp_disabled()?

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

Re: [PATCH mptcp-next v5 05/12] selftests: mptcp: connect: add dedicated port counter

[Geliang Tang]

On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds a new dedicated counter 'PORT' instead of
> > TEST_COUNT to
> > increase port numbers in mptcp_connect.sh.
> > 
> > This can avoid outputting discontinuous test counters.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index c7483902026b..68073b255983 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -33,6 +33,7 @@ do_tcp=0
> >  checksum=false
> >  filesize=0
> >  connect_per_transfer=1
> > +PORT=0
> 
> Can you not initialise it her at '10000'...
> 
> >  
> >  if [ $tc_loss -eq 100 ];then
> >  	tc_loss=1%
> > @@ -317,7 +318,7 @@ do_transfer()
> >  	local extra_args="$7"
> >  
> >  	local port
> > -	port=$((10000+TEST_COUNT))
> > +	port=$((10000+PORT++))
> 
> ... and here, have:
> 
>   port=$((PORT + TEST_COUNT))
> 
> (or PORT++ if TEST_COUNT is removed later)
> 
> >  	TEST_COUNT=$((TEST_COUNT+1))
> >  
> >  	if [ "$rcvbuf" -gt 0 ]; then
> > @@ -712,7 +713,7 @@ EOF
> >  
> >  	mptcp_lib_print_info "INFO: test $msg"
> >  
> > -	TEST_COUNT=10000
> > +	PORT=10000
> 
> So here you don't need to override the value, no?

This doesn't work, the value of port is changed. Keep this patch
unchanged.

> 
> >  	local extra_args="-o TRANSPARENT"
> >  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
> >  		    ${connect_addr} ${local_addr} "${extra_args}"
> 
> While at it, maybe use ${PORT} in check_mptcp_disabled()?
> 
> Cheers,
> Matt

[PATCH mptcp-next v5 06/12] selftests: mptcp: connect: print out ping tests info

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

'ping tests' shows in the test results of mptcp_connect.sh:

1..68
ok 1 - mptcp_connect: New MPTCP socket can be blocked via sysctl
ok 2 - mptcp_connect: ping tests
ok 3 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1 (10.0.1.1:10000) MPTCP
ok 4 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1 (10.0.1.1:10001) TCP

But not in the test output of this script:

New MPTCP socket can be blocked via sysctl			   [ OK ]
INFO: validating network environment with pings
INFO: Using loss of 0.01% delay 30 ms reorder 94% 9% with delay 7ms on
ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration    79ms) [ OK ]
ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    49ms) [ OK ]

This patch fixes this by adding ping tests in test output.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 68073b255983..06e945914ace 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -861,6 +861,9 @@ mptcp_lib_result_code "${ret}" "ping tests"
 
 stop_if_error "Could not even run ping tests"
 
+echo -e "ping tests"
+mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
+
 [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
 echo -n "INFO: Using loss of $tc_loss "
 test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
-- 
2.40.1

Re: [PATCH mptcp-next v5 06/12] selftests: mptcp: connect: print out ping tests info

[Matthieu Baerts]

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> 'ping tests' shows in the test results of mptcp_connect.sh:
> 
> 1..68
> ok 1 - mptcp_connect: New MPTCP socket can be blocked via sysctl
> ok 2 - mptcp_connect: ping tests
> ok 3 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1 (10.0.1.1:10000) MPTCP
> ok 4 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1 (10.0.1.1:10001) TCP
> 
> But not in the test output of this script:
> 
> New MPTCP socket can be blocked via sysctl			   [ OK ]
> INFO: validating network environment with pings
> INFO: Using loss of 0.01% delay 30 ms reorder 94% 9% with delay 7ms on
> ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration    79ms) [ OK ]
> ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    49ms) [ OK ]
> 
> This patch fixes this by adding ping tests in test output.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 68073b255983..06e945914ace 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -861,6 +861,9 @@ mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
>  
> +echo -e "ping tests"

Maybe replace the "INFO: validating network environment with pings"
message, no?

> +mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"

Please use the new helper suggested in previous comments, to have
something like:

  print_test "Ping tests"
  (...)
  mptcp_lib_print_success

> +
>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
>  echo -n "INFO: Using loss of $tc_loss "
>  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "

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

Re: [PATCH mptcp-next v5 06/12] selftests: mptcp: connect: print out ping tests info

[Geliang Tang]

On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > 'ping tests' shows in the test results of mptcp_connect.sh:
> > 
> > 1..68
> > ok 1 - mptcp_connect: New MPTCP socket can be blocked via sysctl
> > ok 2 - mptcp_connect: ping tests
> > ok 3 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1
> > (10.0.1.1:10000) MPTCP
> > ok 4 - mptcp_connect: loopback v4: ns1 MPTCP -> ns1
> > (10.0.1.1:10001) TCP
> > 
> > But not in the test output of this script:
> > 
> > New MPTCP socket can be blocked via sysctl			  
> > [ OK ]
> > INFO: validating network environment with pings
> > INFO: Using loss of 0.01% delay 30 ms reorder 94% 9% with delay 7ms
> > on
> > ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration    79ms)
> > [ OK ]
> > ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration    49ms)
> > [ OK ]
> > 
> > This patch fixes this by adding ping tests in test output.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 68073b255983..06e945914ace 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -861,6 +861,9 @@ mptcp_lib_result_code "${ret}" "ping tests"
> >  
> >  stop_if_error "Could not even run ping tests"
> >  
> > +echo -e "ping tests"
> 
> Maybe replace the "INFO: validating network environment with pings"
> message, no?
> 
> > +mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
> 
> Please use the new helper suggested in previous comments, to have
> something like:
> 
>   print_test "Ping tests"
>   (...)
>   mptcp_lib_print_success

I forgot to update this. A squash-to patch just sent out.

Thanks,
-Geliang

> 
> > +
> >  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root
> > netem loss random $tc_loss delay ${tc_delay}ms
> >  echo -n "INFO: Using loss of $tc_loss "
> >  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
> 
> Cheers,
> Matt

[PATCH mptcp-next v5 07/12] selftests: mptcp: print test results with counters

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper mptcp_lib_print_test_counter() to print
out test counter in each test result and increase the counter. Use
this helper to print out test counters for every tests in diag.sh,
mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
and userspace_pm.sh.

Each output looks like:

diag.sh
 01 no msk on netns creation                          [  OK  ]
 02 listen match for dport 10000                      [  OK  ]
 03 listen match for sport 10000                      [  OK  ]
 04 listen match for saddr and sport                  [  OK  ]
 05 all listen sockets                                [  OK  ]

mptcp_connect.sh
 01 New MPTCP socket can be blocked via sysctl                      [ OK ]
 INFO: validating network environment with pings
 02 ping tests                                                      [ OK ]
 INFO: Using loss of 0.16% delay 25 ms reorder .. with delay 6ms on ns3eth4
 03 ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP  (duration 116ms) [ OK ]
 04 ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP    (duration  33ms) [ OK ]
 05 ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP  (duration  25ms) [ OK ]
 06 ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP  (duration 128ms) [ OK ]
 07 ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP    (duration  31ms) [ OK ]

mptcp_sockopt.sh
 01 transfer ipv4                                                [ OK ]
 02 mark ipv4                                                    [ OK ]
 03 transfer ipv6                                                [ OK ]
 04 mark ipv6                                                    [ OK ]
 PASS: all packets had packet mark set
 05 sockopt v4                                                   [ OK ]
 06 sockopt v6                                                   [ OK ]
 PASS: SOL_MPTCP getsockopt has expected information
 07 TCP_INQ: -t tcp                                              [ OK ]
 PASS: TCP_INQ cmsg/ioctl -t tcp
 08 TCP_INQ: -6 -t tcp                                           [ OK ]
 PASS: TCP_INQ cmsg/ioctl -6 -t tcp
 09 TCP_INQ: -r tcp                                              [ OK ]
 PASS: TCP_INQ cmsg/ioctl -r tcp
 10 TCP_INQ: -6 -r tcp                                           [ OK ]

pm_netlink.sh
 01 defaults addr list                        [ OK ]
 02 simple add/get addr                       [ OK ]
 03 dump addrs                                [ OK ]
 04 simple del addr                           [ OK ]
 05 dump addrs after del                      [ OK ]
 06 duplicate addr                            [ OK ]
 07 id addr increment                         [ OK ]
 08 hard addr limit                           [ OK ]
 09 above hard addr limit                     [ OK ]

simult_flows.sh
 01 balanced bwidth                          7411 max 8456       [ OK ]
 02 balanced bwidth - reverse direction      7380 max 8456       [ OK ]
 03 balanced bwidth with unbalanced delay    7434 max 8456       [ OK ]

userspace_pm.sh
 INFO: Init
 01 Created network namespaces ns1, ns2                       [ OK ]
 INFO: Make connections
 02 Established IPv4 MPTCP Connection ns2 => ns1              [ OK ]
 03 Established IPv6 MPTCP Connection ns2 => ns1              [ OK ]
 INFO: Announce tests
 04 ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token             [ OK ]
 05 ADD_ADDR id:14 10.0.2.2 (ns2) => ns1, reuse port          [ OK ]

Having test counters helps to quickly identify issues when looking at a
long list of output logs and results.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh     |  8 ++---
 .../selftests/net/mptcp/mptcp_connect.sh      | 33 +++++++++++--------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  8 +++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 12 ++++---
 .../testing/selftests/net/mptcp/pm_netlink.sh |  6 ++--
 .../selftests/net/mptcp/simult_flows.sh       |  7 ++--
 .../selftests/net/mptcp/userspace_pm.sh       |  3 +-
 7 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index f9f62a8f41e3..01e9f11f1f47 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -9,7 +9,7 @@
 . "$(dirname "${0}")/mptcp_lib.sh"
 
 ns=""
-test_cnt=1
+test_cnt=0
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
 ret=0
@@ -55,7 +55,7 @@ __chk_nr()
 
 	nr=$(eval $command)
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"
 	if [ "$nr" != "$expected" ]; then
 		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
 			mptcp_lib_print_warn "[SKIP] Feature probably not supported"
@@ -69,7 +69,6 @@ __chk_nr()
 		mptcp_lib_print_ok "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
-	test_cnt=$((test_cnt+1))
 }
 
 __chk_msk_nr()
@@ -114,7 +113,7 @@ wait_msk_nr()
 		sleep 1
 	done
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"
 	if [ $i -ge $timeout ]; then
 		mptcp_lib_print_err "[FAIL] timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
@@ -127,7 +126,6 @@ wait_msk_nr()
 		mptcp_lib_print_ok "[ OK ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
-	test_cnt=$((test_cnt+1))
 }
 
 chk_msk_fallback_nr()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 06e945914ace..00bbe451e50d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -131,6 +131,7 @@ ns2=""
 ns3=""
 ns4=""
 
+#shellcheck disable=SC2034
 TEST_COUNT=0
 TEST_GROUP=""
 
@@ -255,8 +256,9 @@ check_mptcp_disabled()
 
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
-		echo -n -e "net.mptcp.enabled sysctl is not 1 by default"
-		mptcp_lib_print_err "\t\t\t   [FAIL]"
+		mptcp_lib_print_test_counter TEST_COUNT "%s" \
+			"net.mptcp.enabled sysctl is not 1 by default"
+		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
 		ret=1
 		return 1
@@ -269,15 +271,17 @@ check_mptcp_disabled()
 	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
-		echo -n -e "New MPTCP socket cannot be blocked via sysctl"
-		mptcp_lib_print_err "\t\t\t   [FAIL]"
+		mptcp_lib_print_test_counter TEST_COUNT "%s" \
+			"New MPTCP socket cannot be blocked via sysctl"
+		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
 		ret=1
 		return 1
 	fi
 
-	echo -n -e "New MPTCP socket can be blocked via sysctl"
-	mptcp_lib_print_ok "\t\t\t   [ OK ]"
+	mptcp_lib_print_test_counter TEST_COUNT "%s" \
+		"New MPTCP socket can be blocked via sysctl"
+	mptcp_lib_print_ok "\t\t\t\t   [ OK ]"
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
 }
@@ -319,7 +323,6 @@ do_transfer()
 
 	local port
 	port=$((10000+PORT++))
-	TEST_COUNT=$((TEST_COUNT+1))
 
 	if [ "$rcvbuf" -gt 0 ]; then
 		extra_args="$extra_args -R $rcvbuf"
@@ -346,7 +349,7 @@ do_transfer()
 	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
 	local result_msg
 	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s" ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port} ${srv_proto})"
-	printf "%s\t" "${result_msg}"
+	mptcp_lib_print_test_counter TEST_COUNT "%s\t" "${result_msg}"
 
 	if $capture; then
 		local capuser
@@ -663,7 +666,8 @@ run_test_transparent()
 	# following function has been exported (T). Not great but better than
 	# checking for a specific kernel version.
 	if ! mptcp_lib_kallsyms_has "T __ip_sock_set_tos$"; then
-		echo "INFO: ${msg} not supported by the kernel: SKIP"
+		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
+			"INFO: ${msg} not supported by the kernel: SKIP"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
 	fi
@@ -680,7 +684,8 @@ table inet mangle {
 }
 EOF
 	then
-		echo "SKIP: $msg, could not load nft ruleset"
+		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
+			"SKIP: $msg, could not load nft ruleset"
 		mptcp_lib_fail_if_expected_feature "nft rules"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
@@ -696,7 +701,8 @@ EOF
 
 	if ! ip -net "$listener_ns" $r6flag rule add fwmark 1 lookup 100; then
 		ip netns exec "$listener_ns" nft flush ruleset
-		echo "SKIP: $msg, ip $r6flag rule failed"
+		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
+			"SKIP: $msg, ip $r6flag rule failed"
 		mptcp_lib_fail_if_expected_feature "ip rule"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
@@ -705,7 +711,8 @@ EOF
 	if ! ip -net "$listener_ns" route add local $local_addr/0 dev lo table 100; then
 		ip netns exec "$listener_ns" nft flush ruleset
 		ip -net "$listener_ns" $r6flag rule del fwmark 1 lookup 100
-		echo "SKIP: $msg, ip route add local $local_addr failed"
+		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
+			"SKIP: $msg, ip route add local $local_addr failed"
 		mptcp_lib_fail_if_expected_feature "ip route"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
@@ -861,7 +868,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
 
 stop_if_error "Could not even run ping tests"
 
-echo -e "ping tests"
+mptcp_lib_print_test_counter TEST_COUNT "%s" "ping tests"
 mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
 
 [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 7e309493eda2..df495658f043 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -411,3 +411,11 @@ mptcp_lib_events() {
 	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
 	pid=$!
 }
+
+mptcp_lib_print_test_counter() {
+	declare -n counter="${1}"
+	local fmt="${2}"
+	local msg="${3}"
+
+	printf "%02u ${fmt}" "$((++counter))" "${msg}"
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index cfa0cfb918f4..a2a5049f22bb 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -17,6 +17,8 @@ timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
 iptables="iptables"
 ip6tables="ip6tables"
+#shellcheck disable=SC2034
+test_cnt=0
 
 ns1=""
 ns2=""
@@ -161,7 +163,7 @@ do_transfer()
 	wait $spid
 	local rets=$?
 
-	printf "%-50s" "transfer ${ip}"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "transfer ${ip}"
 	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
 		echo " client exit code $retc, server $rets" 1>&2
 		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
@@ -178,7 +180,7 @@ do_transfer()
 	fi
 	mptcp_lib_print_ok "[ OK ]"
 
-	printf "%-50s" "mark ${ip}"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "mark ${ip}"
 	if [ $local_addr = "::" ];then
 		check_mark $listener_ns 6 || retc=1
 		check_mark $connector_ns 6 || retc=1
@@ -226,7 +228,7 @@ do_mptcp_sockopt_tests()
 	ip netns exec "$ns_sbox" ./mptcp_sockopt
 	lret=$?
 
-	printf "%-50s" "sockopt v4"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "sockopt v4"
 	if [ $lret -ne 0 ]; then
 		mptcp_lib_print_err "FAIL: SOL_MPTCP getsockopt"
 		mptcp_lib_result_fail "sockopt v4"
@@ -239,7 +241,7 @@ do_mptcp_sockopt_tests()
 	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
 	lret=$?
 
-	printf "%-50s" "sockopt v6"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "sockopt v6"
 	if [ $lret -ne 0 ]; then
 		mptcp_lib_print_err "FAIL: SOL_MPTCP getsockopt (ipv6)"
 		mptcp_lib_result_fail "sockopt v6"
@@ -269,7 +271,7 @@ run_tests()
 
 do_tcpinq_test()
 {
-	printf "%-50s" "TCP_INQ: $*"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "TCP_INQ: $*"
 	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
 	local lret=$?
 	if [ $lret -ne 0 ];then
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 68fad278ac59..87ed60ed4112 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -9,6 +9,8 @@
 . "$(dirname "${0}")/mptcp_lib.sh"
 
 ret=0
+#shellcheck disable=SC2034
+test_cnt=0
 
 usage() {
 	echo "Usage: $0 [ -h ]"
@@ -53,7 +55,7 @@ check()
 	local msg="$3"
 	local rc=0
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"
 	mptcp_lib_check_output "${err}" "${cmd}" "${expected}" || rc=${?}
 	if [ ${rc} -eq 2 ]; then
 		mptcp_lib_result_fail "${msg} # error ${rc}"
@@ -189,7 +191,7 @@ subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
 else
 	for st in fullmesh nofullmesh backup,fullmesh; do
 		st="          (${st})"
-		printf "%-50s" "${st}"
+		mptcp_lib_print_test_counter test_cnt "%-50s" "${st}"
 		mptcp_lib_print_warn "[SKIP]"
 		mptcp_lib_result_skip "${st}"
 	done
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 79cb377ee0bd..eb2eaa48035f 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -14,7 +14,7 @@ ns3=""
 capture=false
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
-test_cnt=1
+test_cnt=0
 ret=0
 bail=0
 slack=50
@@ -127,7 +127,6 @@ do_transfer()
 	local max_time=$3
 	local port
 	port=$((10000+test_cnt))
-	test_cnt=$((test_cnt+1))
 
 	:> "$cout"
 	:> "$sout"
@@ -239,7 +238,7 @@ run_test()
 	# completion (see mptcp_connect): 200ms on each side, add some slack
 	time=$((time + 400 + slack))
 
-	printf "%-60s" "$msg"
+	mptcp_lib_print_test_counter test_cnt "%-60s" "$msg"
 	do_transfer $small $large $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"
@@ -249,7 +248,7 @@ run_test()
 	fi
 
 	msg+=" - reverse direction"
-	printf "%-60s" "${msg}"
+	mptcp_lib_print_test_counter test_cnt "%-60s" "${msg}"
 	do_transfer $large $small $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 33bbb0d5807f..27f308601005 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -53,6 +53,7 @@ server_addr_id=${RANDOM:0:2}
 ns1=""
 ns2=""
 ret=0
+test_cnt=0
 test_name=""
 
 _printf() {
@@ -69,7 +70,7 @@ print_test()
 {
 	test_name="${1}"
 
-	_printf "%-68s" "${test_name}"
+	mptcp_lib_print_test_counter test_cnt "%-68s" "${test_name}"
 }
 
 test_pass()
-- 
2.40.1

Re: [PATCH mptcp-next v5 07/12] selftests: mptcp: print test results with counters

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper mptcp_lib_print_test_counter() to print
> out test counter in each test result and increase the counter. Use
> this helper to print out test counters for every tests in diag.sh,
> mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
> and userspace_pm.sh.
> 
> Each output looks like:
> 
> diag.sh
>  01 no msk on netns creation                          [  OK  ]
>  02 listen match for dport 10000                      [  OK  ]
>  03 listen match for sport 10000                      [  OK  ]
>  04 listen match for saddr and sport                  [  OK  ]
>  05 all listen sockets                                [  OK  ]
> 
> mptcp_connect.sh
>  01 New MPTCP socket can be blocked via sysctl                      [ OK ]
>  INFO: validating network environment with pings
>  02 ping tests                                                      [ OK ]
>  INFO: Using loss of 0.16% delay 25 ms reorder .. with delay 6ms on ns3eth4
>  03 ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP  (duration 116ms) [ OK ]
>  04 ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP    (duration  33ms) [ OK ]
>  05 ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP  (duration  25ms) [ OK ]
>  06 ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP  (duration 128ms) [ OK ]
>  07 ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP    (duration  31ms) [ OK ]
> 
> mptcp_sockopt.sh
>  01 transfer ipv4                                                [ OK ]
>  02 mark ipv4                                                    [ OK ]
>  03 transfer ipv6                                                [ OK ]
>  04 mark ipv6                                                    [ OK ]
>  PASS: all packets had packet mark set
>  05 sockopt v4                                                   [ OK ]
>  06 sockopt v6                                                   [ OK ]
>  PASS: SOL_MPTCP getsockopt has expected information
>  07 TCP_INQ: -t tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -t tcp
>  08 TCP_INQ: -6 -t tcp                                           [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>  09 TCP_INQ: -r tcp                                              [ OK ]
>  PASS: TCP_INQ cmsg/ioctl -r tcp
>  10 TCP_INQ: -6 -r tcp                                           [ OK ]
> 
> pm_netlink.sh
>  01 defaults addr list                        [ OK ]
>  02 simple add/get addr                       [ OK ]
>  03 dump addrs                                [ OK ]
>  04 simple del addr                           [ OK ]
>  05 dump addrs after del                      [ OK ]
>  06 duplicate addr                            [ OK ]
>  07 id addr increment                         [ OK ]
>  08 hard addr limit                           [ OK ]
>  09 above hard addr limit                     [ OK ]
> 
> simult_flows.sh
>  01 balanced bwidth                          7411 max 8456       [ OK ]
>  02 balanced bwidth - reverse direction      7380 max 8456       [ OK ]
>  03 balanced bwidth with unbalanced delay    7434 max 8456       [ OK ]
> 
> userspace_pm.sh
>  INFO: Init
>  01 Created network namespaces ns1, ns2                       [ OK ]
>  INFO: Make connections
>  02 Established IPv4 MPTCP Connection ns2 => ns1              [ OK ]
>  03 Established IPv6 MPTCP Connection ns2 => ns1              [ OK ]
>  INFO: Announce tests
>  04 ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token             [ OK ]
>  05 ADD_ADDR id:14 10.0.2.2 (ns2) => ns1, reuse port          [ OK ]
> 
> Having test counters helps to quickly identify issues when looking at a
> long list of output logs and results.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh     |  8 ++---
>  .../selftests/net/mptcp/mptcp_connect.sh      | 33 +++++++++++--------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  |  8 +++++
>  .../selftests/net/mptcp/mptcp_sockopt.sh      | 12 ++++---
>  .../testing/selftests/net/mptcp/pm_netlink.sh |  6 ++--
>  .../selftests/net/mptcp/simult_flows.sh       |  7 ++--
>  .../selftests/net/mptcp/userspace_pm.sh       |  3 +-
>  7 files changed, 47 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index f9f62a8f41e3..01e9f11f1f47 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -9,7 +9,7 @@
>  . "$(dirname "${0}")/mptcp_lib.sh"
>  
>  ns=""
> -test_cnt=1
> +test_cnt=0

Is it normal shellcheck doesn't complain about it not being used?

>  timeout_poll=30
>  timeout_test=$((timeout_poll * 2 + 1))
>  ret=0
> @@ -55,7 +55,7 @@ __chk_nr()
>  
>  	nr=$(eval $command)
>  
> -	printf "%-50s" "$msg"
> +	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"

Same here, probably best with a helper, to avoid repeating the long list
of arguments:

  # $1: test name
  print_test() {
      mptcp_lib_print_test_counter test_cnt "%-50s" "${1}"
  }

  (...)

  print_test "${msg}"

Same in the other files not using a helper already (like mptcp_join.sh
and userspace_pm.sh)

(see my comment in mptcp_lib.sh: maybe easier to remove params, and just
call 'mptcp_lib_print_test_counter "${msg}"')

>  	if [ "$nr" != "$expected" ]; then
>  		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
>  			mptcp_lib_print_warn "[SKIP] Feature probably not supported"

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 06e945914ace..00bbe451e50d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -131,6 +131,7 @@ ns2=""
>  ns3=""
>  ns4=""
>  
> +#shellcheck disable=SC2034

Please justify all disabled shellcheck checks, e.g.

#shellcheck disable=SC2034 # TEST_COUNT is used by mptcp_lib.sh

same below

(see my comment below: why not defining it in mptcp_lib.sh then?)

>  TEST_COUNT=0
>  TEST_GROUP=""
>  
> @@ -255,8 +256,9 @@ check_mptcp_disabled()
>  
>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> -		echo -n -e "net.mptcp.enabled sysctl is not 1 by default"
> -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> +			"net.mptcp.enabled sysctl is not 1 by default"

The test name should be printed before the test: so the counter will be
incremented in case of issue or not:

  mptcp_lib_print_test_counter "MPTCP socket can be blocked via sysctl"
  if [ ... ]; then
       mptcp_lib_print_fail "net.mptcp.enabled sysctl is not (...)"
       (...)
  fi

  if [ ... ]; then
       mptcp_lib_print_fail "MPTCP socket cannot be blocked via sysctl"
       (...)
  fi

  mptcp_lib_print_success


> +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
>  		ret=1
>  		return 1
> @@ -269,15 +271,17 @@ check_mptcp_disabled()
>  	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
> -		echo -n -e "New MPTCP socket cannot be blocked via sysctl"
> -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> +			"New MPTCP socket cannot be blocked via sysctl"
> +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
>  		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
>  		ret=1
>  		return 1
>  	fi
>  
> -	echo -n -e "New MPTCP socket can be blocked via sysctl"
> -	mptcp_lib_print_ok "\t\t\t   [ OK ]"
> +	mptcp_lib_print_test_counter TEST_COUNT "%s" \
> +		"New MPTCP socket can be blocked via sysctl"
> +	mptcp_lib_print_ok "\t\t\t\t   [ OK ]"
>  	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
>  	return 0
>  }
> @@ -319,7 +323,6 @@ do_transfer()
>  
>  	local port
>  	port=$((10000+PORT++))
> -	TEST_COUNT=$((TEST_COUNT+1))
>  
>  	if [ "$rcvbuf" -gt 0 ]; then
>  		extra_args="$extra_args -R $rcvbuf"
> @@ -346,7 +349,7 @@ do_transfer()
>  	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
>  	local result_msg
>  	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s" ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port} ${srv_proto})"
> -	printf "%s\t" "${result_msg}"
> +	mptcp_lib_print_test_counter TEST_COUNT "%s\t" "${result_msg}"
>  
>  	if $capture; then
>  		local capuser
> @@ -663,7 +666,8 @@ run_test_transparent()
>  	# following function has been exported (T). Not great but better than
>  	# checking for a specific kernel version.
>  	if ! mptcp_lib_kallsyms_has "T __ip_sock_set_tos$"; then
> -		echo "INFO: ${msg} not supported by the kernel: SKIP"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> +			"INFO: ${msg} not supported by the kernel: SKIP"

Same here, the test name and counter should be handled before the 'if',
not just for the SKIP if possible.

>  		mptcp_lib_result_skip "${TEST_GROUP}"
>  		return
>  	fi
> @@ -680,7 +684,8 @@ table inet mangle {
>  }
>  EOF
>  	then
> -		echo "SKIP: $msg, could not load nft ruleset"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> +			"SKIP: $msg, could not load nft ruleset"

Same here, the test name and counter should be handled before the 'if',
not just for the SKIP if possible.

>  		mptcp_lib_fail_if_expected_feature "nft rules"
>  		mptcp_lib_result_skip "${TEST_GROUP}"
>  		return
> @@ -696,7 +701,8 @@ EOF
>  
>  	if ! ip -net "$listener_ns" $r6flag rule add fwmark 1 lookup 100; then
>  		ip netns exec "$listener_ns" nft flush ruleset
> -		echo "SKIP: $msg, ip $r6flag rule failed"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> +			"SKIP: $msg, ip $r6flag rule failed"

Same here, the test name and counter should be handled before the 'if',
not just for the SKIP if possible.

>  		mptcp_lib_fail_if_expected_feature "ip rule"
>  		mptcp_lib_result_skip "${TEST_GROUP}"
>  		return
> @@ -705,7 +711,8 @@ EOF
>  	if ! ip -net "$listener_ns" route add local $local_addr/0 dev lo table 100; then
>  		ip netns exec "$listener_ns" nft flush ruleset
>  		ip -net "$listener_ns" $r6flag rule del fwmark 1 lookup 100
> -		echo "SKIP: $msg, ip route add local $local_addr failed"
> +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> +			"SKIP: $msg, ip route add local $local_addr failed"

Same here, the test name and counter should be handled before the 'if',
not just for the SKIP if possible.

>  		mptcp_lib_fail_if_expected_feature "ip route"
>  		mptcp_lib_result_skip "${TEST_GROUP}"
>  		return
> @@ -861,7 +868,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
>  
> -echo -e "ping tests"
> +mptcp_lib_print_test_counter TEST_COUNT "%s" "ping tests"
>  mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
>  
>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 7e309493eda2..df495658f043 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -411,3 +411,11 @@ mptcp_lib_events() {
>  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
>  	pid=$!
>  }
> +
> +mptcp_lib_print_test_counter() {

Maybe more "mptcp_lib_print_test_title" or "_name": it doesn't just
print the counter, mostly the "title" (including the counter, part of
the title)

> +	declare -n counter="${1}"
> +	local fmt="${2}"
> +	local msg="${3}"
> +
> +	printf "%02u ${fmt}" "$((++counter))" "${msg}"

Maybe having this:

  : "${MPTCP_LIB_PRINT_TEST_FORMAT:="%02u %-50s"}"
  MPTCP_LIB_TEST_COUNTER=0

  (...)

  # $1: test name
  mptcp_lib_print_test_counter() {
      printf "${MPTCP_LIB_PRINT_TEST_FORMAT}" \
             "$((++MPTCP_LIB_TEST_COUNTER))" "${1}"
  }

Would simplify stuff?

So tests can specify the format by setting MPTCP_LIB_PRINT_TEST_FORMAT,
e.g. to support more than 99 entries. And you don't need to define
TEST_COUNT and disable SC2034.

Would that work?

You could even use it in mptcp_join.sh: print_title() would call this
new helper.

> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index cfa0cfb918f4..a2a5049f22bb 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -17,6 +17,8 @@ timeout_poll=30
>  timeout_test=$((timeout_poll * 2 + 1))
>  iptables="iptables"
>  ip6tables="ip6tables"
> +#shellcheck disable=SC2034
> +test_cnt=0
>  
>  ns1=""
>  ns2=""
> @@ -161,7 +163,7 @@ do_transfer()
>  	wait $spid
>  	local rets=$?
>  
> -	printf "%-50s" "transfer ${ip}"
> +	mptcp_lib_print_test_counter test_cnt "%-50s" "transfer ${ip}"

Same my comments on previous patches: if here a helper is used to print
the test name, you would only need to change this helper, not doing the
same modification multiple times at the same places.

>  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
>  		echo " client exit code $retc, server $rets" 1>&2
>  		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2

(...)

> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 79cb377ee0bd..eb2eaa48035f 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -14,7 +14,7 @@ ns3=""
>  capture=false
>  timeout_poll=30
>  timeout_test=$((timeout_poll * 2 + 1))
> -test_cnt=1
> +test_cnt=0

Why ShellCheck is not complaining the variable is not used here?

>  ret=0
>  bail=0
>  slack=50

(...)

While at it, can you make sure all the results are aligned for
simult_flows by increasing the '%-XXs'?

> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 33bbb0d5807f..27f308601005 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -53,6 +53,7 @@ server_addr_id=${RANDOM:0:2}
>  ns1=""
>  ns2=""
>  ret=0

Why ShellCheck is not complaining the variable is not used here?

> +test_cnt=0
>  test_name=""
>  
>  _printf() {

(...)

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

Re: [PATCH mptcp-next v5 07/12] selftests: mptcp: print test results with counters

[Geliang Tang]

On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 10:43, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds a new helper mptcp_lib_print_test_counter() to
> > print
> > out test counter in each test result and increase the counter. Use
> > this helper to print out test counters for every tests in diag.sh,
> > mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
> > and userspace_pm.sh.
> > 
> > Each output looks like:
> > 
> > diag.sh
> >  01 no msk on netns creation                          [  OK  ]
> >  02 listen match for dport 10000                      [  OK  ]
> >  03 listen match for sport 10000                      [  OK  ]
> >  04 listen match for saddr and sport                  [  OK  ]
> >  05 all listen sockets                                [  OK  ]
> > 
> > mptcp_connect.sh
> >  01 New MPTCP socket can be blocked via sysctl                     
> > [ OK ]
> >  INFO: validating network environment with pings
> >  02 ping tests                                                     
> > [ OK ]
> >  INFO: Using loss of 0.16% delay 25 ms reorder .. with delay 6ms on
> > ns3eth4
> >  03 ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP  (duration 116ms)
> > [ OK ]
> >  04 ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP    (duration  33ms)
> > [ OK ]
> >  05 ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP  (duration  25ms)
> > [ OK ]
> >  06 ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP  (duration 128ms)
> > [ OK ]
> >  07 ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP    (duration  31ms)
> > [ OK ]
> > 
> > mptcp_sockopt.sh
> >  01 transfer ipv4                                                [
> > OK ]
> >  02 mark ipv4                                                    [
> > OK ]
> >  03 transfer ipv6                                                [
> > OK ]
> >  04 mark ipv6                                                    [
> > OK ]
> >  PASS: all packets had packet mark set
> >  05 sockopt v4                                                   [
> > OK ]
> >  06 sockopt v6                                                   [
> > OK ]
> >  PASS: SOL_MPTCP getsockopt has expected information
> >  07 TCP_INQ: -t tcp                                              [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -t tcp
> >  08 TCP_INQ: -6 -t tcp                                           [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> >  09 TCP_INQ: -r tcp                                              [
> > OK ]
> >  PASS: TCP_INQ cmsg/ioctl -r tcp
> >  10 TCP_INQ: -6 -r tcp                                           [
> > OK ]
> > 
> > pm_netlink.sh
> >  01 defaults addr list                        [ OK ]
> >  02 simple add/get addr                       [ OK ]
> >  03 dump addrs                                [ OK ]
> >  04 simple del addr                           [ OK ]
> >  05 dump addrs after del                      [ OK ]
> >  06 duplicate addr                            [ OK ]
> >  07 id addr increment                         [ OK ]
> >  08 hard addr limit                           [ OK ]
> >  09 above hard addr limit                     [ OK ]
> > 
> > simult_flows.sh
> >  01 balanced bwidth                          7411 max 8456       [
> > OK ]
> >  02 balanced bwidth - reverse direction      7380 max 8456       [
> > OK ]
> >  03 balanced bwidth with unbalanced delay    7434 max 8456       [
> > OK ]
> > 
> > userspace_pm.sh
> >  INFO: Init
> >  01 Created network namespaces ns1, ns2                       [ OK
> > ]
> >  INFO: Make connections
> >  02 Established IPv4 MPTCP Connection ns2 => ns1              [ OK
> > ]
> >  03 Established IPv6 MPTCP Connection ns2 => ns1              [ OK
> > ]
> >  INFO: Announce tests
> >  04 ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token             [ OK
> > ]
> >  05 ADD_ADDR id:14 10.0.2.2 (ns2) => ns1, reuse port          [ OK
> > ]
> > 
> > Having test counters helps to quickly identify issues when looking
> > at a
> > long list of output logs and results.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/diag.sh     |  8 ++---
> >  .../selftests/net/mptcp/mptcp_connect.sh      | 33 +++++++++++----
> > ----
> >  .../testing/selftests/net/mptcp/mptcp_lib.sh  |  8 +++++
> >  .../selftests/net/mptcp/mptcp_sockopt.sh      | 12 ++++---
> >  .../testing/selftests/net/mptcp/pm_netlink.sh |  6 ++--
> >  .../selftests/net/mptcp/simult_flows.sh       |  7 ++--
> >  .../selftests/net/mptcp/userspace_pm.sh       |  3 +-
> >  7 files changed, 47 insertions(+), 30 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> > b/tools/testing/selftests/net/mptcp/diag.sh
> > index f9f62a8f41e3..01e9f11f1f47 100755
> > --- a/tools/testing/selftests/net/mptcp/diag.sh
> > +++ b/tools/testing/selftests/net/mptcp/diag.sh
> > @@ -9,7 +9,7 @@
> >  . "$(dirname "${0}")/mptcp_lib.sh"
> >  
> >  ns=""
> > -test_cnt=1
> > +test_cnt=0
> 
> Is it normal shellcheck doesn't complain about it not being used?

test_cnt still be used in this script in another place:

	ret=$test_cnt

> 
> >  timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> >  ret=0
> > @@ -55,7 +55,7 @@ __chk_nr()
> >  
> >  	nr=$(eval $command)
> >  
> > -	printf "%-50s" "$msg"
> > +	mptcp_lib_print_test_counter test_cnt "%-50s" "$msg"
> 
> Same here, probably best with a helper, to avoid repeating the long
> list
> of arguments:
> 
>   # $1: test name
>   print_test() {
>       mptcp_lib_print_test_counter test_cnt "%-50s" "${1}"
>   }
> 
>   (...)
> 
>   print_test "${msg}"


print_title is added here.

> 
> Same in the other files not using a helper already (like
> mptcp_join.sh
> and userspace_pm.sh)
> 
> (see my comment in mptcp_lib.sh: maybe easier to remove params, and
> just
> call 'mptcp_lib_print_test_counter "${msg}"')
> 
> >  	if [ "$nr" != "$expected" ]; then
> >  		if [ "$nr" = "$skip" ] && !
> > mptcp_lib_expect_all_features; then
> >  			mptcp_lib_print_warn "[SKIP] Feature
> > probably not supported"
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 06e945914ace..00bbe451e50d 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -131,6 +131,7 @@ ns2=""
> >  ns3=""
> >  ns4=""
> >  
> > +#shellcheck disable=SC2034
> 
> Please justify all disabled shellcheck checks, e.g.
> 
> #shellcheck disable=SC2034 # TEST_COUNT is used by mptcp_lib.sh

Updated.

> 
> same below
> 
> (see my comment below: why not defining it in mptcp_lib.sh then?)
> 
> >  TEST_COUNT=0
> >  TEST_GROUP=""
> >  
> > @@ -255,8 +256,9 @@ check_mptcp_disabled()
> >  
> >  	# net.mptcp.enabled should be enabled by default
> >  	if [ "$(ip netns exec ${disabled_ns} sysctl
> > net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
> > -		echo -n -e "net.mptcp.enabled sysctl is not 1 by
> > default"
> > -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +			"net.mptcp.enabled sysctl is not 1 by
> > default"
> 
> The test name should be printed before the test: so the counter will
> be
> incremented in case of issue or not:
> 
>   mptcp_lib_print_test_counter "MPTCP socket can be blocked via
> sysctl"
>   if [ ... ]; then
>        mptcp_lib_print_fail "net.mptcp.enabled sysctl is not (...)"
>        (...)
>   fi
> 
>   if [ ... ]; then
>        mptcp_lib_print_fail "MPTCP socket cannot be blocked via
> sysctl"
>        (...)
>   fi
> 
>   mptcp_lib_print_success

Updated.

> 
> 
> > +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "net.mptcp.enabled sysctl is
> > not 1 by default"
> >  		ret=1
> >  		return 1
> > @@ -269,15 +271,17 @@ check_mptcp_disabled()
> >  	mptcp_lib_ns_exit "${disabled_ns}"
> >  
> >  	if [ ${err} -eq 0 ]; then
> > -		echo -n -e "New MPTCP socket cannot be blocked via
> > sysctl"
> > -		mptcp_lib_print_err "\t\t\t   [FAIL]"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +			"New MPTCP socket cannot be blocked via
> > sysctl"
> > +		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
> >  		mptcp_lib_result_fail "New MPTCP socket cannot be
> > blocked via sysctl"
> >  		ret=1
> >  		return 1
> >  	fi
> >  
> > -	echo -n -e "New MPTCP socket can be blocked via sysctl"
> > -	mptcp_lib_print_ok "\t\t\t   [ OK ]"
> > +	mptcp_lib_print_test_counter TEST_COUNT "%s" \
> > +		"New MPTCP socket can be blocked via sysctl"
> > +	mptcp_lib_print_ok "\t\t\t\t   [ OK ]"
> >  	mptcp_lib_result_pass "New MPTCP socket can be blocked via
> > sysctl"
> >  	return 0
> >  }
> > @@ -319,7 +323,6 @@ do_transfer()
> >  
> >  	local port
> >  	port=$((10000+PORT++))
> > -	TEST_COUNT=$((TEST_COUNT+1))
> >  
> >  	if [ "$rcvbuf" -gt 0 ]; then
> >  		extra_args="$extra_args -R $rcvbuf"
> > @@ -346,7 +349,7 @@ do_transfer()
> >  	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
> >  	local result_msg
> >  	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s"
> > ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port}
> > ${srv_proto})"
> > -	printf "%s\t" "${result_msg}"
> > +	mptcp_lib_print_test_counter TEST_COUNT "%s\t"
> > "${result_msg}"
> >  
> >  	if $capture; then
> >  		local capuser
> > @@ -663,7 +666,8 @@ run_test_transparent()
> >  	# following function has been exported (T). Not great but
> > better than
> >  	# checking for a specific kernel version.
> >  	if ! mptcp_lib_kallsyms_has "T __ip_sock_set_tos$"; then
> > -		echo "INFO: ${msg} not supported by the kernel:
> > SKIP"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"INFO: ${msg} not supported by the kernel:
> > SKIP"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> >  	fi
> > @@ -680,7 +684,8 @@ table inet mangle {
> >  }
> >  EOF
> >  	then
> > -		echo "SKIP: $msg, could not load nft ruleset"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, could not load nft ruleset"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "nft rules"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -696,7 +701,8 @@ EOF
> >  
> >  	if ! ip -net "$listener_ns" $r6flag rule add fwmark 1
> > lookup 100; then
> >  		ip netns exec "$listener_ns" nft flush ruleset
> > -		echo "SKIP: $msg, ip $r6flag rule failed"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, ip $r6flag rule failed"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "ip rule"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -705,7 +711,8 @@ EOF
> >  	if ! ip -net "$listener_ns" route add local $local_addr/0
> > dev lo table 100; then
> >  		ip netns exec "$listener_ns" nft flush ruleset
> >  		ip -net "$listener_ns" $r6flag rule del fwmark 1
> > lookup 100
> > -		echo "SKIP: $msg, ip route add local $local_addr
> > failed"
> > +		mptcp_lib_print_test_counter TEST_COUNT "%s\n" \
> > +			"SKIP: $msg, ip route add local
> > $local_addr failed"
> 
> Same here, the test name and counter should be handled before the
> 'if',
> not just for the SKIP if possible.

Updated.

> 
> >  		mptcp_lib_fail_if_expected_feature "ip route"
> >  		mptcp_lib_result_skip "${TEST_GROUP}"
> >  		return
> > @@ -861,7 +868,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
> >  
> >  stop_if_error "Could not even run ping tests"
> >  
> > -echo -e "ping tests"
> > +mptcp_lib_print_test_counter TEST_COUNT "%s" "ping tests"
> >  mptcp_lib_print_ok "\t\t\t\t\t\t\t\t   [ OK ]"
> >  
> >  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root
> > netem loss random $tc_loss delay ${tc_delay}ms
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > index 7e309493eda2..df495658f043 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > @@ -411,3 +411,11 @@ mptcp_lib_events() {
> >  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1
> > &
> >  	pid=$!
> >  }
> > +
> > +mptcp_lib_print_test_counter() {
> 
> Maybe more "mptcp_lib_print_test_title" or "_name": it doesn't just
> print the counter, mostly the "title" (including the counter, part of
> the title)

Rename to mptcp_lib_print_title.

> 
> > +	declare -n counter="${1}"
> > +	local fmt="${2}"
> > +	local msg="${3}"
> > +
> > +	printf "%02u ${fmt}" "$((++counter))" "${msg}"
> 
> Maybe having this:
> 
>   : "${MPTCP_LIB_PRINT_TEST_FORMAT:="%02u %-50s"}"
>   MPTCP_LIB_TEST_COUNTER=0
> 
>   (...)
> 
>   # $1: test name
>   mptcp_lib_print_test_counter() {
>       printf "${MPTCP_LIB_PRINT_TEST_FORMAT}" \
>              "$((++MPTCP_LIB_TEST_COUNTER))" "${1}"
>   }
> 
> Would simplify stuff?

Do you mean to define TEST_COUNT in mptcp_lib.sh? That means we need to
rename test_cnt to TEST_COUNT. I remember you objected to move vars
into mptcp_lib.sh like in "selftests: mptcp: add mptcp_lib_ns_init/exit
helpers". So I pass TEST_COUNT or test_cnt as an argument to
mptcp_lib_print_test_counter().

I think this can keep continuity with other functions in mptcp_lib,
like mptcp_lib_ns_init/exit.

> 
> So tests can specify the format by setting
> MPTCP_LIB_PRINT_TEST_FORMAT,
> e.g. to support more than 99 entries. And you don't need to define
> TEST_COUNT and disable SC2034.
> 
> Would that work?
> 
> You could even use it in mptcp_join.sh: print_title() would call this
> new helper.
> 
> > +}
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > index cfa0cfb918f4..a2a5049f22bb 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > @@ -17,6 +17,8 @@ timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> >  iptables="iptables"
> >  ip6tables="ip6tables"
> > +#shellcheck disable=SC2034
> > +test_cnt=0
> >  
> >  ns1=""
> >  ns2=""
> > @@ -161,7 +163,7 @@ do_transfer()
> >  	wait $spid
> >  	local rets=$?
> >  
> > -	printf "%-50s" "transfer ${ip}"
> > +	mptcp_lib_print_test_counter test_cnt "%-50s" "transfer
> > ${ip}"
> 
> Same my comments on previous patches: if here a helper is used to
> print
> the test name, you would only need to change this helper, not doing
> the
> same modification multiple times at the same places.

Do this in print_title helper.

> 
> >  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
> >  		echo " client exit code $retc, server $rets" 1>&2
> >  		echo -e "\nnetns ${listener_ns} socket stat for
> > ${port}:" 1>&2
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > index 79cb377ee0bd..eb2eaa48035f 100755
> > --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> > +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> > @@ -14,7 +14,7 @@ ns3=""
> >  capture=false
> >  timeout_poll=30
> >  timeout_test=$((timeout_poll * 2 + 1))
> > -test_cnt=1
> > +test_cnt=0
> 
> Why ShellCheck is not complaining the variable is not used here?

test_cnt still be used in this script.

> 
> >  ret=0
> >  bail=0
> >  slack=50
> 
> (...)
> 
> While at it, can you make sure all the results are aligned for
> simult_flows by increasing the '%-XXs'?
> 
> > diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > index 33bbb0d5807f..27f308601005 100755
> > --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > @@ -53,6 +53,7 @@ server_addr_id=${RANDOM:0:2}
> >  ns1=""
> >  ns2=""
> >  ret=0
> 
> Why ShellCheck is not complaining the variable is not used here?

The same.

> 
> > +test_cnt=0
> >  test_name=""
> >  
> >  _printf() {
> 
> (...)
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v5 07/12] selftests: mptcp: print test results with counters

[Matthieu Baerts]

Hi Geliang,

On 28/02/2024 09:23, Geliang Tang wrote:
> On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 26/02/2024 10:43, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch adds a new helper mptcp_lib_print_test_counter() to
>>> print
>>> out test counter in each test result and increase the counter. Use
>>> this helper to print out test counters for every tests in diag.sh,
>>> mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
>>> and userspace_pm.sh.

(...)

>>> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
>>> b/tools/testing/selftests/net/mptcp/diag.sh
>>> index f9f62a8f41e3..01e9f11f1f47 100755
>>> --- a/tools/testing/selftests/net/mptcp/diag.sh
>>> +++ b/tools/testing/selftests/net/mptcp/diag.sh
>>> @@ -9,7 +9,7 @@
>>>  . "$(dirname "${0}")/mptcp_lib.sh"
>>>  
>>>  ns=""
>>> -test_cnt=1
>>> +test_cnt=0
>>
>> Is it normal shellcheck doesn't complain about it not being used?
> 
> test_cnt still be used in this script in another place:
> 
> 	ret=$test_cnt

Oh, I remember I saw that when fixing other thing and apparently I
forgot about that: we should stop doing that! e.g. what if only the 4th
test fail? → We will do 'exit 4' which is 'exit ${KSFT_SKIP}' → the
whole test will be marked as skipped instead of 'failed'!

So we should do ret=${KSFT_FAIL} (or ret=1).

Do you mind sending a patch fixing that for mptcp-net please? (with a
Fixes tag: I guess it is there from the beginning or almost).

>>>  timeout_poll=30
>>>  timeout_test=$((timeout_poll * 2 + 1))
>>>  ret=0

(...)

>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> index 7e309493eda2..df495658f043 100644
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> @@ -411,3 +411,11 @@ mptcp_lib_events() {
>>>  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1
>>> &
>>>  	pid=$!
>>>  }
>>> +
>>> +mptcp_lib_print_test_counter() {
>>
>> Maybe more "mptcp_lib_print_test_title" or "_name": it doesn't just
>> print the counter, mostly the "title" (including the counter, part of
>> the title)
> 
> Rename to mptcp_lib_print_title.
> 
>>
>>> +	declare -n counter="${1}"
>>> +	local fmt="${2}"
>>> +	local msg="${3}"
>>> +
>>> +	printf "%02u ${fmt}" "$((++counter))" "${msg}"
>>
>> Maybe having this:
>>
>>   : "${MPTCP_LIB_PRINT_TEST_FORMAT:="%02u %-50s"}"
>>   MPTCP_LIB_TEST_COUNTER=0
>>
>>   (...)
>>
>>   # $1: test name
>>   mptcp_lib_print_test_counter() {
>>       printf "${MPTCP_LIB_PRINT_TEST_FORMAT}" \
>>              "$((++MPTCP_LIB_TEST_COUNTER))" "${1}"
>>   }
>>
>> Would simplify stuff?
> 
> Do you mean to define TEST_COUNT in mptcp_lib.sh? That means we need to
> rename test_cnt to TEST_COUNT. I remember you objected to move vars
> into mptcp_lib.sh like in "selftests: mptcp: add mptcp_lib_ns_init/exit
> helpers". So I pass TEST_COUNT or test_cnt as an argument to
> mptcp_lib_print_test_counter().

Here it is different I think: here I suggest moving TEST_COUNT to
mptcp_lib.sh (renamed MPTCP_LIB_TEST_COUNTER), use it for all tests, and
only use it in mptcp_lib.sh. For the netns vars, they were use in many
places, with different names, and not the same number → we didn't want
to rename them everywhere. Here, it is a basic counter, just a single
variable that can be used in all tests, in very specific places (only to
read it, not to modify it).

I don't know, it looks different to me. If we can use it the same way
everywhere (when printing test title), that looks like a good modification.

> I think this can keep continuity with other functions in mptcp_lib,
> like mptcp_lib_ns_init/exit.
> 
>>
>> So tests can specify the format by setting
>> MPTCP_LIB_PRINT_TEST_FORMAT,
>> e.g. to support more than 99 entries. And you don't need to define
>> TEST_COUNT and disable SC2034.
>>
>> Would that work?
>>
>> You could even use it in mptcp_join.sh: print_title() would call this
>> new helper.
>>
>>> +}
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v5 08/12] selftests: mptcp: move test_fail out of check_expected_one

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch moves test_fail out of check_expected_one(), since test_fail
is a private function in userspace_pm.sh, and check_expected_one will be
exported in mptcp_lib.sh in the next commit.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 27f308601005..473639a8009f 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -247,7 +247,7 @@ check_expected_one()
 
 	if [ "${prev_ret}" = "0" ]
 	then
-		test_fail
+		return 2
 	fi
 
 	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \
@@ -263,13 +263,20 @@ check_expected()
 
 	for var in "${@}"
 	do
-		check_expected_one "${var}" "${rc}" || rc=1
+		check_expected_one "${var}" "${rc}" || rc="${?}"
+		if [ "${rc}" -eq 2 ]
+		then
+			break
+		fi
 	done
 
 	if [ ${rc} -eq 0 ]
 	then
 		test_pass
 		return 0
+	elif [ "${rc}" -eq 2 ]
+	then
+		test_fail
 	fi
 
 	return 1
-- 
2.40.1

Re: [PATCH mptcp-next v5 08/12] selftests: mptcp: move test_fail out of check_expected_one

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch moves test_fail out of check_expected_one(), since test_fail
> is a private function in userspace_pm.sh, and check_expected_one will be
> exported in mptcp_lib.sh in the next commit.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 27f308601005..473639a8009f 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -247,7 +247,7 @@ check_expected_one()
>  
>  	if [ "${prev_ret}" = "0" ]
>  	then
> -		test_fail

I think, the goal to do it here was to print "FAIL", then the details.

> +		return 2

Just return 1, no?

>  	fi
>  
>  	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \

What's the output now in case of issue? Does it still make sense to use
the '\t' here at the beginning?

> @@ -263,13 +263,20 @@ check_expected()
>  
>  	for var in "${@}"
>  	do
> -		check_expected_one "${var}" "${rc}" || rc=1
> +		check_expected_one "${var}" "${rc}" || rc="${?}"
> +		if [ "${rc}" -eq 2 ]
> +		then
> +			break

Why breaking? If you break, you only print one error, not all like before.

Please check the behaviour in case of error before and after the
modification.

> +		fi
>  	done
>  
>  	if [ ${rc} -eq 0 ]
>  	then
>  		test_pass
>  		return 0
> +	elif [ "${rc}" -eq 2 ]
> +	then
> +		test_fail

Just 'test_fail' after the 'if'.

>  	fi
>  
>  	return 1

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

[PATCH mptcp-next v5 09/12] selftests: mptcp: extract mptcp_lib_check_expected

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Extract the main part of check_expected() in userspace_pm.sh to a new
function mptcp_lib_check_expected() in mptcp_lib.sh. It will be used
in both mptcp_john.sh and userspace_pm.sh.

check_expected_one() is moved into mptcp_lib.sh too as a sub function
of mptcp_lib_check_expected().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 48 +++++++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 39 ++-------------
 2 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index df495658f043..ed0013b47edb 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -1,6 +1,9 @@
 #! /bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
+# Some variables are used below but indirectly, see check_expected_one()
+#shellcheck disable=SC2034
+
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
 readonly KSFT_SKIP=4
@@ -419,3 +422,48 @@ mptcp_lib_print_test_counter() {
 
 	printf "%02u ${fmt}" "$((++counter))" "${msg}"
 }
+
+# $@: all var names to check
+mptcp_lib_check_expected() {
+	# $1: var name ; $2: prev ret
+	check_expected_one() {
+		local var="${1}"
+		local exp="e_${var}"
+		local prev_ret="${2}"
+
+		if [ "${!var}" = "${!exp}" ]
+		then
+			return 0
+		fi
+
+		if [ "${prev_ret}" = "0" ]
+		then
+			return 2
+		fi
+
+		printf "\tExpected value for '%s': '%s', got '%s'.\n" \
+			"${var}" "${!exp}" "${!var}"
+		return 1
+	}
+
+	local rc=0
+	local var
+
+	for var in "${@}"
+	do
+		check_expected_one "${var}" "${rc}" || rc="${?}"
+		if [ "${rc}" -eq 2 ]
+		then
+			break
+		fi
+	done
+	unset -f check_expected_one
+
+	if [ ${rc} -eq 0 ]
+	then
+		mptcp_lib_print_ok "[ OK ]"
+		return 0
+	fi
+
+	return "${rc}"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 473639a8009f..3f475ae44d0b 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -5,7 +5,7 @@
 # code but we accept it.
 #shellcheck disable=SC2086
 
-# Some variables are used below but indirectly, see check_expected_one()
+# Some variables are used below but indirectly
 #shellcheck disable=SC2034
 
 . "$(dirname "${0}")/mptcp_lib.sh"
@@ -233,46 +233,15 @@ make_connection()
 	fi
 }
 
-# $1: var name ; $2: prev ret
-check_expected_one()
-{
-	local var="${1}"
-	local exp="e_${var}"
-	local prev_ret="${2}"
-
-	if [ "${!var}" = "${!exp}" ]
-	then
-		return 0
-	fi
-
-	if [ "${prev_ret}" = "0" ]
-	then
-		return 2
-	fi
-
-	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \
-		"${var}" "${!exp}" "${!var}"
-	return 1
-}
-
 # $@: all var names to check
 check_expected()
 {
 	local rc=0
-	local var
 
-	for var in "${@}"
-	do
-		check_expected_one "${var}" "${rc}" || rc="${?}"
-		if [ "${rc}" -eq 2 ]
-		then
-			break
-		fi
-	done
-
-	if [ ${rc} -eq 0 ]
+	mptcp_lib_check_expected "${@}" || rc="${?}"
+	if [ "${rc}" -eq 0 ]
 	then
-		test_pass
+		mptcp_lib_result_pass "${test_name}"
 		return 0
 	elif [ "${rc}" -eq 2 ]
 	then
-- 
2.40.1

Re: [PATCH mptcp-next v5 09/12] selftests: mptcp: extract mptcp_lib_check_expected

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Extract the main part of check_expected() in userspace_pm.sh to a new
> function mptcp_lib_check_expected() in mptcp_lib.sh. It will be used
> in both mptcp_john.sh and userspace_pm.sh.
> 
> check_expected_one() is moved into mptcp_lib.sh too as a sub function
> of mptcp_lib_check_expected().

Out of curiosity: why a subfunction?

(see below)

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 48 +++++++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 39 ++-------------
>  2 files changed, 52 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index df495658f043..ed0013b47edb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -1,6 +1,9 @@
>  #! /bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  
> +# Some variables are used below but indirectly, see check_expected_one()
> +#shellcheck disable=SC2034

Please move that just above the lines or functions having these
false-positive errors to reduce the scope. Otherwise, this rule will be
applied everywhere, and maybe missing valid issues.

In other words, don't declare it here, but in mptcp_join.sh, where used.
For userspace_pm.sh, you can keep it where it is, I suppose.

> +
>  readonly KSFT_PASS=0
>  readonly KSFT_FAIL=1
>  readonly KSFT_SKIP=4
> @@ -419,3 +422,48 @@ mptcp_lib_print_test_counter() {
>  
>  	printf "%02u ${fmt}" "$((++counter))" "${msg}"
>  }
> +
> +# $@: all var names to check
> +mptcp_lib_check_expected() {
> +	# $1: var name ; $2: prev ret
> +	check_expected_one() {

You still need to prefix it with mptcp_lib_ because you could override
functions with the same name declared elsewhere: when
mptcp_lib_check_expected(), this subfunction will be created globally,
potentially overriding other ones with the same name.

> +		local var="${1}"
> +		local exp="e_${var}"
> +		local prev_ret="${2}"
> +
> +		if [ "${!var}" = "${!exp}" ]

Please keep the same code style as in the rest of this file: "then" and
"do" are on the same line as "if" / "for".

> +		then
> +			return 0
> +		fi
> +
> +		if [ "${prev_ret}" = "0" ]
> +		then
> +			return 2
> +		fi
> +
> +		printf "\tExpected value for '%s': '%s', got '%s'.\n" \
> +			"${var}" "${!exp}" "${!var}"
> +		return 1
> +	}
> +
> +	local rc=0
> +	local var
> +
> +	for var in "${@}"
> +	do
> +		check_expected_one "${var}" "${rc}" || rc="${?}"
> +		if [ "${rc}" -eq 2 ]
> +		then
> +			break
> +		fi
> +	done
> +	unset -f check_expected_one
> +
> +	if [ ${rc} -eq 0 ]
> +	then
> +		mptcp_lib_print_ok "[ OK ]"
> +		return 0
> +	fi
> +
> +	return "${rc}"
> +}
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 473639a8009f..3f475ae44d0b 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -5,7 +5,7 @@
>  # code but we accept it.
>  #shellcheck disable=SC2086
>  
> -# Some variables are used below but indirectly, see check_expected_one()
> +# Some variables are used below but indirectly

Maybe point to check_expected now?

>  #shellcheck disable=SC2034
>  
>  . "$(dirname "${0}")/mptcp_lib.sh"
> @@ -233,46 +233,15 @@ make_connection()
>  	fi
>  }
>  
> -# $1: var name ; $2: prev ret
> -check_expected_one()
> -{
> -	local var="${1}"
> -	local exp="e_${var}"
> -	local prev_ret="${2}"
> -
> -	if [ "${!var}" = "${!exp}" ]
> -	then
> -		return 0
> -	fi
> -
> -	if [ "${prev_ret}" = "0" ]
> -	then
> -		return 2
> -	fi
> -
> -	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \
> -		"${var}" "${!exp}" "${!var}"
> -	return 1
> -}
> -
>  # $@: all var names to check
>  check_expected()
>  {
>  	local rc=0
> -	local var
>  
> -	for var in "${@}"
> -	do
> -		check_expected_one "${var}" "${rc}" || rc="${?}"
> -		if [ "${rc}" -eq 2 ]
> -		then
> -			break
> -		fi
> -	done
> -
> -	if [ ${rc} -eq 0 ]
> +	mptcp_lib_check_expected "${@}" || rc="${?}"
> +	if [ "${rc}" -eq 0 ]
>  	then
> -		test_pass
> +		mptcp_lib_result_pass "${test_name}"
>  		return 0
>  	elif [ "${rc}" -eq 2 ]
>  	then

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

[PATCH mptcp-next v5 10/12] selftests: mptcp: export event macros in mptcp_lib

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

SUB_ESTABLISHED, LISTENER_CREATED, LISTENER_CLOSED, AF_INET and AF_INET6
are defined in both mptcp_join.sh and userspace_pm.sh, export all event
macros into mptcp_lib.sh. Add MPTCP_LIB_ prefix for the first three and
add readonly for the last two.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh |  23 ++--
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  11 ++
 .../selftests/net/mptcp/userspace_pm.sh       | 121 +++++++++---------
 3 files changed, 80 insertions(+), 75 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 8e8977031fa0..c9b779510528 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2789,13 +2789,6 @@ backup_tests()
 	fi
 }
 
-SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
-LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
-LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
-
-AF_INET=2
-AF_INET6=10
-
 verify_listener_events()
 {
 	local evt=$1
@@ -2809,9 +2802,9 @@ verify_listener_events()
 	local sport
 	local name
 
-	if [ $e_type = $LISTENER_CREATED ]; then
+	if [ $e_type = $MPTCP_LIB_LISTENER_CREATED ]; then
 		name="LISTENER_CREATED"
-	elif [ $e_type = $LISTENER_CLOSED ]; then
+	elif [ $e_type = $MPTCP_LIB_LISTENER_CLOSED ]; then
 		name="LISTENER_CLOSED "
 	else
 		name="$e_type"
@@ -2878,8 +2871,10 @@ add_addr_ports_tests()
 		chk_add_nr 1 1 1
 		chk_rm_nr 1 1 invert
 
-		verify_listener_events $evts_ns1 $LISTENER_CREATED $AF_INET 10.0.2.1 10100
-		verify_listener_events $evts_ns1 $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
+		verify_listener_events $evts_ns1 $MPTCP_LIB_LISTENER_CREATED \
+				       $AF_INET 10.0.2.1 10100
+		verify_listener_events $evts_ns1 $MPTCP_LIB_LISTENER_CLOSED \
+				       $AF_INET 10.0.2.1 10100
 		kill_events_pids
 	fi
 
@@ -3485,11 +3480,11 @@ userspace_tests()
 		userspace_pm_chk_get_addr "${ns1}" "10" "id 10 flags signal 10.0.2.1"
 		userspace_pm_chk_get_addr "${ns1}" "20" "id 20 flags signal 10.0.3.1"
 		userspace_pm_rm_addr $ns1 10
-		userspace_pm_rm_sf $ns1 "::ffff:10.0.2.1" $SUB_ESTABLISHED
+		userspace_pm_rm_sf $ns1 "::ffff:10.0.2.1" $MPTCP_LIB_SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns1}" \
 			"id 20 flags signal 10.0.3.1" "after rm_addr 10"
 		userspace_pm_rm_addr $ns1 20
-		userspace_pm_rm_sf $ns1 10.0.3.1 $SUB_ESTABLISHED
+		userspace_pm_rm_sf $ns1 10.0.3.1 $MPTCP_LIB_SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns1}" "" "after rm_addr 20"
 		chk_rm_nr 2 2 invert
 		chk_mptcp_info subflows 0 subflows 0
@@ -3516,7 +3511,7 @@ userspace_tests()
 			"subflow"
 		userspace_pm_chk_get_addr "${ns2}" "20" "id 20 flags subflow 10.0.3.2"
 		userspace_pm_rm_addr $ns2 20
-		userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED
+		userspace_pm_rm_sf $ns2 10.0.3.2 $MPTCP_LIB_SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns2}" \
 			"" \
 			"after rm_addr 20"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index ed0013b47edb..5feb11801469 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 # Some variables are used below but indirectly, see check_expected_one()
+# MPTCP_LIB_ANNOUNCED and AF_INET
 #shellcheck disable=SC2034
 
 readonly KSFT_PASS=0
@@ -11,6 +12,16 @@ readonly KSFT_SKIP=4
 # shellcheck disable=SC2155 # declare and assign separately
 readonly KSFT_TEST="${MPTCP_LIB_KSFT_TEST:-$(basename "${0}" .sh)}"
 
+MPTCP_LIB_ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
+MPTCP_LIB_REMOVED=7          # MPTCP_EVENT_REMOVED
+MPTCP_LIB_SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
+MPTCP_LIB_SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
+MPTCP_LIB_LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
+MPTCP_LIB_LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
+
+readonly AF_INET=2
+readonly AF_INET6=10
+
 MPTCP_LIB_SUBTESTS=()
 MPTCP_LIB_SUBTESTS_DUPLICATED=0
 
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 3f475ae44d0b..6d358b7e47e9 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -19,16 +19,6 @@ if ! mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 fi
 mptcp_lib_check_tools ip
 
-ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
-REMOVED=7          # MPTCP_EVENT_REMOVED
-SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
-SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
-LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
-LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
-
-AF_INET=2
-AF_INET6=10
-
 file=""
 server_evts=""
 client_evts=""
@@ -309,8 +299,8 @@ test_announce()
 	   ns2eth1
 	print_test "ADD_ADDR id:${client_addr_id} 10.0.2.2 (ns2) => ns1, reuse port"
 	sleep 0.5
-	verify_announce_event $server_evts $ANNOUNCED $server4_token "10.0.2.2" $client_addr_id \
-			      "$client4_port"
+	verify_announce_event $server_evts $MPTCP_LIB_ANNOUNCED $server4_token \
+			      "10.0.2.2" $client_addr_id "$client4_port"
 
 	# ADD_ADDR6 from the client to server machine reusing the subflow port
 	:>"$server_evts"
@@ -318,8 +308,8 @@ test_announce()
 	   dead:beef:2::2 token "$client6_token" id $client_addr_id dev ns2eth1
 	print_test "ADD_ADDR6 id:${client_addr_id} dead:beef:2::2 (ns2) => ns1, reuse port"
 	sleep 0.5
-	verify_announce_event "$server_evts" "$ANNOUNCED" "$server6_token" "dead:beef:2::2"\
-			      "$client_addr_id" "$client6_port" "v6"
+	verify_announce_event "$server_evts" "$MPTCP_LIB_ANNOUNCED" "$server6_token" \
+			      "dead:beef:2::2" "$client_addr_id" "$client6_port" "v6"
 
 	# ADD_ADDR from the client to server machine using a new port
 	:>"$server_evts"
@@ -328,7 +318,7 @@ test_announce()
 	   $client_addr_id dev ns2eth1 port $new4_port
 	print_test "ADD_ADDR id:${client_addr_id} 10.0.2.2 (ns2) => ns1, new port"
 	sleep 0.5
-	verify_announce_event "$server_evts" "$ANNOUNCED" "$server4_token" "10.0.2.2"\
+	verify_announce_event "$server_evts" "$MPTCP_LIB_ANNOUNCED" "$server4_token" "10.0.2.2"\
 			      "$client_addr_id" "$new4_port"
 
 	# Capture events on the network namespace running the client
@@ -339,7 +329,7 @@ test_announce()
 	   $server_addr_id dev ns1eth2
 	print_test "ADD_ADDR id:${server_addr_id} 10.0.2.1 (ns1) => ns2, reuse port"
 	sleep 0.5
-	verify_announce_event "$client_evts" "$ANNOUNCED" "$client4_token" "10.0.2.1"\
+	verify_announce_event "$client_evts" "$MPTCP_LIB_ANNOUNCED" "$client4_token" "10.0.2.1"\
 			      "$server_addr_id" "$app4_port"
 
 	# ADD_ADDR6 from the server to client machine reusing the subflow port
@@ -348,8 +338,8 @@ test_announce()
 	   $server_addr_id dev ns1eth2
 	print_test "ADD_ADDR6 id:${server_addr_id} dead:beef:2::1 (ns1) => ns2, reuse port"
 	sleep 0.5
-	verify_announce_event "$client_evts" "$ANNOUNCED" "$client6_token" "dead:beef:2::1"\
-			      "$server_addr_id" "$app6_port" "v6"
+	verify_announce_event "$client_evts" "$MPTCP_LIB_ANNOUNCED" "$client6_token" \
+			      "dead:beef:2::1" "$server_addr_id" "$app6_port" "v6"
 
 	# ADD_ADDR from the server to client machine using a new port
 	:>"$client_evts"
@@ -358,7 +348,7 @@ test_announce()
 	   $server_addr_id dev ns1eth2 port $new4_port
 	print_test "ADD_ADDR id:${server_addr_id} 10.0.2.1 (ns1) => ns2, new port"
 	sleep 0.5
-	verify_announce_event "$client_evts" "$ANNOUNCED" "$client4_token" "10.0.2.1"\
+	verify_announce_event "$client_evts" "$MPTCP_LIB_ANNOUNCED" "$client4_token" "10.0.2.1"\
 			      "$server_addr_id" "$new4_port"
 }
 
@@ -419,7 +409,7 @@ test_remove()
 	   $client_addr_id
 	print_test "RM_ADDR id:${client_addr_id} ns2 => ns1"
 	sleep 0.5
-	verify_remove_event "$server_evts" "$REMOVED" "$server4_token" "$client_addr_id"
+	verify_remove_event "$server_evts" "$MPTCP_LIB_REMOVED" "$server4_token" "$client_addr_id"
 
 	# RM_ADDR from the client to server machine
 	:>"$server_evts"
@@ -428,7 +418,7 @@ test_remove()
 	   $client_addr_id
 	print_test "RM_ADDR id:${client_addr_id} ns2 => ns1"
 	sleep 0.5
-	verify_remove_event "$server_evts" "$REMOVED" "$server4_token" "$client_addr_id"
+	verify_remove_event "$server_evts" "$MPTCP_LIB_REMOVED" "$server4_token" "$client_addr_id"
 
 	# RM_ADDR6 from the client to server machine
 	:>"$server_evts"
@@ -436,7 +426,7 @@ test_remove()
 	   $client_addr_id
 	print_test "RM_ADDR6 id:${client_addr_id} ns2 => ns1"
 	sleep 0.5
-	verify_remove_event "$server_evts" "$REMOVED" "$server6_token" "$client_addr_id"
+	verify_remove_event "$server_evts" "$MPTCP_LIB_REMOVED" "$server6_token" "$client_addr_id"
 
 	# Capture events on the network namespace running the client
 	:>"$client_evts"
@@ -446,7 +436,7 @@ test_remove()
 	   $server_addr_id
 	print_test "RM_ADDR id:${server_addr_id} ns1 => ns2"
 	sleep 0.5
-	verify_remove_event "$client_evts" "$REMOVED" "$client4_token" "$server_addr_id"
+	verify_remove_event "$client_evts" "$MPTCP_LIB_REMOVED" "$client4_token" "$server_addr_id"
 
 	# RM_ADDR from the server to client machine
 	:>"$client_evts"
@@ -455,7 +445,7 @@ test_remove()
 	   $server_addr_id
 	print_test "RM_ADDR id:${server_addr_id} ns1 => ns2"
 	sleep 0.5
-	verify_remove_event "$client_evts" "$REMOVED" "$client4_token" "$server_addr_id"
+	verify_remove_event "$client_evts" "$MPTCP_LIB_REMOVED" "$client4_token" "$server_addr_id"
 
 	# RM_ADDR6 from the server to client machine
 	:>"$client_evts"
@@ -463,7 +453,7 @@ test_remove()
 	   $server_addr_id
 	print_test "RM_ADDR6 id:${server_addr_id} ns1 => ns2"
 	sleep 0.5
-	verify_remove_event "$client_evts" "$REMOVED" "$client6_token" "$server_addr_id"
+	verify_remove_event "$client_evts" "$MPTCP_LIB_REMOVED" "$client6_token" "$server_addr_id"
 }
 
 verify_subflow_events()
@@ -492,7 +482,7 @@ verify_subflow_events()
 
 	info="${e_saddr} (${e_from}) => ${e_daddr}:${e_dport} (${e_to})"
 
-	if [ "$e_type" = "$SUB_ESTABLISHED" ]
+	if [ "$e_type" = "$MPTCP_LIB_SUB_ESTABLISHED" ]
 	then
 		if [ "$e_family" = "$AF_INET6" ]
 		then
@@ -549,22 +539,24 @@ test_subflows()
 	ip netns exec "$ns1" ./pm_nl_ctl csf lip 10.0.2.1 lid 23 rip 10.0.2.2\
 	   rport "$client4_port" token "$server4_token"
 	sleep 0.5
-	verify_subflow_events $server_evts $SUB_ESTABLISHED $server4_token $AF_INET "10.0.2.1" \
-			      "10.0.2.2" "$client4_port" "23" "$client_addr_id" "ns1" "ns2"
+	verify_subflow_events $server_evts $MPTCP_LIB_SUB_ESTABLISHED $server4_token \
+			      $AF_INET "10.0.2.1" "10.0.2.2" "$client4_port" "23" \
+			      "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
 	local sport
-	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from server to client machine
 	:>"$server_evts"
 	ip netns exec "$ns1" ./pm_nl_ctl dsf lip 10.0.2.1 lport "$sport" rip 10.0.2.2 rport\
 	   "$client4_port" token "$server4_token"
 	sleep 0.5
-	verify_subflow_events "$server_evts" "$SUB_CLOSED" "$server4_token" "$AF_INET" "10.0.2.1"\
-			      "10.0.2.2" "$client4_port" "23" "$client_addr_id" "ns1" "ns2"
+	verify_subflow_events "$server_evts" "$MPTCP_LIB_SUB_CLOSED" "$server4_token" \
+			      "$AF_INET" "10.0.2.1" "10.0.2.2" "$client4_port" "23" \
+			      "$client_addr_id" "ns1" "ns2"
 
 	# RM_ADDR from client to server machine
 	ip netns exec "$ns2" ./pm_nl_ctl rem id $client_addr_id token\
@@ -587,21 +579,21 @@ test_subflows()
 	ip netns exec "$ns1" ./pm_nl_ctl csf lip dead:beef:2::1 lid 23 rip\
 	   dead:beef:2::2 rport "$client6_port" token "$server6_token"
 	sleep 0.5
-	verify_subflow_events "$server_evts" "$SUB_ESTABLISHED" "$server6_token" "$AF_INET6"\
-			      "dead:beef:2::1" "dead:beef:2::2" "$client6_port" "23"\
-			      "$client_addr_id" "ns1" "ns2"
+	verify_subflow_events "$server_evts" "$MPTCP_LIB_SUB_ESTABLISHED" "$server6_token" \
+			      "$AF_INET6" "dead:beef:2::1" "dead:beef:2::2" "$client6_port" \
+			      "23" "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW6 from server to client machine
 	:>"$server_evts"
 	ip netns exec "$ns1" ./pm_nl_ctl dsf lip dead:beef:2::1 lport "$sport" rip\
 	   dead:beef:2::2 rport "$client6_port" token "$server6_token"
 	sleep 0.5
-	verify_subflow_events "$server_evts" "$SUB_CLOSED" "$server6_token" "$AF_INET6"\
+	verify_subflow_events "$server_evts" "$MPTCP_LIB_SUB_CLOSED" "$server6_token" "$AF_INET6"\
 			      "dead:beef:2::1" "dead:beef:2::2" "$client6_port" "23"\
 			      "$client_addr_id" "ns1" "ns2"
 
@@ -626,22 +618,23 @@ test_subflows()
 	ip netns exec "$ns1" ./pm_nl_ctl csf lip 10.0.2.1 lid 23 rip 10.0.2.2 rport\
 	   $new4_port token "$server4_token"
 	sleep 0.5
-	verify_subflow_events "$server_evts" "$SUB_ESTABLISHED" "$server4_token" "$AF_INET"\
-			      "10.0.2.1" "10.0.2.2" "$new4_port" "23"\
+	verify_subflow_events "$server_evts" "$MPTCP_LIB_SUB_ESTABLISHED" "$server4_token" \
+			      "$AF_INET" "10.0.2.1" "10.0.2.2" "$new4_port" "23" \
 			      "$client_addr_id" "ns1" "ns2"
 
 	# Delete the listener from the client ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$server_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from server to client machine
 	:>"$server_evts"
 	ip netns exec "$ns1" ./pm_nl_ctl dsf lip 10.0.2.1 lport "$sport" rip 10.0.2.2 rport\
 	   $new4_port token "$server4_token"
 	sleep 0.5
-	verify_subflow_events "$server_evts" "$SUB_CLOSED" "$server4_token" "$AF_INET" "10.0.2.1"\
-			      "10.0.2.2" "$new4_port" "23" "$client_addr_id" "ns1" "ns2"
+	verify_subflow_events "$server_evts" "$MPTCP_LIB_SUB_CLOSED" "$server4_token" \
+			      "$AF_INET" "10.0.2.1" "10.0.2.2" "$new4_port" "23" \
+			      "$client_addr_id" "ns1" "ns2"
 
 	# RM_ADDR from client to server machine
 	ip netns exec "$ns2" ./pm_nl_ctl rem id $client_addr_id token\
@@ -665,21 +658,22 @@ test_subflows()
 	ip netns exec "$ns2" ./pm_nl_ctl csf lip 10.0.2.2 lid 23 rip 10.0.2.1 rport\
 	   $app4_port token "$client4_token"
 	sleep 0.5
-	verify_subflow_events $client_evts $SUB_ESTABLISHED $client4_token $AF_INET "10.0.2.2"\
-			      "10.0.2.1" "$app4_port" "23" "$server_addr_id" "ns2" "ns1"
+	verify_subflow_events $client_evts $MPTCP_LIB_SUB_ESTABLISHED $client4_token $AF_INET \
+			      "10.0.2.2" "10.0.2.1" "$app4_port" "23" "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $app4_port token "$client4_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client4_token" "$AF_INET" "10.0.2.2"\
-			      "10.0.2.1" "$app4_port" "23" "$server_addr_id" "ns2" "ns1"
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_CLOSED" "$client4_token" \
+			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$app4_port" "23" \
+			      "$server_addr_id" "ns2" "ns1"
 
 	# RM_ADDR from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
@@ -702,7 +696,7 @@ test_subflows()
 	ip netns exec "$ns2" ./pm_nl_ctl csf lip dead:beef:2::2 lid 23 rip\
 	   dead:beef:2::1 rport $app6_port token "$client6_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_ESTABLISHED" "$client6_token"\
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_ESTABLISHED" "$client6_token"\
 			      "$AF_INET6" "dead:beef:2::2"\
 			      "dead:beef:2::1" "$app6_port" "23"\
 			      "$server_addr_id" "ns2" "ns1"
@@ -710,15 +704,16 @@ test_subflows()
 	# Delete the listener from the server ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW6 from client to server machine
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip dead:beef:2::2 lport "$sport" rip\
 	   dead:beef:2::1 rport $app6_port token "$client6_token"
 	sleep 0.5
-	verify_subflow_events $client_evts $SUB_CLOSED $client6_token $AF_INET6 "dead:beef:2::2"\
-			      "dead:beef:2::1" "$app6_port" "23" "$server_addr_id" "ns2" "ns1"
+	verify_subflow_events $client_evts $MPTCP_LIB_SUB_CLOSED $client6_token \
+			      $AF_INET6 "dead:beef:2::2" "dead:beef:2::1" "$app6_port" \
+			      "23" "$server_addr_id" "ns2" "ns1"
 
 	# RM_ADDR6 from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
@@ -741,21 +736,23 @@ test_subflows()
 	ip netns exec "$ns2" ./pm_nl_ctl csf lip 10.0.2.2 lid 23 rip 10.0.2.1 rport\
 	   $new4_port token "$client4_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_ESTABLISHED" "$client4_token" "$AF_INET"\
-			      "10.0.2.2" "10.0.2.1" "$new4_port" "23" "$server_addr_id" "ns2" "ns1"
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_ESTABLISHED" "$client4_token" \
+			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$new4_port" "23" \
+			      "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $new4_port token "$client4_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client4_token" "$AF_INET" "10.0.2.2"\
-			      "10.0.2.1" "$new4_port" "23" "$server_addr_id" "ns2" "ns1"
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_CLOSED" "$client4_token" \
+			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$new4_port" "23" \
+			      "$server_addr_id" "ns2" "ns1"
 
 	# RM_ADDR from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
@@ -778,7 +775,7 @@ test_subflows_v4_v6_mix()
 	   $server_addr_id dev ns1eth2
 	print_test "ADD_ADDR4 id:${server_addr_id} 10.0.2.1 (ns1) => ns2, reuse port"
 	sleep 0.5
-	verify_announce_event "$client_evts" "$ANNOUNCED" "$client6_token" "10.0.2.1"\
+	verify_announce_event "$client_evts" "$MPTCP_LIB_ANNOUNCED" "$client6_token" "10.0.2.1"\
 			      "$server_addr_id" "$app6_port"
 
 	# CREATE_SUBFLOW from client to server machine
@@ -786,21 +783,21 @@ test_subflows_v4_v6_mix()
 	ip netns exec "$ns2" ./pm_nl_ctl csf lip 10.0.2.2 lid 23 rip 10.0.2.1 rport\
 	   $app6_port token "$client6_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_ESTABLISHED" "$client6_token"\
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_ESTABLISHED" "$client6_token"\
 			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$app6_port" "23"\
 			      "$server_addr_id" "ns2" "ns1"
 
 	# Delete the listener from the server ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $SUB_ESTABLISHED)
+	sport=$(mptcp_lib_evts_get_info sport "$client_evts" $MPTCP_LIB_SUB_ESTABLISHED)
 
 	# DESTROY_SUBFLOW from client to server machine
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $app6_port token "$client6_token"
 	sleep 0.5
-	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client6_token" \
+	verify_subflow_events "$client_evts" "$MPTCP_LIB_SUB_CLOSED" "$client6_token" \
 			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$app6_port" "23"\
 			      "$server_addr_id" "ns2" "ns1"
 
@@ -892,7 +889,8 @@ test_listener()
 	local listener_pid=$!
 
 	sleep 0.5
-	verify_listener_events $client_evts $LISTENER_CREATED $AF_INET 10.0.2.2 $client4_port
+	verify_listener_events $client_evts $MPTCP_LIB_LISTENER_CREATED \
+			       $AF_INET 10.0.2.2 $client4_port
 
 	# ADD_ADDR from client to server machine reusing the subflow port
 	ip netns exec $ns2 ./pm_nl_ctl ann 10.0.2.2 token $client4_token id\
@@ -908,7 +906,8 @@ test_listener()
 	mptcp_lib_kill_wait $listener_pid
 
 	sleep 0.5
-	verify_listener_events $client_evts $LISTENER_CLOSED $AF_INET 10.0.2.2 $client4_port
+	verify_listener_events $client_evts $MPTCP_LIB_LISTENER_CLOSED \
+			       $AF_INET 10.0.2.2 $client4_port
 }
 
 print_title "Make connections"
-- 
2.40.1

Re: [PATCH mptcp-next v5 10/12] selftests: mptcp: export event macros in mptcp_lib

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> SUB_ESTABLISHED, LISTENER_CREATED, LISTENER_CLOSED, AF_INET and AF_INET6
> are defined in both mptcp_join.sh and userspace_pm.sh, export all event
> macros into mptcp_lib.sh. Add MPTCP_LIB_ prefix for the first three and
> add readonly for the last two.

Why not doing that (prefix + readonly) for both?

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index ed0013b47edb..5feb11801469 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -2,6 +2,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  # Some variables are used below but indirectly, see check_expected_one()
> +# MPTCP_LIB_ANNOUNCED and AF_INET

Better to "export" them then instead

>  #shellcheck disable=SC2034
>  
>  readonly KSFT_PASS=0
> @@ -11,6 +12,16 @@ readonly KSFT_SKIP=4
>  # shellcheck disable=SC2155 # declare and assign separately
>  readonly KSFT_TEST="${MPTCP_LIB_KSFT_TEST:-$(basename "${0}" .sh)}"
>  
> +MPTCP_LIB_ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
> +MPTCP_LIB_REMOVED=7          # MPTCP_EVENT_REMOVED
> +MPTCP_LIB_SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> +MPTCP_LIB_SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
> +MPTCP_LIB_LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> +MPTCP_LIB_LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED

Please use the "readonly" attribute, and align the "#".

Also, probably clearer to add '_EVENT', e.g.

  export MPTCP_LIB_EVENT_ANNOUNCED=6

> +
> +readonly AF_INET=2
> +readonly AF_INET6=10

Prefix with 'MPTCP_LIB_' + export?

> +
>  MPTCP_LIB_SUBTESTS=()
>  MPTCP_LIB_SUBTESTS_DUPLICATED=0
>  
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 3f475ae44d0b..6d358b7e47e9 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -19,16 +19,6 @@ if ! mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
>  fi
>  mptcp_lib_check_tools ip
>  
> -ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
> -REMOVED=7          # MPTCP_EVENT_REMOVED
> -SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> -SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10

Instead of modifying hundreds of lines, probably best to do:

  ANNOUNCED=${MPTCP_LIB_EVENT_ANNOUNCED}
  (...)

Same in mptcp_join.sh? (Maybe that's fine, there are just a few there)

(...)

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

[PATCH mptcp-next v5 11/12] selftests: mptcp: add mptcp_lib_verify_listener_events

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To avoid duplicated code in different MPTCP selftests, we can add and use
helpers defined in mptcp_lib.sh.

The helper verify_listener_events() is defined both in mptcp_join.sh and
userspace_pm.sh, export it into mptcp_lib.sh and rename it with mptcp_lib_
prefix. Use this new helper in both scripts.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 25 +++------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 25 +++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 31 +++++--------------
 3 files changed, 38 insertions(+), 43 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c9b779510528..fa7d07c51973 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2791,16 +2791,12 @@ backup_tests()
 
 verify_listener_events()
 {
-	local evt=$1
 	local e_type=$2
 	local e_family=$3
 	local e_saddr=$4
 	local e_sport=$5
-	local type
-	local family
-	local saddr
-	local sport
 	local name
+	local rc=0
 
 	if [ $e_type = $MPTCP_LIB_LISTENER_CREATED ]; then
 		name="LISTENER_CREATED"
@@ -2817,23 +2813,12 @@ verify_listener_events()
 		return
 	fi
 
-	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
-	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
-	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
-	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
-	else
-		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
-	fi
-
-	if [ $type ] && [ $type = $e_type ] &&
-	   [ $family ] && [ $family = $e_family ] &&
-	   [ $saddr ] && [ $saddr = $e_saddr ] &&
-	   [ $sport ] && [ $sport = $e_sport ]; then
-		print_ok
+	mptcp_lib_verify_listener_events "${@}" || rc="${?}"
+	if [ "${rc}" -eq 0 ]
+	then
 		return 0
 	fi
-	fail_test "$e_type:$type $e_family:$family $e_saddr:$saddr $e_sport:$sport"
+	fail_test "$e_type $e_family $e_saddr $e_sport"
 }
 
 add_addr_ports_tests()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 5feb11801469..a6a520a9a592 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -478,3 +478,28 @@ mptcp_lib_check_expected() {
 
 	return "${rc}"
 }
+
+mptcp_lib_verify_listener_events() {
+	local evt=$1
+	local e_type=$2
+	local e_family=$3
+	local e_saddr=$4
+	local e_sport=$5
+	local type
+	local family
+	local saddr
+	local sport
+	local rc=0
+
+	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
+	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
+	if [ "$family" ] && [ "$family" = $AF_INET6 ]; then
+		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
+	else
+		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
+	fi
+	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
+
+	mptcp_lib_check_expected "type" "family" "saddr" "sport" || rc="${?}"
+	return "${rc}"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 6d358b7e47e9..6fe2a4e1704f 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -842,32 +842,15 @@ test_prio()
 
 verify_listener_events()
 {
-	local evt=$1
-	local e_type=$2
-	local e_family=$3
-	local e_saddr=$4
-	local e_sport=$5
-	local type
-	local family
-	local saddr
-	local sport
-
-	if [ $e_type = $LISTENER_CREATED ]; then
-		print_test "CREATE_LISTENER $e_saddr:$e_sport"
-	elif [ $e_type = $LISTENER_CLOSED ]; then
-		print_test "CLOSE_LISTENER $e_saddr:$e_sport"
-	fi
+	local rc=0
 
-	type=$(mptcp_lib_evts_get_info type $evt $e_type)
-	family=$(mptcp_lib_evts_get_info family $evt $e_type)
-	sport=$(mptcp_lib_evts_get_info sport $evt $e_type)
-	if [ $family ] && [ $family = $AF_INET6 ]; then
-		saddr=$(mptcp_lib_evts_get_info saddr6 $evt $e_type)
+	mptcp_lib_verify_listener_events "${@}" || rc="${?}"
+	if [ "${rc}" -eq 0 ]
+	then
+		mptcp_lib_result_pass "${test_name}"
 	else
-		saddr=$(mptcp_lib_evts_get_info saddr4 $evt $e_type)
+		test_fail
 	fi
-
-	check_expected "type" "family" "saddr" "sport"
 }
 
 test_listener()
@@ -883,6 +866,7 @@ test_listener()
 	# Capture events on the network namespace running the client
 	:>$client_evts
 
+	print_test "Listener event LISTENER_CREATED 10.0.2.2:$client4_port"
 	# Attempt to add a listener at 10.0.2.2:<subflow-port>
 	ip netns exec $ns2 ./pm_nl_ctl listen 10.0.2.2\
 		$client4_port &
@@ -902,6 +886,7 @@ test_listener()
 		rport $client4_port token $server4_token
 	sleep 0.5
 
+	print_test "Listener event LISTENER_CLOSED 10.0.2.2:$client4_port"
 	# Delete the listener from the client ns, if one was created
 	mptcp_lib_kill_wait $listener_pid
 
-- 
2.40.1

Re: [PATCH mptcp-next v5 11/12] selftests: mptcp: add mptcp_lib_verify_listener_events

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To avoid duplicated code in different MPTCP selftests, we can add and use
> helpers defined in mptcp_lib.sh.
> 
> The helper verify_listener_events() is defined both in mptcp_join.sh and
> userspace_pm.sh, export it into mptcp_lib.sh and rename it with mptcp_lib_
> prefix. Use this new helper in both scripts.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 25 +++------------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 25 +++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 31 +++++--------------
>  3 files changed, 38 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c9b779510528..fa7d07c51973 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2791,16 +2791,12 @@ backup_tests()
>  
>  verify_listener_events()
>  {
> -	local evt=$1
>  	local e_type=$2
>  	local e_family=$3
>  	local e_saddr=$4
>  	local e_sport=$5
> -	local type
> -	local family
> -	local saddr
> -	local sport
>  	local name
> +	local rc=0
>  
>  	if [ $e_type = $MPTCP_LIB_LISTENER_CREATED ]; then
>  		name="LISTENER_CREATED"
> @@ -2817,23 +2813,12 @@ verify_listener_events()
>  		return
>  	fi
>  
> -	type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
> -	family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
> -	sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
> -	if [ $family ] && [ $family = $AF_INET6 ]; then
> -		saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
> -	else
> -		saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
> -	fi
> -
> -	if [ $type ] && [ $type = $e_type ] &&
> -	   [ $family ] && [ $family = $e_family ] &&
> -	   [ $saddr ] && [ $saddr = $e_saddr ] &&
> -	   [ $sport ] && [ $sport = $e_sport ]; then
> -		print_ok

Should you not keep 'print_ok' here?

> +	mptcp_lib_verify_listener_events "${@}" || rc="${?}"
> +	if [ "${rc}" -eq 0 ]
> +	then

Keep the same style as in the rest of the file: 'then' at the previous line.

>  		return 0
>  	fi
> -	fail_test "$e_type:$type $e_family:$family $e_saddr:$saddr $e_sport:$sport"
> +	fail_test "$e_type $e_family $e_saddr $e_sport"
>  }
>  
>  add_addr_ports_tests()

(...)

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

[PATCH mptcp-next v5 12/12] selftests: mptcp: use KSFT_SKIP/KSFT_PASS/KSFT_FAIL

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch uses the public var KSFT_SKIP in mptcp_lib.sh instead of
ksft_skip, and drop 'ksft_skip=4' in mptcp_join.sh.

Use KSFT_PASS and KSFT_FAIL macros instead of 0 and 1 after 'exit '
and 'ret=' in all scripts:

        exit 0 -> exit ${KSFT_PASS}
        exit 1 -> exit ${KSFT_FAIL}
         ret=0 ->  ret=${KSFT_PASS}
         ret=1 ->  ret=${KSFT_FAIL}

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/net/mptcp/mptcp_connect.sh       | 18 +++++++++---------
 .../testing/selftests/net/mptcp/mptcp_join.sh  | 13 ++++++-------
 .../selftests/net/mptcp/mptcp_sockopt.sh       |  4 ++--
 .../testing/selftests/net/mptcp/pm_netlink.sh  |  4 ++--
 .../selftests/net/mptcp/simult_flows.sh        |  4 ++--
 .../selftests/net/mptcp/userspace_pm.sh        |  4 ++--
 6 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 00bbe451e50d..f54d80a1d0e4 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -65,14 +65,14 @@ while getopts "$optstring" option;do
 	case "$option" in
 	"h")
 		usage $0
-		exit 0
+		exit ${KSFT_PASS}
 		;;
 	"d")
 		if [ $OPTARG -ge 0 ];then
 			tc_delay="$OPTARG"
 		else
 			echo "-d requires numeric argument, got \"$OPTARG\"" 1>&2
-			exit 1
+			exit ${KSFT_FAIL}
 		fi
 		;;
 	"e")
@@ -96,7 +96,7 @@ while getopts "$optstring" option;do
 			sndbuf="$OPTARG"
 		else
 			echo "-S requires numeric argument, got \"$OPTARG\"" 1>&2
-			exit 1
+			exit ${KSFT_FAIL}
 		fi
 		;;
 	"R")
@@ -104,7 +104,7 @@ while getopts "$optstring" option;do
 			rcvbuf="$OPTARG"
 		else
 			echo "-R requires numeric argument, got \"$OPTARG\"" 1>&2
-			exit 1
+			exit ${KSFT_FAIL}
 		fi
 		;;
 	"m")
@@ -121,7 +121,7 @@ while getopts "$optstring" option;do
 		;;
 	"?")
 		usage $0
-		exit 1
+		exit ${KSFT_FAIL}
 		;;
 	esac
 done
@@ -260,7 +260,7 @@ check_mptcp_disabled()
 			"net.mptcp.enabled sysctl is not 1 by default"
 		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
-		ret=1
+		ret=${KSFT_FAIL}
 		return 1
 	fi
 	ip netns exec ${disabled_ns} sysctl -q net.mptcp.enabled=0
@@ -275,7 +275,7 @@ check_mptcp_disabled()
 			"New MPTCP socket cannot be blocked via sysctl"
 		mptcp_lib_print_err "\t\t\t\t   [FAIL]"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
-		ret=1
+		ret=${KSFT_FAIL}
 		return 1
 	fi
 
@@ -303,7 +303,7 @@ do_ping()
 
 	if [ $rc -ne 0 ] ; then
 		echo "$listener_ns -> $connect_addr connectivity [FAIL]" 1>&2
-		ret=1
+		ret=${KSFT_FAIL}
 
 		return 1
 	fi
@@ -823,7 +823,7 @@ log_if_error()
 		echo "FAIL: ${msg}" 1>&2
 
 		final_ret=${ret}
-		ret=0
+		ret=${KSFT_PASS}
 
 		return ${final_ret}
 	fi
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fa7d07c51973..ba6d34bb2d0b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -25,7 +25,6 @@ err=""
 capout=""
 ns1=""
 ns2=""
-ksft_skip=4
 iptables="iptables"
 ip6tables="ip6tables"
 timeout_poll=30
@@ -398,15 +397,15 @@ setup_fail_rules()
 		-p tcp \
 		-m length --length 150:9999 \
 		-m statistic --mode nth --packet 1 --every 99999 \
-		-j MARK --set-mark 42 || return ${ksft_skip}
+		-j MARK --set-mark 42 || return ${KSFT_SKIP}
 
-	tc -n $ns2 qdisc add dev ns2eth$i clsact || return ${ksft_skip}
+	tc -n $ns2 qdisc add dev ns2eth$i clsact || return ${KSFT_SKIP}
 	tc -n $ns2 filter add dev ns2eth$i egress \
 		protocol ip prio 1000 \
 		handle 42 fw \
 		action pedit munge offset 148 u8 invert \
 		pipe csum tcp \
-		index 100 || return ${ksft_skip}
+		index 100 || return ${KSFT_SKIP}
 }
 
 reset_with_fail()
@@ -420,7 +419,7 @@ reset_with_fail()
 	local rc=0
 	setup_fail_rules "${@}" || rc=$?
 
-	if [ ${rc} -eq ${ksft_skip} ]; then
+	if [ ${rc} -eq ${KSFT_SKIP} ]; then
 		mark_as_skipped "unable to set the 'fail' rules"
 		return 1
 	fi
@@ -456,7 +455,7 @@ reset_with_tcp_filter()
 # $1: err msg
 fail_test()
 {
-	ret=1
+	ret=${KSFT_FAIL}
 
 	print_fail "${@}"
 
@@ -3639,7 +3638,7 @@ usage()
 {
 	if [ -n "${1}" ]; then
 		echo "${1}"
-		ret=1
+		ret=${KSFT_FAIL}
 	fi
 
 	echo "mptcp_join usage:"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index a2a5049f22bb..59ae6c649904 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -106,7 +106,7 @@ check_mark()
 	for v in $values; do
 		if [ $v -ne 0 ]; then
 			echo "FAIL: got $tables $values in ns $ns , not 0 - not all expected packets marked" 1>&2
-			ret=1
+			ret=${KSFT_FAIL}
 			return 1
 		fi
 	done
@@ -175,7 +175,7 @@ do_transfer()
 		mptcp_lib_print_err "[FAIL]"
 		mptcp_lib_result_fail "transfer ${ip}"
 
-		ret=1
+		ret=${KSFT_FAIL}
 		return 1
 	fi
 	mptcp_lib_print_ok "[ OK ]"
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 87ed60ed4112..82dbdec930e6 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -21,11 +21,11 @@ while getopts "$optstring" option;do
 	case "$option" in
 	"h")
 		usage $0
-		exit 0
+		exit ${KSFT_PASS}
 		;;
 	"?")
 		usage $0
-		exit 1
+		exit ${KSFT_FAIL}
 		;;
 	esac
 done
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index eb2eaa48035f..ff255e4a2ed3 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -262,7 +262,7 @@ while getopts "bcdh" option;do
 	case "$option" in
 	"h")
 		usage $0
-		exit 0
+		exit ${KSFT_PASS}
 		;;
 	"b")
 		bail=1
@@ -275,7 +275,7 @@ while getopts "bcdh" option;do
 		;;
 	"?")
 		usage $0
-		exit 1
+		exit ${KSFT_FAIL}
 		;;
 	esac
 done
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 6fe2a4e1704f..1124ebab3ace 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -79,7 +79,7 @@ test_skip()
 test_fail()
 {
 	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
-	ret=1
+	ret=${KSFT_FAIL}
 
 	if [ -n "${1}" ]; then
 		_printf "\t%s\n" "${1}"
@@ -204,7 +204,7 @@ make_connection()
 	else
 		test_fail "Expected tokens (c:${client_token} - s:${server_token}) and server (c:${client_serverside} - s:${server_serverside})"
 		mptcp_lib_result_print_all_tap
-		exit 1
+		exit ${KSFT_FAIL}
 	fi
 
 	if [ "$is_v6" = "v6" ]
-- 
2.40.1

Re: selftests: mptcp: use KSFT_SKIP/KSFT_PASS/KSFT_FAIL: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8046892754

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/44959d827650


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v5 00/12] add helpers and vars in mptcp_lib.sh, part 3

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v5:
>  - don't use mptcp_lib_print_test_counter in mptcp_join.sh, it breaks
>    skip_test().
> 
> v4:
>  - set test_cnt to 0, use ++counter in mptcp_lib_print_test_counter() to
>    fix the following mismatched test counters:
> 
> # 012 userspace pm server fullmesh
> #       syn                                 [ OK ]
> #       synack                              [ OK ]
> #       ack                                 [ OK ]
> #       add                                 [ OK ]
> #       echo                                [FAIL] got 1 ADD_ADDR echo[s] expected 2
> # Server ns stats
> # TcpPassiveOpens                 5                  0.0
> # TcpInSegs                       25                 0.0
> ... ...
> #
> # 1 failure(s) has(ve) been detected:
> #       - 13: userspace pm server fullmesh
> 
> v3:
>  - fix shellcheck errors in v2
> 
> v2:
>  - fix shellcheck errors in v1
>  - print test results with counters

Thank you for the new version fixing shellcheck errors.

Note that it is not easy to review this series with all the
modifications everywhere. Hopefully the new modifications will be
helpful for the maintenance and to spot issues.

Did you check that the messages in case of errors were all the same?

Please see the comments on the individual patches.

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