[PATCH mptcp-next v6 0/9] use helpers in lib.sh and net_helpers.sh

[PATCH mptcp-next v6 0/9] use helpers in lib.sh and net_helpers.sh

[Matthieu Baerts (NGI0)]

(This is a new version for the series created by Geliang. @Geliang: I
 hope that's OK for you like that, it was easier to describing the
 modifications by email. I validated my changes by running MPTCP
 selftests only)

This set uses the public helpers defined in net selftests to replace
helpers in mptcp_lib.sh.

v6:
 - Replace "rename ns in setup_ns" by "remove 'ns' var in setup_ns": we
   don't need it, avoid extra conflicts.

v5: (Matt)
 - New patches: 1-6: some unrelated fixes, some modifications to avoid
   adding workaround on our side (changing variable names, trap, etc.)
 - "rename ns in setup_ns": drop the modifications in cleanup_ns, exit
   in case of error, and updated commit message.
 - Drop patches 2, 4, 5/6 from v4.

v4:
 - two more fixes added
 - exit when setup_ns fails in mptcp_lib_ns_init

v3:
 - address Matt's comments in v2 (thanks, it's very useful.)
 - don't rename ns in diag.sh, but in lib.sh
 - ns in get_counter needs to be renamed too
 - add a ns_name valid check in setup_ns
 - drop $NS_LIST in mptcp_lib_ns_init
 - don't drop mptcp_lib_ns_init, use cleanup_ns in it
 - don't use cleanup_all_ns, still use mptcp_lib_ns_init

v2:
 - only patch 3 updated, mptcp_join.sh, move cleanup_all_ns from
   cleanup_partial to cleanup, setup_ns will delete existing
   namespaces automatically.
 - update commit logs.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v6:
- Link to v5: https://lore.kernel.org/r/20240524-selftests-net-lib-fixes-v5-0-b9e0968571a3@kernel.org

---
Geliang Tang (3):
      selftests: net: lib: remove 'ns' var in setup_ns
      selftests: mptcp: lib: use setup/cleanup_ns helpers
      selftests: mptcp: lib: use wait_local_port_listen helper

Matthieu Baerts (NGI0) (6):
      selftests: net: lib: set 'i' as local
      selftests: net: lib: support errexit with busywait
      selftests: net: lib: avoid error removing empty netns name
      selftests: net: lib: ignore possible error
      selftests: net: lib: remove ns from list after clean-up
      selftests: net: lib: do not set ns var as readonly

 tools/testing/selftests/net/lib.sh             | 71 +++++++++++++++-----------
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 33 ++++--------
 2 files changed, 50 insertions(+), 54 deletions(-)
---
base-commit: 55aceb609c1baec3e762e938e2028df7ecd545e1
change-id: 20240524-selftests-net-lib-fixes-5a70b8d3b828

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v6 1/9] selftests: net: lib: set 'i' as local

[Matthieu Baerts (NGI0)]

Without this, the 'i' variable declared before could be overridden by
accident, e.g.

  for i in "${@}"; do
      __ksft_status_merge "${i}"  ## 'i' has been modified
      foo "${i}"                  ## using 'i' with an unexpected value
  done

After a quick look, it looks like 'i' is currently not used after having
been modified in __ksft_status_merge(), but still, better be safe than
sorry. I saw this while modifying the same file, not because I suspected
an issue somewhere.

Fixes: 596c8819cb78 ("selftests: forwarding: Have RET track kselftest framework constants")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index edc030e81a46..be80f7b1f9d7 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -27,6 +27,7 @@ __ksft_status_merge()
 	local -A weights
 	local weight=0
 
+	local i
 	for i in "$@"; do
 		weights[$i]=$((weight++))
 	done

-- 
2.43.0

[PATCH mptcp-next v6 2/9] selftests: net: lib: support errexit with busywait

[Matthieu Baerts (NGI0)]

If errexit is enabled ('set -e'), loopy_wait -- or busywait and others
using it -- will stop after the first failure.

Note that if the returned status of loopy_wait is checked, and even if
errexit is enabled, Bash will not stop at the first error.

Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index be80f7b1f9d7..2fbc01003b0f 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -68,9 +68,7 @@ loopy_wait()
 	while true
 	do
 		local out
-		out=$("$@")
-		local ret=$?
-		if ((!ret)); then
+		if out=$("$@"); then
 			echo -n "$out"
 			return 0
 		fi

-- 
2.43.0

[PATCH mptcp-next v6 3/9] selftests: net: lib: avoid error removing empty netns name

[Matthieu Baerts (NGI0)]

