[PATCH net-next 0/6] selftests: mptcp: use net/lib.sh to manage netns

[PATCH net-next 0/6] selftests: mptcp: use net/lib.sh to manage netns

[Matthieu Baerts (NGI0)]

The goal of this series is to use helpers from net/lib.sh with MPTCP
selftests.

- Patches 1 to 4 are some clean-ups and preparation in net/lib.sh:

  - Patch 1 simplifies the code handling errexit by ignoring possible
    errors instead of disabling errexit temporary.

  - Patch 2 removes the netns from the list after having cleaned it, not
    to try to clean it twice.

  - Patch 3 removes the 'readonly' attribute for the netns variable, to
    allow using the same name in local variables.

  - Patch 4 removes the local 'ns' var, not to conflict with the global
    one it needs to setup.

- Patch 5 uses helpers from net/lib.sh to create and delete netns in
  MPTCP selftests.

- Patch 6 uses wait_local_port_listen helper from net/net_helper.sh.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@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) (3):
      selftests: net: lib: ignore possible errors
      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             | 55 +++++++++++++++-----------
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 33 +++++-----------
 2 files changed, 42 insertions(+), 46 deletions(-)
---
base-commit: a999973236543f0b8f6daeaa7ecba7488c3a593b
change-id: 20240607-upstream-net-next-20240607-selftests-mptcp-net-lib-365e43e2e1ca

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

[PATCH net-next 1/6] selftests: net: lib: ignore possible errors

[Matthieu Baerts (NGI0)]

No need to disable errexit temporary, simply ignore the only possible
and not handled error.

Reviewed-by: Geliang Tang <geliang@kernel.org>
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 net-next 2/6] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts (NGI0)]

Instead of only appending items to the list, removing 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.

Reviewed-by: Geliang Tang <geliang@kernel.org>
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 net-next 3/6] 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 variables 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 a
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.

Reviewed-by: Geliang Tang <geliang@kernel.org>
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 net-next 4/6] 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 any 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>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 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 net-next 5/6] 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>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 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 net-next 6/6] 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.

It looks like this helper from net_helper.sh was originally coming from
MPTCP, but MPTCP selftests have not been updated to use it from this
shared place.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 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 net-next 0/6] selftests: mptcp: use net/lib.sh to manage netns

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 07 Jun 2024 18:31:01 +0200 you wrote:
> The goal of this series is to use helpers from net/lib.sh with MPTCP
> selftests.
> 
> - Patches 1 to 4 are some clean-ups and preparation in net/lib.sh:
> 
>   - Patch 1 simplifies the code handling errexit by ignoring possible
>     errors instead of disabling errexit temporary.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] selftests: net: lib: ignore possible errors
    https://git.kernel.org/netdev/net-next/c/7e0620bc6a5e
  - [net-next,2/6] selftests: net: lib: remove ns from list after clean-up
    https://git.kernel.org/netdev/net-next/c/92fe5670271a
  - [net-next,3/6] selftests: net: lib: do not set ns var as readonly
    https://git.kernel.org/netdev/net-next/c/577db6bd5750
  - [net-next,4/6] selftests: net: lib: remove 'ns' var in setup_ns
    https://git.kernel.org/netdev/net-next/c/f8a2d2f874b7
  - [net-next,5/6] selftests: mptcp: lib: use setup/cleanup_ns helpers
    https://git.kernel.org/netdev/net-next/c/f265d3119a29
  - [net-next,6/6] selftests: mptcp: lib: use wait_local_port_listen helper
    https://git.kernel.org/netdev/net-next/c/1af3bc912eac

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 2/6] selftests: net: lib: remove ns from list after clean-up

[Simon Horman]

On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
> Instead of only appending items to the list, removing 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.
> 
> Reviewed-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Hi Matthieu,

I like this, and I am happy to see that it has been accepted.

I do wonder if we can go a step further and use an associative array for
ns_list (maybe renamed).  I think this would reduce remove_ns_list to
something like:

	unset ns_list["$item"]

OTOH, perhaps this breaks with older versions of bash that we still
care about.

...

Re: [PATCH net-next 2/6] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts]

Hi Simon,

Thank you for your reply!

On 14/06/2024 12:40, Simon Horman wrote:
> On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
>> Instead of only appending items to the list, removing 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.
> 
> I do wonder if we can go a step further and use an associative array for
> ns_list (maybe renamed).  I think this would reduce remove_ns_list to
> something like:
> 
> 	unset ns_list["$item"]

I agree that it would ease the removal of one item -- which is not
complex to deal with the new helper :) -- but do you see any other benefits?

For the moment, there is no other value to associate with, so we would
do something like NS_MAP["$ns"]=1. We could link the name of the global
variable, but that's not needed for the tests for the moment.

Also, I don't know if it is important, but when we will iterate over the
list of netns, it will not be done following the same order items have
been added into the hashmap. So we will change the order in which items
are deleted.

> OTOH, perhaps this breaks with older versions of bash that we still
> care about.

Good point. I don't have the answer, but associative arrays are starting
to be quite old now :)

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

Re: [PATCH net-next 2/6] selftests: net: lib: remove ns from list after clean-up

[Simon Horman]

Hi Matthieu,

Likewise, thanks for your response.

On Fri, Jun 14, 2024 at 04:42:38PM +0200, Matthieu Baerts wrote:
> Hi Simon,
> 
> Thank you for your reply!
> 
> On 14/06/2024 12:40, Simon Horman wrote:
> > On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
> >> Instead of only appending items to the list, removing 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.
> > 
> > I do wonder if we can go a step further and use an associative array for
> > ns_list (maybe renamed).  I think this would reduce remove_ns_list to
> > something like:
> > 
> > 	unset ns_list["$item"]
> 
> I agree that it would ease the removal of one item -- which is not
> complex to deal with the new helper :) -- but do you see any other benefits?
> 
> For the moment, there is no other value to associate with, so we would
> do something like NS_MAP["$ns"]=1. We could link the name of the global
> variable, but that's not needed for the tests for the moment.
> 
> Also, I don't know if it is important, but when we will iterate over the
> list of netns, it will not be done following the same order items have
> been added into the hashmap. So we will change the order in which items
> are deleted.

I agree that it would probably end up being a NS_MAP["$ns"]=1,
i.e. a dummy value as there is no natural one to use.

I had not considered the order issue.

And yes, the benefit I see would be limited to removal.
Which as you point out is not a terrible burden with the helper you added.
While, OTOH, my idea adds complexity and unknowns elsewhere.

So overall, perhaps it's best left as an idea for later.
As the code changes for other reasons (who knows what?)
an associative array may make more sense than it does now.

> > OTOH, perhaps this breaks with older versions of bash that we still
> > care about.
> 
> Good point. I don't have the answer, but associative arrays are starting
> to be quite old now :)

Yes, I think so too.
But I also thought it was worth mentioning.