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

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

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

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

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.

Geliang Tang (6):
  selftests: net: rename ns in setup/cleanup_ns
  selftests: mptcp: rename ns to _ns in get_counter
  selftests: mptcp: use setup/cleanup_ns helpers
  selftests: mptcp: diag: trap cleanup after ns_init
  selftests: mptcp: join: print title after init_partial
  selftests: mptcp: use wait_local_port_listen helper

 tools/testing/selftests/net/lib.sh            | 33 ++++++++++-------
 tools/testing/selftests/net/mptcp/diag.sh     |  2 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh |  4 +-
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 37 ++++++-------------
 4 files changed, 35 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v4 1/6] selftests: net: rename ns in setup/cleanup_ns

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helpers setup_ns and cleanup_ns don't work when a namespace named
"ns" is passed to them.

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 both setup_ns and
cleanup_ns. To solve this, this patch renames the local variable "ns"
as "_ns".

Also a ns_name valid check has been added to setup_ns. If "_ns" is
passed in as a ns_name, setup_ns helper exits.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/lib.sh | 33 ++++++++++++++++++------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index edc030e81a46..ce611bc73098 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -128,7 +128,7 @@ slowwait_for_counter()
 
 cleanup_ns()
 {
-	local ns=""
+	local _ns=""
 	local errexit=0
 	local ret=0
 
@@ -138,10 +138,11 @@ cleanup_ns()
 		set +e
 	fi
 
-	for ns in "$@"; do
-		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"
+	for _ns in "$@"; do
+		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"
 			ret=1
 		fi
 	done
@@ -159,29 +160,35 @@ cleanup_all_ns()
 # setup_ns local remote
 setup_ns()
 {
-	local ns=""
+	local _ns=""
 	local ns_name=""
 	local ns_list=""
 	local ns_exist=
 	for ns_name in "$@"; do
+		if [ "${ns_name}" == "_ns" ]; then
+			echo "Failed to setup namespace '${ns_name}': invalid name"
+			cleanup_ns "$ns_list"
+			set -e
+			return $ksft_fail
+		fi
 		# 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="${ns_name,,}-$(mktemp -u XXXXXX)"
+			eval readonly ${ns_name}="$_ns"
 			ns_exist=false
 		else
-			eval ns='$'${ns_name}
-			cleanup_ns "$ns"
+			eval _ns='$'${ns_name}
+			cleanup_ns "$_ns"
 			ns_exist=true
 		fi
 
-		if ! ip netns add "$ns"; then
+		if ! ip netns add "$_ns"; then
 			echo "Failed to create namespace $ns_name"
 			cleanup_ns "$ns_list"
 			return $ksft_skip
 		fi
-		ip -n "$ns" link set lo up
-		! $ns_exist && ns_list="$ns_list $ns"
+		ip -n "$_ns" link set lo up
+		! $ns_exist && ns_list="$ns_list $_ns"
 	done
 	NS_LIST="$NS_LIST $ns_list"
 }
-- 
2.43.0

Re: [PATCH mptcp-next v4 1/6] selftests: net: rename ns in setup/cleanup_ns

[Matthieu Baerts]

Hi Geliang,

On 24/05/2024 08:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helpers setup_ns and cleanup_ns don't work when a namespace named
> "ns" is passed to them.
> 
> 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 both setup_ns and
> cleanup_ns. To solve this, this patch renames the local variable "ns"
> as "_ns".

Do you mind justifying why the new name helps? Something like:

  (...)
  as "_ns", which is more unlikely to conflict with existing variables.
  "_ns" name appears to be unused in the selftests.

> Also a ns_name valid check has been added to setup_ns. If "_ns" is
> passed in as a ns_name, setup_ns helper exits.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/lib.sh | 33 ++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index edc030e81a46..ce611bc73098 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -128,7 +128,7 @@ slowwait_for_counter()
>  
>  cleanup_ns()
>  {
> -	local ns=""
> +	local _ns=""

Are you sure this modification in cleanup_ns is needed as well? The
variable is local, and it is not unset / modified using the name passed
in argument.

>  	local errexit=0
>  	local ret=0
>  
> @@ -138,10 +138,11 @@ cleanup_ns()
>  		set +e
>  	fi
>  
> -	for ns in "$@"; do
> -		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"
> +	for _ns in "$@"; do
> +		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"
>  			ret=1
>  		fi
>  	done
> @@ -159,29 +160,35 @@ cleanup_all_ns()
>  # setup_ns local remote
>  setup_ns()
>  {
> -	local ns=""
> +	local _ns=""
>  	local ns_name=""
>  	local ns_list=""
>  	local ns_exist=
>  	for ns_name in "$@"; do
> +		if [ "${ns_name}" == "_ns" ]; then
> +			echo "Failed to setup namespace '${ns_name}': invalid name"
> +			cleanup_ns "$ns_list"

If the list is empty, it will call cleanup_ns with "", and it will try
to do 'ip netns delete ""', etc.

If ns_list is an array, then 'cleanup_ns "${ns_list[@]}"' is OK. Mmh, I
see there is the same issue below. I can send a fix for that, that would
be for -net, not next. I will look at that this afternoon.

> +			set -e

Why do you need this? (see below)

> +			return $ksft_fail

Why not 'exit ${ksft_fail}' instead? That's an internal error, you can
do that.

> +		fi
>  		# 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="${ns_name,,}-$(mktemp -u XXXXXX)"
> +			eval readonly ${ns_name}="$_ns"
>  			ns_exist=false
>  		else
> -			eval ns='$'${ns_name}
> -			cleanup_ns "$ns"
> +			eval _ns='$'${ns_name}
> +			cleanup_ns "$_ns"
>  			ns_exist=true
>  		fi
>  
> -		if ! ip netns add "$ns"; then
> +		if ! ip netns add "$_ns"; then
>  			echo "Failed to create namespace $ns_name"
>  			cleanup_ns "$ns_list"
>  			return $ksft_skip
>  		fi
> -		ip -n "$ns" link set lo up
> -		! $ns_exist && ns_list="$ns_list $ns"
> +		ip -n "$_ns" link set lo up
> +		! $ns_exist && ns_list="$ns_list $_ns"
>  	done
>  	NS_LIST="$NS_LIST $ns_list"
>  }

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

[PATCH mptcp-next v4 2/6] selftests: mptcp: rename ns to _ns in get_counter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

If "setup_ns ns" is used in diag.sh, it fails with errors:

 ./mptcp_lib.sh: line 289: local: ns: readonly variable
 13 ....chk 2 cestab                                  [ OK ]
 14 ....chk 2->0 msk in use after flush               [ OK ]

That is because "ns" is also a local variable in mptcp_lib_get_counter.
To solve this, this patch renames it as "_ns".

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 6ffa9b7a3260..3397bf511f41 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -312,11 +312,11 @@ mptcp_lib_is_v6() {
 
 # $1: ns, $2: MIB counter
 mptcp_lib_get_counter() {
-	local ns="${1}"
+	local _ns="${1}"
 	local counter="${2}"
 	local count
 
-	count=$(ip netns exec "${ns}" nstat -asz "${counter}" |
+	count=$(ip netns exec "${_ns}" nstat -asz "${counter}" |
 		awk 'NR==1 {next} {print $2}')
 	if [ -z "${count}" ]; then
 		mptcp_lib_fail_if_expected_feature "${counter} counter"
-- 
2.43.0

Re: [PATCH mptcp-next v4 2/6] selftests: mptcp: rename ns to _ns in get_counter

[Matthieu Baerts]

Hi Geliang,

On 24/05/2024 08:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> If "setup_ns ns" is used in diag.sh, it fails with errors:
> 
>  ./mptcp_lib.sh: line 289: local: ns: readonly variable

Mmh, that's due to Bash not allowing local variables with the same name
as readonly ones. Maybe we can avoid using the "readonly" variable,
because we use 'ns' in many other helpers in mptcp_lib.sh, let me check
that, I will come back to you later.

>  13 ....chk 2 cestab                                  [ OK ]
>  14 ....chk 2->0 msk in use after flush               [ OK ]
> 
> That is because "ns" is also a local variable in mptcp_lib_get_counter.
> To solve this, this patch renames it as "_ns".
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v4 3/6] selftests: mptcp: use setup/cleanup_ns helpers

[Geliang Tang]

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 3397bf511f41..60f802e808a7 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 v4 4/6] selftests: mptcp: diag: trap cleanup after ns_init

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

mptcp_lib_ns_init may fail sometimes, then ns isn't setup correctly. In
that case, cleanup is invoked, and ns is used in cleanup. So an "Invalid
netns name ''" error occurs. The patch moves "trap cleanup" from the front
of mptcp_lib_ns_init to the back of it to fix this.

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

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 776d43a6922d..cabe6f8f489f 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -266,8 +266,8 @@ wait_connected()
 	done
 }
 
-trap cleanup EXIT
 mptcp_lib_ns_init ns
+trap cleanup EXIT
 
 echo "a" | \
 	timeout ${timeout_test} \
-- 
2.43.0

[PATCH mptcp-next v4 5/6] selftests: mptcp: join: print title after init_partial

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

init_partial may fail sometimes, in that case, titles shouldn't be
printed out. So the patch moves mptcp_lib_print_title from the front
of init_partial to the back of it.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 2b66c5fa71eb..d20a053eee8f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -269,8 +269,6 @@ reset()
 		return 1
 	fi
 
-	mptcp_lib_print_title "${TEST_NAME}"
-
 	if [ "${init}" != "1" ]; then
 		init
 	else
@@ -279,6 +277,8 @@ reset()
 
 	init_partial
 
+	mptcp_lib_print_title "${TEST_NAME}"
+
 	return 0
 }
 
-- 
2.43.0

Re: [PATCH mptcp-next v4 5/6] selftests: mptcp: join: print title after init_partial

[Matthieu Baerts]

On 24/05/2024 08:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> init_partial may fail sometimes, in that case, titles shouldn't be
> printed out. So the patch moves mptcp_lib_print_title from the front
> of init_partial to the back of it.

I'm not sure to understand why the title should not be printed: it would
help to understand which test had an issue: the next one or the previous
one, no?

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

[PATCH mptcp-next v4 6/6] selftests: mptcp: use wait_local_port_listen helper

[Geliang Tang]

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 60f802e808a7..b539a5436560 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 v4 0/6] 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: Unstable: 1 failed test(s): packetdrill_sockopts 🔴
- KVM Validation: debug: Unstable: 1 failed test(s): packetdrill_sockopts 🔴
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9220006386

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


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 v4 0/6] 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/9220006386

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


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)