If there is an error to create the first netns with 'setup_ns()',
'cleanup_ns()' will be called with an empty string as first parameter.

The consequences is that 'cleanup_ns()' will try to delete an invalid
netns, and wait 20 seconds if the netns list is empty.

Instead of just checking if the name is not empty, convert the string
separated by spaces to an array. Manipulating the array is cleaner, and
calling 'cleanup_ns()' with an empty array will be a no-op.

Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 2fbc01003b0f..9155c914c064 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -15,7 +15,7 @@ ksft_xfail=2
 ksft_skip=4
 
 # namespace list created by setup_ns
-NS_LIST=""
+NS_LIST=()
 
 ##############################################################################
 # Helpers
@@ -138,6 +138,7 @@ cleanup_ns()
 	fi
 
 	for ns in "$@"; do
+		[ -z "${ns}" ] && continue
 		ip netns delete "${ns}" &> /dev/null
 		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
 			echo "Warn: Failed to remove namespace $ns"
@@ -151,7 +152,7 @@ cleanup_ns()
 
 cleanup_all_ns()
 {
-	cleanup_ns $NS_LIST
+	cleanup_ns "${NS_LIST[@]}"
 }
 
 # setup netns with given names as prefix. e.g
@@ -160,7 +161,7 @@ setup_ns()
 {
 	local ns=""
 	local ns_name=""
-	local ns_list=""
+	local ns_list=()
 	local ns_exist=
 	for ns_name in "$@"; do
 		# Some test may setup/remove same netns multi times
@@ -176,13 +177,13 @@ setup_ns()
 
 		if ! ip netns add "$ns"; then
 			echo "Failed to create namespace $ns_name"
-			cleanup_ns "$ns_list"
+			cleanup_ns "${ns_list[@]}"
 			return $ksft_skip
 		fi
 		ip -n "$ns" link set lo up
-		! $ns_exist && ns_list="$ns_list $ns"
+		! $ns_exist && ns_list+=("$ns")
 	done
-	NS_LIST="$NS_LIST $ns_list"
+	NS_LIST+=("${ns_list[@]}")
 }
 
 tc_rule_stats_get()

-- 
2.43.0

[PATCH mptcp-next v6 4/9] selftests: net: lib: ignore possible error

[Matthieu Baerts (NGI0)]

No need to disable errexit temporary, simply ignore the possible error.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 9155c914c064..b2572aff6286 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -128,25 +128,17 @@ slowwait_for_counter()
 cleanup_ns()
 {
 	local ns=""
-	local errexit=0
 	local ret=0
 
-	# disable errexit temporary
-	if [[ $- =~ "e" ]]; then
-		errexit=1
-		set +e
-	fi
-
 	for ns in "$@"; do
 		[ -z "${ns}" ] && continue
-		ip netns delete "${ns}" &> /dev/null
+		ip netns delete "${ns}" &> /dev/null || true
 		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
 			echo "Warn: Failed to remove namespace $ns"
 			ret=1
 		fi
 	done
 
-	[ $errexit -eq 1 ] && set -e
 	return $ret
 }
 

-- 
2.43.0

[PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts (NGI0)]

Instead of only appending items to the list, remove them when the netns
has been deleted.

By doing that, we can make sure 'cleanup_all_ns()' is not trying to
remove already deleted netns.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index b2572aff6286..c7a8cfb477cc 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -125,6 +125,20 @@ slowwait_for_counter()
 	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
 }
 
+remove_ns_list()
+{
+	local item=$1
+	local ns
+	local ns_list=("${NS_LIST[@]}")
+	NS_LIST=()
+
+	for ns in "${ns_list[@]}"; do
+		if [ "${ns}" != "${item}" ]; then
+			NS_LIST+=("${ns}")
+		fi
+	done
+}
+
 cleanup_ns()
 {
 	local ns=""
@@ -136,6 +150,8 @@ cleanup_ns()
 		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
 			echo "Warn: Failed to remove namespace $ns"
 			ret=1
+		else
+			remove_ns_list "${ns}"
 		fi
 	done
 
@@ -154,17 +170,14 @@ setup_ns()
 	local ns=""
 	local ns_name=""
 	local ns_list=()
-	local ns_exist=
 	for ns_name in "$@"; do
 		# Some test may setup/remove same netns multi times
 		if unset ${ns_name} 2> /dev/null; then
 			ns="${ns_name,,}-$(mktemp -u XXXXXX)"
 			eval readonly ${ns_name}="$ns"
-			ns_exist=false
 		else
 			eval ns='$'${ns_name}
 			cleanup_ns "$ns"
-			ns_exist=true
 		fi
 
 		if ! ip netns add "$ns"; then
@@ -173,7 +186,7 @@ setup_ns()
 			return $ksft_skip
 		fi
 		ip -n "$ns" link set lo up
-		! $ns_exist && ns_list+=("$ns")
+		ns_list+=("$ns")
 	done
 	NS_LIST+=("${ns_list[@]}")
 }

