diff mbox series

[mptcp-next,v2,3/4] selftests: mptcp: use cleanup_all_ns helper in lib.sh

Message ID d341c904ef486b7e0039c3812ddd659ef7791f9f.1716451525.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series use helpers in lib.sh and net_helpers.sh | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 84 lines checked
matttbe/shellcheck success No ShellCheck issues
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang May 23, 2024, 8:08 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch uses cleanup_all_ns() helper defined in lib.sh instead of
all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
mptcp helper in mptcp_lib.sh.

In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
cleanup(), together with "ns1 - ns4".

In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial() directly,
each existing namespace will delete automaticly in setup_ns(), only
adding cleanup_all_ns in cleanup() is enough.

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

Comments

Geliang Tang May 23, 2024, 8:39 a.m. UTC | #1 
On Thu, 2024-05-23 at 16:08 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch uses cleanup_all_ns() helper defined in lib.sh instead of
> all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
> mptcp helper in mptcp_lib.sh.
> 
> In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
> directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
> cleanup(), together with "ns1 - ns4".
> 
> In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial()
> directly,
> each existing namespace will delete automaticly in setup_ns(), only

Sorry, should be "automatically", CI complain about it.

-Geliang

> adding cleanup_all_ns in cleanup() is enough.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh          | 2 +-
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +--
>  tools/testing/selftests/net/mptcp/mptcp_join.sh    | 3 +--
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh     | 8 --------
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 2 +-
>  tools/testing/selftests/net/mptcp/pm_netlink.sh    | 2 +-
>  tools/testing/selftests/net/mptcp/simult_flows.sh  | 2 +-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh  | 2 +-
>  8 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> b/tools/testing/selftests/net/mptcp/diag.sh
> index eec1f04d231f..9e19e3e8d833 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -33,7 +33,7 @@ cleanup()
>  {
>  	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -
> SIGKILL &>/dev/null
>  
> -	mptcp_lib_ns_exit "${ns1}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index b77fb7065bfb..4e2c5dd0de3c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -142,7 +142,7 @@ cleanup()
>  	rm -f "$sin" "$sout"
>  	rm -f "$capout"
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> @@ -271,7 +271,6 @@ check_mptcp_disabled()
>  	local err=0
>  	LC_ALL=C ip netns exec ${disabled_ns} ./mptcp_connect -p
> 10000 -s MPTCP 127.0.0.1 < "$cin" 2>&1 | \
>  		grep -q "^socket: Protocol not available$" && err=1
> -	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
>  		mptcp_lib_pr_fail "New MPTCP socket cannot be
> blocked via sysctl"
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fefa9173bdaa..87a518b8c19f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -132,8 +132,6 @@ init_shapers()
>  cleanup_partial()
>  {
>  	rm -f "$capout"
> -
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}"
>  }
>  
>  init() {
> @@ -166,6 +164,7 @@ cleanup()
>  	rm -rf $evts_ns1 $evts_ns2
>  	rm -f "$err"
>  	cleanup_partial
> +	cleanup_all_ns
>  }
>  
>  print_check()
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 59eb77e7813d..bd7d78e4aa83 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
>  	done
>  }
>  
> -mptcp_lib_ns_exit() {
> -	local netns
> -	for netns in "${@}"; do
> -		ip netns del "${netns}"
> -		rm -f /tmp/"${netns}".{nstat,out}
> -	done
> -}
> -
>  mptcp_lib_events() {
>  	local ns="${1}"
>  	local evts="${2}"
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 68899a303a1a..e1026b028739 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -98,7 +98,7 @@ init()
>  #shellcheck disable=SC2317
>  cleanup()
>  {
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
> +	cleanup_all_ns
>  	rm -f "$cin" "$cout"
>  	rm -f "$sin" "$sout"
>  }
> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 2757378b1b13..5b4d83c2e280 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -36,7 +36,7 @@ err=$(mktemp)
>  cleanup()
>  {
>  	rm -f "${err}"
> -	mptcp_lib_ns_exit "${ns1}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh
> b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index d0b39c2e38a3..6eddb3bba2e8 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -42,7 +42,7 @@ cleanup()
>  	rm -f "$large" "$small"
>  	rm -f "$capout"
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 9e2981f2d7f5..0c089e7f5f0a 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -107,7 +107,7 @@ cleanup()
>  		mptcp_lib_kill_wait $pid
>  	done
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}"
> +	cleanup_all_ns
>  
>  	rm -rf $file $client_evts $server_evts
>
Matthieu Baerts (NGI0) May 23, 2024, 9:22 a.m. UTC | #2 
Hi Geliang,

On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch uses cleanup_all_ns() helper defined in lib.sh instead of
> all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
> mptcp helper in mptcp_lib.sh.
> 
> In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
> directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
> cleanup(), together with "ns1 - ns4".

Mmh, I think it is better to clear resources when we don't need them,
than cleaning them all at the end. New code doing that might be OK, but
here switching to that while not really gaining anything new looks less
good.

> In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial() directly,
> each existing namespace will delete automaticly in setup_ns(), only
> adding cleanup_all_ns in cleanup() is enough.

Mmh, same here: I think it is better to clean at the end of a test, than
at the beginning of a new one: if there are issues to delete it, it will
be clearer it is due to the previous test, not the new one.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 59eb77e7813d..bd7d78e4aa83 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
>  	done
>  }
>  
> -mptcp_lib_ns_exit() {
> -	local netns
> -	for netns in "${@}"; do
> -		ip netns del "${netns}"
> -		rm -f /tmp/"${netns}".{nstat,out}

These files are no longer removed. It is then no longer just a "simple"
refactoring. Such behaviour change should be mentioned in the commit
message, explaining why it is OK to do that.

I would suggest to keep mptcp_lib_ns_exit helper(), and simply call
'cleanup_ns "${@}"' here instead. If you do that, there is no need to
modify the other selftests, and you can squash this in patch 2 with
'setup_ns()'. WDYT?

> -	done
> -}
> -
>  mptcp_lib_events() {
>  	local ns="${1}"
>  	local evts="${2}"

(...)

Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index eec1f04d231f..9e19e3e8d833 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -33,7 +33,7 @@  cleanup()
 {
 	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
 
-	mptcp_lib_ns_exit "${ns1}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index b77fb7065bfb..4e2c5dd0de3c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -142,7 +142,7 @@  cleanup()
 	rm -f "$sin" "$sout"
 	rm -f "$capout"
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
@@ -271,7 +271,6 @@  check_mptcp_disabled()
 	local err=0
 	LC_ALL=C ip netns exec ${disabled_ns} ./mptcp_connect -p 10000 -s MPTCP 127.0.0.1 < "$cin" 2>&1 | \
 		grep -q "^socket: Protocol not available$" && err=1
-	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
 		mptcp_lib_pr_fail "New MPTCP socket cannot be blocked via sysctl"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fefa9173bdaa..87a518b8c19f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -132,8 +132,6 @@  init_shapers()
 cleanup_partial()
 {
 	rm -f "$capout"
-
-	mptcp_lib_ns_exit "${ns1}" "${ns2}"
 }
 
 init() {
@@ -166,6 +164,7 @@  cleanup()
 	rm -rf $evts_ns1 $evts_ns2
 	rm -f "$err"
 	cleanup_partial
+	cleanup_all_ns
 }
 
 print_check()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 59eb77e7813d..bd7d78e4aa83 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -424,14 +424,6 @@  mptcp_lib_ns_init() {
 	done
 }
 
-mptcp_lib_ns_exit() {
-	local netns
-	for netns in "${@}"; do
-		ip netns del "${netns}"
-		rm -f /tmp/"${netns}".{nstat,out}
-	done
-}
-
 mptcp_lib_events() {
 	local ns="${1}"
 	local evts="${2}"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 68899a303a1a..e1026b028739 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -98,7 +98,7 @@  init()
 #shellcheck disable=SC2317
 cleanup()
 {
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
+	cleanup_all_ns
 	rm -f "$cin" "$cout"
 	rm -f "$sin" "$sout"
 }
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 2757378b1b13..5b4d83c2e280 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -36,7 +36,7 @@  err=$(mktemp)
 cleanup()
 {
 	rm -f "${err}"
-	mptcp_lib_ns_exit "${ns1}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index d0b39c2e38a3..6eddb3bba2e8 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -42,7 +42,7 @@  cleanup()
 	rm -f "$large" "$small"
 	rm -f "$capout"
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 9e2981f2d7f5..0c089e7f5f0a 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -107,7 +107,7 @@  cleanup()
 		mptcp_lib_kill_wait $pid
 	done
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}"
+	cleanup_all_ns
 
 	rm -rf $file $client_evts $server_evts