[PATCH mptcp-next v3 0/3] add helpers and vars in mptcp_lib.sh, part 2

[PATCH mptcp-next v3 0/3] add helpers and vars in mptcp_lib.sh, part 2

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v3:
 - patch #2, drop this line in mptcp_lib_ns_exit():
	rm -f /tmp/"$netns".{nstat,out}

Depends on:
 - dump for userspace pm, v14

Geliang Tang (3):
  selftests: mptcp: connect: add local variable rndh
  selftests: mptcp: add mptcp_lib_ns_* helpers
  selftests: mptcp: add mptcp_lib_evts_* helpers

 tools/testing/selftests/net/mptcp/diag.sh     |  9 +--
 .../selftests/net/mptcp/mptcp_connect.sh      | 25 +++---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 27 ++-----
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 78 +++++++++++++++++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 16 ++--
 .../testing/selftests/net/mptcp/pm_netlink.sh |  9 +--
 .../selftests/net/mptcp/simult_flows.sh       | 16 ++--
 .../selftests/net/mptcp/userspace_pm.sh       | 42 +++-------
 8 files changed, 122 insertions(+), 100 deletions(-)

-- 
2.40.1

[PATCH mptcp-next v3 1/3] selftests: mptcp: connect: add local variable rndh

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a local variable rndh in do_transfer(), setting it with
${connector_ns:4}, not the global variable rndh. The global one is hidden
in the next commit.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index ea52110c3fbc..b609649311f6 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -348,6 +348,7 @@ do_transfer()
 
 	if $capture; then
 		local capuser
+		local rndh="${connector_ns:4}"
 		if [ -z $SUDO_USER ] ; then
 			capuser=""
 		else
-- 
2.40.1

[PATCH mptcp-next v3 2/3] selftests: mptcp: add mptcp_lib_ns_* helpers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add helpers mptcp_lib_ns_init() and mptcp_lib_ns_exit() in mptcp_lib.sh to
initialize and delete the given namespaces. Then every test script can
invoke these helpers and use all namespaces.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh     |  9 +++----
 .../selftests/net/mptcp/mptcp_connect.sh      | 24 +++++++------------
 .../testing/selftests/net/mptcp/mptcp_join.sh | 11 ++-------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 23 ++++++++++++++++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 16 ++++---------
 .../testing/selftests/net/mptcp/pm_netlink.sh |  9 +++----
 .../selftests/net/mptcp/simult_flows.sh       | 16 ++++---------
 .../selftests/net/mptcp/userspace_pm.sh       | 14 ++++-------
 8 files changed, 54 insertions(+), 68 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 60a7009ce1b5..162d7b90c3f5 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -3,9 +3,8 @@
 
 . "$(dirname "${0}")/mptcp_lib.sh"
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns="ns1-$rndh"
+ns=""
+mptcp_lib_ns_init ns
 ksft_skip=4
 test_cnt=1
 timeout_poll=30
@@ -30,7 +29,7 @@ cleanup()
 {
 	ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
 
-	ip netns del $ns
+	mptcp_lib_ns_exit "${ns}"
 }
 
 mptcp_lib_check_mptcp
@@ -205,8 +204,6 @@ wait_connected()
 }
 
 trap cleanup EXIT
-ip netns add $ns
-ip -n $ns link set dev lo up
 
 echo "a" | \
 	timeout ${timeout_test} \
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index b609649311f6..6900f7f4d88d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -121,12 +121,11 @@ while getopts "$optstring" option;do
 	esac
 done
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
-ns3="ns3-$rndh"
-ns4="ns4-$rndh"
+ns1=""
+ns2=""
+ns3=""
+ns4=""
+mptcp_lib_ns_init ns1 ns2 ns3 ns4
 
 TEST_COUNT=0
 TEST_GROUP=""
@@ -140,9 +139,9 @@ cleanup()
 
 	local netns
 	for netns in "$ns1" "$ns2" "$ns3" "$ns4";do
-		ip netns del $netns
 		rm -f /tmp/$netns.{nstat,out}
 	done
+	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
 }
 
 mptcp_lib_check_mptcp
@@ -158,11 +157,6 @@ cin_disconnect="$cin".disconnect
 cout_disconnect="$cout".disconnect
 trap cleanup EXIT
 