-- 
2.43.0

[PATCH mptcp-next v6 6/9] selftests: net: lib: do not set ns var as readonly

[Matthieu Baerts (NGI0)]

It sounds good to mark the global netns variable as 'readonly', but Bash
doesn't allow the creation of local variable with the same name.

Because it looks like 'readonly' is mainly used here to check if a netns
with that name has already been set, it sounds fine to check if the
variable with this name has already been set instead. By doing that, we
avoid having to modify helpers from MPTCP selftests using the same
variable name as the one used to store the created netns name.

While at it, also avoid an unnecessary call to 'eval' to set a local
variable.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index c7a8cfb477cc..114b927fee25 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -172,11 +172,11 @@ setup_ns()
 	local ns_list=()
 	for ns_name in "$@"; do
 		# Some test may setup/remove same netns multi times
-		if unset ${ns_name} 2> /dev/null; then
+		if [ -z "${!ns_name}" ]; then
 			ns="${ns_name,,}-$(mktemp -u XXXXXX)"
-			eval readonly ${ns_name}="$ns"
+			eval "${ns_name}=${ns}"
 		else
-			eval ns='$'${ns_name}
+			ns="${!ns_name}"
 			cleanup_ns "$ns"
 		fi
 

-- 
2.43.0

[PATCH mptcp-next v6 7/9] selftests: net: lib: remove 'ns' var in setup_ns

[Matthieu Baerts (NGI0)]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper setup_ns() doesn't work when a net namespace named "ns" is
passed to it.

For example, in net/mptcp/diag.sh, the name of the namespace is "ns". If
"setup_ns ns" is used in it, diag.sh fails with errors:

  Invalid netns name "./mptcp_connect"
  Cannot open network namespace "10000": No such file or directory
  Cannot open network namespace "10000": No such file or directory

That is because "ns" is also a local variable in setup_ns, and it will
not set the value for the global variable that has been giving in
argument. To solve this, we could rename the variable, but it sounds
better to drop it, as we can resolve the name using the variable passed
in argument instead.

The other local variables -- "ns_list" and "ns_name" -- are more
unlikely to conflict with existing global variables. They don't seem to
be currently used in other net selftests.

Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/lib.sh | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 114b927fee25..915f319bcc8b 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -167,26 +167,30 @@ cleanup_all_ns()
 # setup_ns local remote
 setup_ns()
 {
-	local ns=""
 	local ns_name=""
 	local ns_list=()
 	for ns_name in "$@"; do
-		# Some test may setup/remove same netns multi times
-		if [ -z "${!ns_name}" ]; then
-			ns="${ns_name,,}-$(mktemp -u XXXXXX)"
-			eval "${ns_name}=${ns}"
-		else
-			ns="${!ns_name}"
-			cleanup_ns "$ns"
+		# avoid conflicts with local var: internal error
+		if [ "${ns_name}" = "ns_name" ]; then
+			echo "Failed to setup namespace '${ns_name}': invalid name"
+			cleanup_ns "${ns_list[@]}"
+			exit $ksft_fail
 		fi
 
-		if ! ip netns add "$ns"; then
+		# Some test may setup/remove same netns multi times
+		if [ -z "${!ns_name}" ]; then
+			eval "${ns_name}=${ns_name,,}-$(mktemp -u XXXXXX)"
+		else
+			cleanup_ns "${!ns_name}"
+		fi
+
+		if ! ip netns add "${!ns_name}"; then
 			echo "Failed to create namespace $ns_name"
 			cleanup_ns "${ns_list[@]}"
 			return $ksft_skip
 		fi
-		ip -n "$ns" link set lo up
-		ns_list+=("$ns")
+		ip -n "${!ns_name}" link set lo up
+		ns_list+=("${!ns_name}")
 	done
 	NS_LIST+=("${ns_list[@]}")
 }

-- 
2.43.0

[PATCH mptcp-next v6 8/9] selftests: mptcp: lib: use setup/cleanup_ns helpers

[Matthieu Baerts (NGI0)]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch includes lib.sh into mptcp_lib.sh, uses setup_ns helper
defined in lib.sh to set up namespaces in mptcp_lib_ns_init(), and
uses cleanup_ns to delete namespaces in mptcp_lib_ns_exit().

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 6ffa9b7a3260..d9e30516dc72 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -1,6 +1,8 @@
 #! /bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
+. "$(dirname "${0}")/../lib.sh"
+
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
 readonly KSFT_SKIP=4
@@ -438,17 +440,13 @@ mptcp_lib_check_tools() {
 }
 
 mptcp_lib_ns_init() {
-	local sec rndh
-
-	sec=$(date +%s)
-	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
+	if ! setup_ns ${@}; then
+		mptcp_lib_pr_fail "Failed to setup namespace ${@}"
+		exit ${KSFT_FAIL}
+	fi
 
 	local netns
 	for netns in "${@}"; do
-		eval "${netns}=${netns}-${rndh}"
-
-		ip netns add "${!netns}" || exit ${KSFT_SKIP}
-		ip -net "${!netns}" link set lo up
 		ip netns exec "${!netns}" sysctl -q net.mptcp.enabled=1
 		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
 		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
@@ -456,9 +454,10 @@ mptcp_lib_ns_init() {
 }
 
 mptcp_lib_ns_exit() {
+	cleanup_ns "${@}"
+
 	local netns
 	for netns in "${@}"; do
-		ip netns del "${netns}"
 		rm -f /tmp/"${netns}".{nstat,out}
 	done
 }

-- 
2.43.0

[PATCH mptcp-next v6 9/9] selftests: mptcp: lib: use wait_local_port_listen helper

[Matthieu Baerts (NGI0)]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch includes net_helper.sh into mptcp_lib.sh, uses the helper
wait_local_port_listen() defined in it to implement the similar mptcp
helper. This can drop some duplicate code.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index d9e30516dc72..194c8fc2e55a 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
 
 . "$(dirname "${0}")/../lib.sh"
+. "$(dirname "${0}")/../net_helper.sh"
 
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
@@ -363,20 +364,7 @@ mptcp_lib_check_transfer() {
 
 # $1: ns, $2: port
 mptcp_lib_wait_local_port_listen() {
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex
-	port_hex="$(printf "%04X" "${port}")"
-
-	local _
-	for _ in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) \
-			     {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
+	wait_local_port_listen "${@}" "tcp"
 }
 
 mptcp_lib_check_output() {

-- 
2.43.0

Re: [PATCH mptcp-next v6 0/9] use helpers in lib.sh and net_helpers.sh

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9253831822

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/01b4afee398d
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=856116


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 v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Geliang Tang]

On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
> Instead of only appending items to the list, remove them when the
> netns
> has been deleted.
> 
> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
> remove already deleted netns.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib.sh
> b/tools/testing/selftests/net/lib.sh
> index b2572aff6286..c7a8cfb477cc 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -125,6 +125,20 @@ slowwait_for_counter()
>  	slowwait "$timeout" until_counter_is ">= $((base + delta))"
> "$@"
>  }
>  
> +remove_ns_list()
> +{
> +	local item=$1
> +	local ns
> +	local ns_list=("${NS_LIST[@]}")
> +	NS_LIST=()
> +
> +	for ns in "${ns_list[@]}"; do
> +		if [ "${ns}" != "${item}" ]; then
> +			NS_LIST+=("${ns}")
> +		fi
> +	done
> +}
> +
>  cleanup_ns()
>  {
>  	local ns=""
> @@ -136,6 +150,8 @@ cleanup_ns()
>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
> grep -vq "^$ns$" &> /dev/null; then
>  			echo "Warn: Failed to remove namespace $ns"
>  			ret=1
> +		else

nit: Should we also remove ns_list when "ip netns list" is busy? Or
should we not delete ns when "ip netns list" is busy?

I think we should do these two commands together:

	ip netns delete "${ns}" &> /dev/null || true
	remove_ns_list "${ns}"

I'm not sure. WDYT?

Thanks,
-Geliang

> +			remove_ns_list "${ns}"
>  		fi
>  	done
>  
> @@ -154,17 +170,14 @@ setup_ns()
>  	local ns=""
>  	local ns_name=""
>  	local ns_list=()
> -	local ns_exist=
>  	for ns_name in "$@"; do
>  		# Some test may setup/remove same netns multi times
>  		if unset ${ns_name} 2> /dev/null; then
>  			ns="${ns_name,,}-$(mktemp -u XXXXXX)"
>  			eval readonly ${ns_name}="$ns"
> -			ns_exist=false
>  		else
>  			eval ns='$'${ns_name}
>  			cleanup_ns "$ns"
> -			ns_exist=true
>  		fi
>  
>  		if ! ip netns add "$ns"; then
> @@ -173,7 +186,7 @@ setup_ns()
>  			return $ksft_skip
>  		fi
>  		ip -n "$ns" link set lo up
> -		! $ns_exist && ns_list+=("$ns")
> +		ns_list+=("$ns")
>  	done
>  	NS_LIST+=("${ns_list[@]}")
>  }
> 

Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts]