-for i in "$ns1" "$ns2" "$ns3" "$ns4";do
-	ip netns add $i || exit $ksft_skip
-	ip -net $i link set lo up
-done
-
 #  "$ns1"              ns2                    ns3                     ns4
 # ns1eth2    ns2eth1   ns2eth3      ns3eth2   ns3eth4       ns4eth3
 #                           - drop 1% ->            reorder 25%
@@ -251,8 +245,8 @@ fi
 
 check_mptcp_disabled()
 {
-	local disabled_ns="ns_disabled-$rndh"
-	ip netns add ${disabled_ns} || exit $ksft_skip
+	local disabled_ns=""
+	mptcp_lib_ns_init disabled_ns
 
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
@@ -266,7 +260,7 @@ 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
-	ip netns delete ${disabled_ns}
+	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
 		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[ FAIL ]"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 9f1476f0e2ae..bea7627c7313 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -86,17 +86,10 @@ init_partial()
 {
 	capout=$(mktemp)
 
-	local sec rndh
-	sec=$(date +%s)
-	rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-
-	ns1="ns1-$rndh"
-	ns2="ns2-$rndh"
+	mptcp_lib_ns_init ns1 ns2
 
 	local netns
 	for netns in "$ns1" "$ns2"; do
-		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.mptcp.pm_type=0 2>/dev/null || true
 		ip netns exec $netns sysctl -q net.ipv4.conf.all.rp_filter=0
@@ -147,9 +140,9 @@ cleanup_partial()
 
 	local netns
 	for netns in "$ns1" "$ns2"; do
-		ip netns del $netns
 		rm -f /tmp/$netns.{nstat,out}
 	done
+	mptcp_lib_ns_exit "${ns1}" "${ns2}"
 }
 
 init() {
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 556a7d9784d7..14c0b97a153e 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -373,3 +373,26 @@ mptcp_lib_check_output() {
 		return 1
 	fi
 }
+
+mptcp_lib_ns_init() {
+	local sec
+	local rndh=""
+
+	sec=$(date +%s)
+	rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
+
+	local netns
+	for netns in "${@}"; do
+		eval "${netns}=${netns}-${rndh}"
+
+		ip netns add "${!netns}" || exit ${KSFT_SKIP}
+		ip -net "${!netns}" link set lo up
+	done
+}
+
+mptcp_lib_ns_exit() {
+	local netns
+	for netns in "${@}"; do
+		ip netns del "$netns"
+	done
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index fd7de1b3dc55..58bf83d4e7fa 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -14,11 +14,10 @@ timeout_test=$((timeout_poll * 2 + 1))
 iptables="iptables"
 ip6tables="ip6tables"
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
-ns_sbox="ns_sbox-$rndh"
+ns1=""
+ns2=""
+ns_sbox=""
+mptcp_lib_ns_init ns1 ns2 ns_sbox
 
 add_mark_rules()
 {
@@ -42,8 +41,6 @@ init()
 {
 	local netns
 	for netns in "$ns1" "$ns2" "$ns_sbox";do
-		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
@@ -79,10 +76,7 @@ init()
 
 cleanup()
 {
-	local netns
-	for netns in "$ns1" "$ns2" "$ns_sbox"; do
-		ip netns del $netns
-	done
+	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
 	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 1ec9d8622fc9..c117eea92b6a 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -24,15 +24,14 @@ while getopts "$optstring" option;do
 	esac
 done
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
+ns1=""
+mptcp_lib_ns_init ns1
 err=$(mktemp)
 
 cleanup()
 {
 	rm -f $err
-	ip netns del $ns1
+	mptcp_lib_ns_exit "${ns1}"
 }
 
 mptcp_lib_check_mptcp
@@ -40,8 +39,6 @@ mptcp_lib_check_tools ip
 
 trap cleanup EXIT
 
-ip netns add $ns1 || exit $ksft_skip
-ip -net $ns1 link set lo up
 ip netns exec $ns1 sysctl -q net.mptcp.enabled=1
 
 check()
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 7d8388ecc966..67cddd40f211 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -3,11 +3,10 @@
 
 . "$(dirname "${0}")/mptcp_lib.sh"
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
-ns3="ns3-$rndh"
+ns1=""
+ns2=""
+ns3=""
+mptcp_lib_ns_init ns1 ns2 ns3
 capture=false
 ksft_skip=4
 timeout_poll=30
@@ -36,10 +35,7 @@ cleanup()
 	rm -f "$large" "$small"
 	rm -f "$capout"
 
-	local netns
-	for netns in "$ns1" "$ns2" "$ns3";do
-		ip netns del $netns
-	done
+	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
 }
 
 mptcp_lib_check_mptcp
@@ -66,8 +62,6 @@ setup()
 	trap cleanup EXIT
 
 	for i in "$ns1" "$ns2" "$ns3";do
-		ip netns add $i || exit $ksft_skip
-		ip -net $i link set lo up
 		ip netns exec $i sysctl -q net.ipv4.conf.all.rp_filter=0
 		ip netns exec $i sysctl -q net.ipv4.conf.default.rp_filter=0
 	done
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 629fc5d0ecc5..10074709420a 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -50,10 +50,9 @@ app6_port=50004
 client_addr_id=${RANDOM:0:2}
 server_addr_id=${RANDOM:0:2}
 
-sec=$(date +%s)
-rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
+ns1=""
+ns2=""
+mptcp_lib_ns_init ns1 ns2
 ret=0
 test_name=""
 
@@ -118,10 +117,7 @@ cleanup()
 		mptcp_lib_kill_wait $pid
 	done
 
-	local netns
-	for netns in "$ns1" "$ns2" ;do
-		ip netns del "$netns"
-	done
+	mptcp_lib_ns_exit "${ns1}" "${ns2}"
 
 	rm -rf $file $client_evts $server_evts
 
@@ -132,8 +128,6 @@ trap cleanup EXIT
 
 # Create and configure network namespaces for testing
 for i in "$ns1" "$ns2" ;do
-	ip netns add "$i" || exit 1
-	ip -net "$i" link set lo up
 	ip netns exec "$i" sysctl -q net.mptcp.enabled=1
 	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
 done
-- 
2.40.1

[PATCH mptcp-next v3 3/3] selftests: mptcp: add mptcp_lib_evts_* helpers

[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.

This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
and mptcp_join.sh into four helpers: mptcp_lib_evts_init(), _start(),
_kill() and _remove(). Define them in mptcp_lib.sh and use these new
helpers in both scripts.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 16 ++----
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 55 +++++++++++++++++++
 .../selftests/net/mptcp/userspace_pm.sh       | 28 +++-------
 3 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index bea7627c7313..006fefaf2e2a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -158,8 +158,7 @@ init() {
 	cinsent=$(mktemp)
 	cout=$(mktemp)
 	err=$(mktemp)
-	evts_ns1=$(mktemp)
-	evts_ns2=$(mktemp)
+	mptcp_lib_evts_init evts_ns1 evts_ns2
 
 	trap cleanup EXIT
 
@@ -172,7 +171,7 @@ cleanup()
 	rm -f "$cin" "$cout" "$sinfail"
 	rm -f "$sin" "$sout" "$cinsent" "$cinfail"
 	rm -f "$tmpfile"
-	rm -rf $evts_ns1 $evts_ns2
+	mptcp_lib_evts_remove "${evts_ns1}" "${evts_ns2}"
 	rm -f "$err"
 	cleanup_partial
 }
@@ -437,12 +436,8 @@ reset_with_events()
 {
 	reset "${1}" || return 1
 
-	:> "$evts_ns1"
-	:> "$evts_ns2"
-	ip netns exec $ns1 ./pm_nl_ctl events >> "$evts_ns1" 2>&1 &
-	evts_ns1_pid=$!
-	ip netns exec $ns2 ./pm_nl_ctl events >> "$evts_ns2" 2>&1 &
-	evts_ns2_pid=$!
+	mptcp_lib_evts_start "${ns1}" "${ns2}" "${evts_ns1}" "${evts_ns2}" \
+			     evts_ns1_pid evts_ns2_pid
 }
 
 reset_with_tcp_filter()
@@ -614,8 +609,7 @@ wait_mpj()
 
 kill_events_pids()
 {
-	mptcp_lib_kill_wait $evts_ns1_pid
-	mptcp_lib_kill_wait $evts_ns2_pid
+	mptcp_lib_evts_kill evts_ns1_pid evts_ns2_pid
 }
 
 pm_nl_set_limits()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 14c0b97a153e..9573edb4efb1 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -396,3 +396,58 @@ mptcp_lib_ns_exit() {
 		ip netns del "$netns"
 	done
 }
+
+mptcp_lib_evts_init() {
+	local arg
+
+	for arg in "${@}"; do
+		declare -n evts="${arg}"
+
+		if [ -z "${evts}" ]; then
+			evts=$(mktemp)
+		fi
+	done
+}
+
+# $1 ns1, $2 ns2
+mptcp_lib_evts_start() {
+	local ns_1="${1}"
+	local ns_2="${2}"
+	local evts_1="${3}"
+	local evts_2="${4}"
+	declare -n pid_1="${5}"
+	declare -n pid_2="${6}"
+
+	:>"${evts_1}"
+	:>"${evts_2}"
+
+	if [ ${pid_1} -ne 0 ]; then
+		mptcp_lib_kill_wait "${pid_1}"
+	fi
+	ip netns exec "${ns_1}" ./pm_nl_ctl events >> "${evts_1}" 2>&1 &
+	pid_1=$!
+
+	if [ ${pid_2} -ne 0 ]; then
+		mptcp_lib_kill_wait "${pid_2}"
+	fi
+	ip netns exec "${ns_2}" ./pm_nl_ctl events >> "${evts_2}" 2>&1 &
+	pid_2=$!
+}
+
+mptcp_lib_evts_kill() {
+	declare -n pid_1="${1}"
+	declare -n pid_2="${2}"
+
+	mptcp_lib_kill_wait "${pid_1}"
+	mptcp_lib_kill_wait "${pid_2}"
+
+	pid_1=0
+	pid_2=0
+}
+
+mptcp_lib_evts_remove() {
+	local evts_1="${1}"
+	local evts_2="${2}"
+
+	rm -rf "${evts_1}" "${evts_2}"
+}
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 10074709420a..b6a03d96e786 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -111,15 +111,16 @@ cleanup()
 
 	# Terminate the MPTCP connection and related processes
 	local pid
-	for pid in $client4_pid $server4_pid $client6_pid $server6_pid\
-		   $server_evts_pid $client_evts_pid
+	for pid in $client4_pid $server4_pid $client6_pid $server6_pid
 	do
 		mptcp_lib_kill_wait $pid
 	done
+	mptcp_lib_evts_kill server_evts_pid client_evts_pid
 
 	mptcp_lib_ns_exit "${ns1}" "${ns2}"
 
-	rm -rf $file $client_evts $server_evts
+	rm -rf $file
+	mptcp_lib_evts_remove "${server_evts}" "${client_evts}"
 
 	_printf "Done\n"
 }
@@ -176,24 +177,9 @@ make_connection()
 
 	# Capture netlink events over the two network namespaces running
 	# the MPTCP client and server
-	if [ -z "$client_evts" ]; then
-		client_evts=$(mktemp)
-	fi
-	:>"$client_evts"
-	if [ $client_evts_pid -ne 0 ]; then
-		mptcp_lib_kill_wait $client_evts_pid
-	fi
-	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
-	client_evts_pid=$!
-	if [ -z "$server_evts" ]; then
-		server_evts=$(mktemp)
-	fi
-	:>"$server_evts"
-	if [ $server_evts_pid -ne 0 ]; then
-		mptcp_lib_kill_wait $server_evts_pid
-	fi
-	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &
-	server_evts_pid=$!
+	mptcp_lib_evts_init server_evts client_evts
+	mptcp_lib_evts_start "${ns1}" "${ns2}" "${server_evts}" "${client_evts}" \
+			     server_evts_pid client_evts_pid
 	sleep 0.5
 
 	# Run the server
-- 
2.40.1

Re: [PATCH mptcp-next v3 0/3] add helpers and vars in mptcp_lib.sh, part 2

[Matthieu Baerts]

Hi Geliang,

On 21/02/2024 8:06 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v3:
>  - patch #2, drop this line in mptcp_lib_ns_exit():
> 	rm -f /tmp/"$netns".{nstat,out}

Thank you for this new version. Please see my individual comments.

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

Re: [PATCH mptcp-next v3 1/3] selftests: mptcp: connect: add local variable rndh

[Matthieu Baerts]

Hi Geliang,

On 21/02/2024 8:06 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a local variable rndh in do_transfer(), setting it with
> ${connector_ns:4}, not the global variable rndh. The global one is hidden
> in the next commit.

Do you not need to do the same in simult_flows.sh?

If it is just these two modifications, and because you have other
behaviour changes in patch 2/3, maybe easier to squash that in patch 2/3
and add another note in the commit message?

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

Re: [PATCH mptcp-next v3 2/3] selftests: mptcp: add mptcp_lib_ns_* helpers

[Matthieu Baerts]

Hi Geliang,

On 21/02/2024 8:06 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add helpers mptcp_lib_ns_init() and mptcp_lib_ns_exit() in mptcp_lib.sh to
> initialize and delete the given namespaces. Then every test script can
> invoke these helpers and use all namespaces.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh     |  9 +++----
>  .../selftests/net/mptcp/mptcp_connect.sh      | 24 +++++++------------
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 11 ++-------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 23 ++++++++++++++++++
>  .../selftests/net/mptcp/mptcp_sockopt.sh      | 16 ++++---------
>  .../testing/selftests/net/mptcp/pm_netlink.sh |  9 +++----
>  .../selftests/net/mptcp/simult_flows.sh       | 16 ++++---------
>  .../selftests/net/mptcp/userspace_pm.sh       | 14 ++++-------
>  8 files changed, 54 insertions(+), 68 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 60a7009ce1b5..162d7b90c3f5 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -3,9 +3,8 @@
>  
>  . "$(dirname "${0}")/mptcp_lib.sh"
>  
> -sec=$(date +%s)
> -rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -ns="ns1-$rndh"
> +ns=""
> +mptcp_lib_ns_init ns

Please keep the init where the netns was created before: typically after
having declared the trap. Otherwise, the netns could be created, but not
cleaned in case of issues in between.

>  ksft_skip=4
>  test_cnt=1
>  timeout_poll=30

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index b609649311f6..6900f7f4d88d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -121,12 +121,11 @@ while getopts "$optstring" option;do
>  	esac
>  done
>  
> -sec=$(date +%s)
> -rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -ns1="ns1-$rndh"
> -ns2="ns2-$rndh"
> -ns3="ns3-$rndh"
> -ns4="ns4-$rndh"
> +ns1=""
> +ns2=""
> +ns3=""
> +ns4=""
> +mptcp_lib_ns_init ns1 ns2 ns3 ns4

Same here: move it after the trap.

>  
>  TEST_COUNT=0
>  TEST_GROUP=""
> @@ -140,9 +139,9 @@ cleanup()
>  
>  	local netns
>  	for netns in "$ns1" "$ns2" "$ns3" "$ns4";do
> -		ip netns del $netns
>  		rm -f /tmp/$netns.{nstat,out}

My previous comment was maybe not clear enough, sorry: for me, it is
fine to move this to 'mptcp_lib_ns_exit', just please clearly mention it
in the commit message as it is a behaviour change, e.g.

  Note that 'mptcp_lib_ns_exit' now also try to remove temp netns files
  used for the stats even for selftests not using them. That's fine to
  do that because these files have a unique name.

>  	done
> +	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
>  }
>  
>  mptcp_lib_check_mptcp

(...)

> @@ -251,8 +245,8 @@ fi
>  
>  check_mptcp_disabled()
>  {
> -	local disabled_ns="ns_disabled-$rndh"
> -	ip netns add ${disabled_ns} || exit $ksft_skip
> +	local disabled_ns=""

(note that you don't need '=""', 'local disabled_ns' is enough ; but I'm
fine either ways, the result is the same)

> +	mptcp_lib_ns_init disabled_ns
>  
>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 9f1476f0e2ae..bea7627c7313 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -86,17 +86,10 @@ init_partial()
>  {
>  	capout=$(mktemp)
>  
> -	local sec rndh
> -	sec=$(date +%s)
> -	rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -
> -	ns1="ns1-$rndh"
> -	ns2="ns2-$rndh"
> +	mptcp_lib_ns_init ns1 ns2
>  
>  	local netns
>  	for netns in "$ns1" "$ns2"; do
> -		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.mptcp.pm_type=0 2>/dev/null || true
>  		ip netns exec $netns sysctl -q net.ipv4.conf.all.rp_filter=0

Same here, I'm fine to move this in 'mptcp_lib_ns_init' as long as it is
documented in the commit message to explain that "it is fine to do that
everywhere, because (...)".

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 556a7d9784d7..14c0b97a153e 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -373,3 +373,26 @@ mptcp_lib_check_output() {
>  		return 1
>  	fi
>  }
> +
> +mptcp_lib_ns_init() {
> +	local sec
> +	local rndh=""

(detail: same here, it is enough to have 'local rndh' without '=""')

> +
> +	sec=$(date +%s)
> +	rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
> +
> +	local netns
> +	for netns in "${@}"; do
> +		eval "${netns}=${netns}-${rndh}"
> +
> +		ip netns add "${!netns}" || exit ${KSFT_SKIP}
> +		ip -net "${!netns}" link set lo up
> +	done
> +}
> +
> +mptcp_lib_ns_exit() {
> +	local netns
> +	for netns in "${@}"; do
> +		ip netns del "$netns"
> +	done
> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index fd7de1b3dc55..58bf83d4e7fa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -14,11 +14,10 @@ timeout_test=$((timeout_poll * 2 + 1))
>  iptables="iptables"
>  ip6tables="ip6tables"
>  
> -sec=$(date +%s)
> -rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -ns1="ns1-$rndh"
> -ns2="ns2-$rndh"
> -ns_sbox="ns_sbox-$rndh"
> +ns1=""
> +ns2=""
> +ns_sbox=""
> +mptcp_lib_ns_init ns1 ns2 ns_sbox

Same here: move it to the 'init()' function, where it was done

>  add_mark_rules()
>  {
> @@ -42,8 +41,6 @@ init()
>  {
>  	local netns
>  	for netns in "$ns1" "$ns2" "$ns_sbox";do
> -		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
> @@ -79,10 +76,7 @@ init()
>  
>  cleanup()
>  {
> -	local netns
> -	for netns in "$ns1" "$ns2" "$ns_sbox"; do
> -		ip netns del $netns
> -	done
> +	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
>  	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 1ec9d8622fc9..c117eea92b6a 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -24,15 +24,14 @@ while getopts "$optstring" option;do
>  	esac
>  done
>  
> -sec=$(date +%s)
> -rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -ns1="ns1-$rndh"
> +ns1=""
> +mptcp_lib_ns_init ns1

Same here: move it after the trap.

>  err=$(mktemp)
>  
>  cleanup()
>  {
>  	rm -f $err
> -	ip netns del $ns1
> +	mptcp_lib_ns_exit "${ns1}"
>  }
>  
>  mptcp_lib_check_mptcp
> @@ -40,8 +39,6 @@ mptcp_lib_check_tools ip
>  
>  trap cleanup EXIT
>  
> -ip netns add $ns1 || exit $ksft_skip
> -ip -net $ns1 link set lo up
>  ip netns exec $ns1 sysctl -q net.mptcp.enabled=1
>  
>  check()
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 7d8388ecc966..67cddd40f211 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -3,11 +3,10 @@
>  
>  . "$(dirname "${0}")/mptcp_lib.sh"
>  
> -sec=$(date +%s)
> -rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> -ns1="ns1-$rndh"
> -ns2="ns2-$rndh"
> -ns3="ns3-$rndh"
> +ns1=""
> +ns2=""
> +ns3=""
> +mptcp_lib_ns_init ns1 ns2 ns3

Same here: move it after the trap.

>  capture=false
>  ksft_skip=4
>  timeout_poll=30
> @@ -36,10 +35,7 @@ cleanup()
>  	rm -f "$large" "$small"
>  	rm -f "$capout"
>  
> -	local netns
> -	for netns in "$ns1" "$ns2" "$ns3";do
> -		ip netns del $netns
> -	done
> +	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
>  }
>  
>  mptcp_lib_check_mptcp
> @@ -66,8 +62,6 @@ setup()
>  	trap cleanup EXIT
>  
>  	for i in "$ns1" "$ns2" "$ns3";do
> -		ip netns add $i || exit $ksft_skip
> -		ip -net $i link set lo up
>  		ip netns exec $i sysctl -q net.ipv4.conf.all.rp_filter=0
>  		ip netns exec $i sysctl -q net.ipv4.conf.default.rp_filter=0
>  	done
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 629fc5d0ecc5..10074709420a 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -50,10 +50,9 @@ app6_port=50004
>  client_addr_id=${RANDOM:0:2}
>  server_addr_id=${RANDOM:0:2}
>  
> -sec=$(date +%s)
> -rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
> -ns1="ns1-$rndh"
> -ns2="ns2-$rndh"
> +ns1=""
> +ns2=""
> +mptcp_lib_ns_init ns1 ns2

Same here: move it after the trap.

>  ret=0
>  test_name=""
>  
> @@ -118,10 +117,7 @@ cleanup()
>  		mptcp_lib_kill_wait $pid
>  	done
>  
> -	local netns
> -	for netns in "$ns1" "$ns2" ;do
> -		ip netns del "$netns"
> -	done
> +	mptcp_lib_ns_exit "${ns1}" "${ns2}"
>  
>  	rm -rf $file $client_evts $server_evts
>  
> @@ -132,8 +128,6 @@ trap cleanup EXIT
>  
>  # Create and configure network namespaces for testing
>  for i in "$ns1" "$ns2" ;do
> -	ip netns add "$i" || exit 1
> -	ip -net "$i" link set lo up
>  	ip netns exec "$i" sysctl -q net.mptcp.enabled=1
>  	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
>  done

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

Re: [PATCH mptcp-next v3 3/3] selftests: mptcp: add mptcp_lib_evts_* helpers

[Matthieu Baerts]

Hi Geliang,

On 21/02/2024 8:06 am, 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.
> 
> This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
> and mptcp_join.sh into four helpers: mptcp_lib_evts_init(), _start(),
> _kill() and _remove(). Define them in mptcp_lib.sh and use these new
> helpers in both scripts.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 16 ++----
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 55 +++++++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 28 +++-------
>  3 files changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index bea7627c7313..006fefaf2e2a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -158,8 +158,7 @@ init() {
>  	cinsent=$(mktemp)
>  	cout=$(mktemp)
>  	err=$(mktemp)
> -	evts_ns1=$(mktemp)
> -	evts_ns2=$(mktemp)
> +	mptcp_lib_evts_init evts_ns1 evts_ns2

Maybe easier to keep this unchanged? All you do in the lib is:

  <var>=$(mktemp)

Same for the mptcp_lib_evts_remove -- just a 'rm' -- and
mptcp_lib_evts_kill -- just calling mptcp_lib_kill_wait.

Or do you see other advantages?

>  	trap cleanup EXIT
>  

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 14c0b97a153e..9573edb4efb1 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -396,3 +396,58 @@ mptcp_lib_ns_exit() {
>  		ip netns del "$netns"
>  	done
>  }
> +
> +mptcp_lib_evts_init() {
> +	local arg
> +
> +	for arg in "${@}"; do
> +		declare -n evts="${arg}"
> +
> +		if [ -z "${evts}" ]; then
> +			evts=$(mktemp)
> +		fi
> +	done
> +}

(see above, maybe we don't need this helper?)

> +
> +# $1 ns1, $2 ns2
> +mptcp_lib_evts_start() {
> +	local ns_1="${1}"
> +	local ns_2="${2}"
> +	local evts_1="${3}"
> +	local evts_2="${4}"
> +	declare -n pid_1="${5}"
> +	declare -n pid_2="${6}"

(or use ${!pid_1} as in mptcp_lib_ns_init -- or we use 'declare -n'
there too, just for consistency)

(I hope 'declare -n' will respect the 'local' scope, but I guess yes)

> +
> +	:>"${evts_1}"
> +	:>"${evts_2}"
> +
> +	if [ ${pid_1} -ne 0 ]; then
> +		mptcp_lib_kill_wait "${pid_1}"
> +	fi
> +	ip netns exec "${ns_1}" ./pm_nl_ctl events >> "${evts_1}" 2>&1 &
> +	pid_1=$!
> +
> +	if [ ${pid_2} -ne 0 ]; then
> +		mptcp_lib_kill_wait "${pid_2}"
> +	fi
> +	ip netns exec "${ns_2}" ./pm_nl_ctl events >> "${evts_2}" 2>&1 &
> +	pid_2=$!
> +}
> +
> +mptcp_lib_evts_kill() {
> +	declare -n pid_1="${1}"
> +	declare -n pid_2="${2}"
> +
> +	mptcp_lib_kill_wait "${pid_1}"
> +	mptcp_lib_kill_wait "${pid_2}"
> +
> +	pid_1=0
> +	pid_2=0

This was not done before, do you need that?

(but see above, maybe we don't need this helper, and the one below)

> +}
> +
> +mptcp_lib_evts_remove() {
> +	local evts_1="${1}"
> +	local evts_2="${2}"
> +
> +	rm -rf "${evts_1}" "${evts_2}"
> +}

(...)

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

Re: selftests: mptcp: add mptcp_lib_evts_* helpers: 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:
  - Unstable: 2 failed test(s): packetdrill_regressions selftest_mptcp_join 🔴:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7990465311

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


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)