Hi Geliang,

Thank you for your review!

On 28/05/2024 05:47, Geliang Tang wrote:
> On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
>> Instead of only appending items to the list, remove them when the
>> netns
>> has been deleted.
>>
>> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
>> remove already deleted netns.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/lib.sh
>> b/tools/testing/selftests/net/lib.sh
>> index b2572aff6286..c7a8cfb477cc 100644
>> --- a/tools/testing/selftests/net/lib.sh
>> +++ b/tools/testing/selftests/net/lib.sh
>> @@ -125,6 +125,20 @@ slowwait_for_counter()
>>  	slowwait "$timeout" until_counter_is ">= $((base + delta))"
>> "$@"
>>  }
>>  
>> +remove_ns_list()
>> +{
>> +	local item=$1
>> +	local ns
>> +	local ns_list=("${NS_LIST[@]}")
>> +	NS_LIST=()
>> +
>> +	for ns in "${ns_list[@]}"; do
>> +		if [ "${ns}" != "${item}" ]; then
>> +			NS_LIST+=("${ns}")
>> +		fi
>> +	done
>> +}
>> +
>>  cleanup_ns()
>>  {
>>  	local ns=""
>> @@ -136,6 +150,8 @@ cleanup_ns()
>>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
>> grep -vq "^$ns$" &> /dev/null; then
>>  			echo "Warn: Failed to remove namespace $ns"
>>  			ret=1
>> +		else
> 
> nit: Should we also remove ns_list when "ip netns list" is busy? Or
> should we not delete ns when "ip netns list" is busy?
> 
> I think we should do these two commands together:
> 
> 	ip netns delete "${ns}" &> /dev/null || true
> 	remove_ns_list "${ns}"
> 
> I'm not sure. WDYT?

Good point, I'm not sure either. I kept it in the list because in case
of errors, the caller could still get the list of NS that have been
created, but not deleted yet.

To be honest, I don't think any actions should be done on the NS after
having called cleanup: it would probably better to kill all attached
processes before that, and not retry later after errors and a kill. But
I didn't want to change this logic as there might be other impacts. So
at least here, we let the responsibility to the caller, and it can call
cleanup_all_ns() again in case of errors. WDYT?

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

Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Geliang Tang]

On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your review!
> 
> On 28/05/2024 05:47, Geliang Tang wrote:
> > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
> > > Instead of only appending items to the list, remove them when the
> > > netns
> > > has been deleted.
> > > 
> > > By doing that, we can make sure 'cleanup_all_ns()' is not trying
> > > to
> > > remove already deleted netns.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/lib.sh
> > > b/tools/testing/selftests/net/lib.sh
> > > index b2572aff6286..c7a8cfb477cc 100644
> > > --- a/tools/testing/selftests/net/lib.sh
> > > +++ b/tools/testing/selftests/net/lib.sh
> > > @@ -125,6 +125,20 @@ slowwait_for_counter()
> > >  	slowwait "$timeout" until_counter_is ">= $((base +
> > > delta))"
> > > "$@"
> > >  }
> > >  
> > > +remove_ns_list()
> > > +{
> > > +	local item=$1
> > > +	local ns
> > > +	local ns_list=("${NS_LIST[@]}")
> > > +	NS_LIST=()
> > > +
> > > +	for ns in "${ns_list[@]}"; do
> > > +		if [ "${ns}" != "${item}" ]; then
> > > +			NS_LIST+=("${ns}")
> > > +		fi
> > > +	done
> > > +}
> > > +
> > >  cleanup_ns()
> > >  {
> > >  	local ns=""
> > > @@ -136,6 +150,8 @@ cleanup_ns()
> > >  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
> > > grep -vq "^$ns$" &> /dev/null; then
> > >  			echo "Warn: Failed to remove namespace
> > > $ns"
> > >  			ret=1
> > > +		else
> > 
> > nit: Should we also remove ns_list when "ip netns list" is busy? Or
> > should we not delete ns when "ip netns list" is busy?
> > 
> > I think we should do these two commands together:
> > 
> > 	ip netns delete "${ns}" &> /dev/null || true
> > 	remove_ns_list "${ns}"
> > 
> > I'm not sure. WDYT?
> 
> Good point, I'm not sure either. I kept it in the list because in
> case
> of errors, the caller could still get the list of NS that have been
> created, but not deleted yet.
> 
> To be honest, I don't think any actions should be done on the NS
> after
> having called cleanup: it would probably better to kill all attached
> processes before that, and not retry later after errors and a kill.
> But
> I didn't want to change this logic as there might be other impacts.
> So
> at least here, we let the responsibility to the caller, and it can
> call
> cleanup_all_ns() again in case of errors. WDYT?

If so, I think this is better:

        for ns in "$@"; do
                [ -z "${ns}" ] && continue
                ip netns delete "${ns}" &> /dev/null || true
                remove_ns_list "${ns}"
                if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -
vq "^$ns$" &> /dev/null; then
                        echo "Warn: Failed to remove namespace $ns"
                        ret=1
                fi  
        done

If you agree, please update this when merging it. No need to send a v7.

And please add my Reviewd-by tag. I'll repay it in the cover-letter.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v6 0/9] use helpers in lib.sh and net_helpers.sh

[Geliang Tang]

On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
> (This is a new version for the series created by Geliang. @Geliang: I
>  hope that's OK for you like that, it was easier to describing the
>  modifications by email. I validated my changes by running MPTCP
>  selftests only)
> 
> This set uses the public helpers defined in net selftests to replace
> helpers in mptcp_lib.sh.
> 
> v6:
>  - Replace "rename ns in setup_ns" by "remove 'ns' var in setup_ns":
> we
>    don't need it, avoid extra conflicts.
> 
> v5: (Matt)
>  - New patches: 1-6: some unrelated fixes, some modifications to
> avoid
>    adding workaround on our side (changing variable names, trap,
> etc.)
>  - "rename ns in setup_ns": drop the modifications in cleanup_ns,
> exit
>    in case of error, and updated commit message.
>  - Drop patches 2, 4, 5/6 from v4.
> 
> v4:
>  - two more fixes added
>  - exit when setup_ns fails in mptcp_lib_ns_init
> 
> v3:
>  - address Matt's comments in v2 (thanks, it's very useful.)
>  - don't rename ns in diag.sh, but in lib.sh
>  - ns in get_counter needs to be renamed too
>  - add a ns_name valid check in setup_ns
>  - drop $NS_LIST in mptcp_lib_ns_init
>  - don't drop mptcp_lib_ns_init, use cleanup_ns in it
>  - don't use cleanup_all_ns, still use mptcp_lib_ns_init
> 
> v2:
>  - only patch 3 updated, mptcp_join.sh, move cleanup_all_ns from
>    cleanup_partial to cleanup, setup_ns will delete existing
>    namespaces automatically.
>  - update commit logs.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Thanks Matt, please add my tag:

Reviewed-by: Geliang Tang <geliang@kernel.org>

> ---
> Changes in v6:
> - Link to v5:
> https://lore.kernel.org/r/20240524-selftests-net-lib-fixes-v5-0-b9e0968571a3@kernel.org
> 
> ---
> Geliang Tang (3):
>       selftests: net: lib: remove 'ns' var in setup_ns
>       selftests: mptcp: lib: use setup/cleanup_ns helpers
>       selftests: mptcp: lib: use wait_local_port_listen helper
> 
> Matthieu Baerts (NGI0) (6):
>       selftests: net: lib: set 'i' as local
>       selftests: net: lib: support errexit with busywait
>       selftests: net: lib: avoid error removing empty netns name
>       selftests: net: lib: ignore possible error
>       selftests: net: lib: remove ns from list after clean-up
>       selftests: net: lib: do not set ns var as readonly
> 
>  tools/testing/selftests/net/lib.sh             | 71 +++++++++++++++-
> ----------
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 33 ++++--------
>  2 files changed, 50 insertions(+), 54 deletions(-)
> ---
> base-commit: 55aceb609c1baec3e762e938e2028df7ecd545e1
> change-id: 20240524-selftests-net-lib-fixes-5a70b8d3b828
> 
> Best regards,

Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts]

Hi Geliang,

On 28/05/2024 15:18, Geliang Tang wrote:
> On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for your review!
>>
>> On 28/05/2024 05:47, Geliang Tang wrote:
>>> On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
>>>> Instead of only appending items to the list, remove them when the
>>>> netns
>>>> has been deleted.
>>>>
>>>> By doing that, we can make sure 'cleanup_all_ns()' is not trying
>>>> to
>>>> remove already deleted netns.
>>>>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
>>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/lib.sh
>>>> b/tools/testing/selftests/net/lib.sh
>>>> index b2572aff6286..c7a8cfb477cc 100644
>>>> --- a/tools/testing/selftests/net/lib.sh
>>>> +++ b/tools/testing/selftests/net/lib.sh
>>>> @@ -125,6 +125,20 @@ slowwait_for_counter()
>>>>  	slowwait "$timeout" until_counter_is ">= $((base +
>>>> delta))"
>>>> "$@"
>>>>  }
>>>>  
>>>> +remove_ns_list()
>>>> +{
>>>> +	local item=$1
>>>> +	local ns
>>>> +	local ns_list=("${NS_LIST[@]}")
>>>> +	NS_LIST=()
>>>> +
>>>> +	for ns in "${ns_list[@]}"; do
>>>> +		if [ "${ns}" != "${item}" ]; then
>>>> +			NS_LIST+=("${ns}")
>>>> +		fi
>>>> +	done
>>>> +}
>>>> +
>>>>  cleanup_ns()
>>>>  {
>>>>  	local ns=""
>>>> @@ -136,6 +150,8 @@ cleanup_ns()
>>>>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
>>>> grep -vq "^$ns$" &> /dev/null; then
>>>>  			echo "Warn: Failed to remove namespace
>>>> $ns"
>>>>  			ret=1
>>>> +		else
>>>
>>> nit: Should we also remove ns_list when "ip netns list" is busy? Or
>>> should we not delete ns when "ip netns list" is busy?
>>>
>>> I think we should do these two commands together:
>>>
>>> 	ip netns delete "${ns}" &> /dev/null || true
>>> 	remove_ns_list "${ns}"
>>>
>>> I'm not sure. WDYT?
>>
>> Good point, I'm not sure either. I kept it in the list because in
>> case
>> of errors, the caller could still get the list of NS that have been
>> created, but not deleted yet.
>>
>> To be honest, I don't think any actions should be done on the NS
>> after
>> having called cleanup: it would probably better to kill all attached
>> processes before that, and not retry later after errors and a kill.
>> But
>> I didn't want to change this logic as there might be other impacts.
>> So
>> at least here, we let the responsibility to the caller, and it can
>> call
>> cleanup_all_ns() again in case of errors. WDYT?
> 
> If so, I think this is better:
> 
>         for ns in "$@"; do
>                 [ -z "${ns}" ] && continue
>                 ip netns delete "${ns}" &> /dev/null || true
>                 remove_ns_list "${ns}"
>                 if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -
> vq "^$ns$" &> /dev/null; then
>                         echo "Warn: Failed to remove namespace $ns"
>                         ret=1
>                 fi  
>         done

Sorry, I was not clear enough: I wanted to say that I think it is better
to keep "remove_ns_list" in the "else", only to remove the netns from
the list if it has been deleted. By doing that, the caller can call
cleanup_all_ns() again, after having stopped everything.

(That's a detail, I guess the removal should not fail, and if it does,
the caller should adapt the code to make sure the clean doesn't fail.)

> If you agree, please update this when merging it. No need to send a v7.

Thanks! I will wait for your feedback on what is above before applying
this series (even if we can always have squash-to patches later).

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

Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Geliang Tang]

On Tue, 2024-05-28 at 17:26 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/05/2024 15:18, Geliang Tang wrote:
> > On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for your review!
> > > 
> > > On 28/05/2024 05:47, Geliang Tang wrote:
> > > > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > Instead of only appending items to the list, remove them when
> > > > > the
> > > > > netns
> > > > > has been deleted.
> > > > > 
> > > > > By doing that, we can make sure 'cleanup_all_ns()' is not
> > > > > trying
> > > > > to
> > > > > remove already deleted netns.
> > > > > 
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > >  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---
> > > > > -
> > > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/net/lib.sh
> > > > > b/tools/testing/selftests/net/lib.sh
> > > > > index b2572aff6286..c7a8cfb477cc 100644
> > > > > --- a/tools/testing/selftests/net/lib.sh
> > > > > +++ b/tools/testing/selftests/net/lib.sh
> > > > > @@ -125,6 +125,20 @@ slowwait_for_counter()
> > > > >  	slowwait "$timeout" until_counter_is ">= $((base +
> > > > > delta))"
> > > > > "$@"
> > > > >  }
> > > > >  
> > > > > +remove_ns_list()
> > > > > +{
> > > > > +	local item=$1
> > > > > +	local ns
> > > > > +	local ns_list=("${NS_LIST[@]}")
> > > > > +	NS_LIST=()
> > > > > +
> > > > > +	for ns in "${ns_list[@]}"; do
> > > > > +		if [ "${ns}" != "${item}" ]; then
> > > > > +			NS_LIST+=("${ns}")
> > > > > +		fi
> > > > > +	done
> > > > > +}
> > > > > +
> > > > >  cleanup_ns()
> > > > >  {
> > > > >  	local ns=""
> > > > > @@ -136,6 +150,8 @@ cleanup_ns()
> > > > >  		if ! busywait $BUSYWAIT_TIMEOUT ip netns
> > > > > list \|
> > > > > grep -vq "^$ns$" &> /dev/null; then
> > > > >  			echo "Warn: Failed to remove
> > > > > namespace
> > > > > $ns"
> > > > >  			ret=1
> > > > > +		else
> > > > 
> > > > nit: Should we also remove ns_list when "ip netns list" is
> > > > busy? Or
> > > > should we not delete ns when "ip netns list" is busy?
> > > > 
> > > > I think we should do these two commands together:
> > > > 
> > > > 	ip netns delete "${ns}" &> /dev/null || true
> > > > 	remove_ns_list "${ns}"
> > > > 
> > > > I'm not sure. WDYT?
> > > 
> > > Good point, I'm not sure either. I kept it in the list because in
> > > case
> > > of errors, the caller could still get the list of NS that have
> > > been
> > > created, but not deleted yet.
> > > 
> > > To be honest, I don't think any actions should be done on the NS
> > > after
> > > having called cleanup: it would probably better to kill all
> > > attached
> > > processes before that, and not retry later after errors and a
> > > kill.
> > > But
> > > I didn't want to change this logic as there might be other
> > > impacts.
> > > So
> > > at least here, we let the responsibility to the caller, and it
> > > can
> > > call
> > > cleanup_all_ns() again in case of errors. WDYT?
> > 
> > If so, I think this is better:
> > 
> >         for ns in "$@"; do
> >                 [ -z "${ns}" ] && continue
> >                 ip netns delete "${ns}" &> /dev/null || true
> >                 remove_ns_list "${ns}"
> >                 if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
> > grep -
> > vq "^$ns$" &> /dev/null; then
> >                         echo "Warn: Failed to remove namespace $ns"
> >                         ret=1
> >                 fi  
> >         done
> 
> Sorry, I was not clear enough: I wanted to say that I think it is
> better
> to keep "remove_ns_list" in the "else", only to remove the netns from
> the list if it has been deleted. By doing that, the caller can call
> cleanup_all_ns() again, after having stopped everything.

Yes, you're right, it's better to keep "remove_ns_list" in the "else".
My bad, I misunderstood it. Please apply this patch as is.

> 
> (That's a detail, I guess the removal should not fail, and if it
> does,
> the caller should adapt the code to make sure the clean doesn't
> fail.)
> 
> > If you agree, please update this when merging it. No need to send a
> > v7.
> 
> Thanks! I will wait for your feedback on what is above before
> applying
> this series (even if we can always have squash-to patches later).

I changed this series as "Queued" on patchwork.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v6 0/9] use helpers in lib.sh and net_helpers.sh

[Matthieu Baerts]

Hi Geliang,

On 27/05/2024 12:58, Matthieu Baerts (NGI0) wrote:
> (This is a new version for the series created by Geliang. @Geliang: I
>  hope that's OK for you like that, it was easier to describing the
>  modifications by email. I validated my changes by running MPTCP
>  selftests only)
> 
> This set uses the public helpers defined in net selftests to replace
> helpers in mptcp_lib.sh.
> 
> v6:
>  - Replace "rename ns in setup_ns" by "remove 'ns' var in setup_ns": we
>    don't need it, avoid extra conflicts.

Now in our tree (fixes for -net and feat. for -next):

New patches for t/upstream-net and t/upstream:
- 531b9e5222f4: selftests: net: lib: set 'i' as local
- e47f89091b76: selftests: net: lib: support errexit with busywait
- 8266f8c1b153: selftests: net: lib: avoid error removing empty netns name
- Results: e85c0954b4e4..e83ba0520cf6 (export-net)
- Results: 5e8a75a149b7..c1dac3d16c4c (export)

New patches for t/upstream:
- 713a5cd1095c: selftests: net: lib: ignore possible error
- 74952f110c3d: selftests: net: lib: remove ns from list after clean-up
- 8d28d8be5404: selftests: net: lib: do not set ns var as readonly
- 3a8b0674ddc6: selftests: net: lib: remove 'ns' var in setup_ns
- f65043962737: selftests: mptcp: lib: use setup/cleanup_ns helpers
- 15270f7eb6d0: selftests: mptcp: lib: use wait_local_port_listen helper
- Results: c1dac3d16c4c..aedee3f57caf (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/21fc491d972666fdd691ae3c3f9a07177487b19a/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/928d380f179b846a4ba1ab0bfb21f1a215b5a469/checks

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