* [PATCH mptcp-next 1/5] selftests: mptcp: unify namespace names to ns1/2/3/4
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
@ 2024-02-19 9:29 ` Geliang Tang
2024-02-19 12:21 ` Matthieu Baerts
2024-02-19 9:29 ` [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers Geliang Tang
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2024-02-19 9:29 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Most scripts use ns1, ns2, ns3 and ns4 as namespace names, but ns and
ns_sbox are used in diag.sh and mptcp_sockopt.sh. To maintain consistency
with other scripts, this patch renames these variables:
ns -> ns1 in diag.sh
ns_sbox -> ns3 in mptcp_sockopt.sh
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/net/mptcp/diag.sh | 48 +++++++++----------
.../selftests/net/mptcp/mptcp_sockopt.sh | 12 ++---
2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 60a7009ce1b5..e91dde816543 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -5,7 +5,7 @@
sec=$(date +%s)
rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns="ns1-$rndh"
+ns1="ns1-$rndh"
ksft_skip=4
test_cnt=1
timeout_poll=30
@@ -18,19 +18,19 @@ flush_pids()
# give it some time
sleep 1.1
- ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGUSR1 &>/dev/null
+ ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGUSR1 &>/dev/null
for _ in $(seq 10); do
- [ -z "$(ip netns pids "${ns}")" ] && break
+ [ -z "$(ip netns pids "${ns1}")" ] && break
sleep 0.1
done
}
cleanup()
{
- ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
+ ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
- ip netns del $ns
+ ip netns del $ns1
}
mptcp_lib_check_mptcp
@@ -38,7 +38,7 @@ mptcp_lib_check_tools ip ss
get_msk_inuse()
{
- ip netns exec $ns cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
+ ip netns exec $ns1 cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
}
__chk_nr()
@@ -73,7 +73,7 @@ __chk_msk_nr()
local condition=$1
shift 1
- __chk_nr "ss -inmHMN $ns | $condition" "$@"
+ __chk_nr "ss -inmHMN $ns1 | $condition" "$@"
}
chk_msk_nr()
@@ -94,7 +94,7 @@ wait_msk_nr()
msg=$*
while [ $i -lt $timeout ]; do
- nr=$(ss -inmHMN $ns | $condition)
+ nr=$(ss -inmHMN $ns1 | $condition)
[ $nr == $expected ] && break;
[ $nr -gt $max ] && max=$nr
i=$((i + 1))
@@ -133,7 +133,7 @@ __chk_listen()
local expected=$2
local msg="$3"
- __chk_nr "ss -N $ns -Ml '$filter' | grep -c LISTEN" "$expected" "$msg" 0
+ __chk_nr "ss -N $ns1 -Ml '$filter' | grep -c LISTEN" "$expected" "$msg" 0
}
chk_msk_listen()
@@ -163,7 +163,7 @@ chk_msk_inuse()
msg+=" after flush"
fi
- listen_nr=$(ss -N "${ns}" -Ml | grep -c LISTEN)
+ listen_nr=$(ss -N "${ns1}" -Ml | grep -c LISTEN)
expected=$((expected + listen_nr))
for _ in $(seq 10); do
@@ -186,7 +186,7 @@ chk_msk_cestab()
msg+=" after flush"
fi
- __chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" \
+ __chk_nr "mptcp_lib_get_counter ${ns1} MPTcpExtMPCurrEstab" \
"${expected}" "${msg}" ""
}
@@ -205,24 +205,24 @@ wait_connected()
}
trap cleanup EXIT
-ip netns add $ns
-ip -n $ns link set dev lo up
+ip netns add $ns1
+ip -n $ns1 link set dev lo up
echo "a" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p 10000 -l -t ${timeout_poll} -w 20 \
0.0.0.0 >/dev/null &
-mptcp_lib_wait_local_port_listen $ns 10000
+mptcp_lib_wait_local_port_listen $ns1 10000
chk_msk_nr 0 "no msk on netns creation"
chk_msk_listen 10000
echo "b" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p 10000 -r 0 -t ${timeout_poll} -w 20 \
127.0.0.1 >/dev/null &
-wait_connected $ns 10000
+wait_connected $ns1 10000
chk_msk_nr 2 "after MPC handshake "
chk_msk_remote_key_nr 2 "....chk remote_key"
chk_msk_fallback_nr 0 "....chk no fallback"
@@ -235,16 +235,16 @@ chk_msk_cestab 0 "2->0"
echo "a" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p 10001 -l -s TCP -t ${timeout_poll} -w 20 \
0.0.0.0 >/dev/null &
-mptcp_lib_wait_local_port_listen $ns 10001
+mptcp_lib_wait_local_port_listen $ns1 10001
echo "b" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p 10001 -r 0 -t ${timeout_poll} -w 20 \
127.0.0.1 >/dev/null &
-wait_connected $ns 10001
+wait_connected $ns1 10001
chk_msk_fallback_nr 1 "check fallback"
chk_msk_inuse 1
chk_msk_cestab 1
@@ -257,16 +257,16 @@ NR_CLIENTS=100
for I in `seq 1 $NR_CLIENTS`; do
echo "a" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p $((I+10001)) -l -w 20 \
-t ${timeout_poll} 0.0.0.0 >/dev/null &
done
-mptcp_lib_wait_local_port_listen $ns $((NR_CLIENTS + 10001))
+mptcp_lib_wait_local_port_listen $ns1 $((NR_CLIENTS + 10001))
for I in `seq 1 $NR_CLIENTS`; do
echo "b" | \
timeout ${timeout_test} \
- ip netns exec $ns \
+ ip netns exec $ns1 \
./mptcp_connect -p $((I+10001)) -w 20 \
-t ${timeout_poll} 127.0.0.1 >/dev/null &
done
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index fd7de1b3dc55..eb06c3f6184b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -18,7 +18,7 @@ sec=$(date +%s)
rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
ns1="ns1-$rndh"
ns2="ns2-$rndh"
-ns_sbox="ns_sbox-$rndh"
+ns3="ns3-$rndh"
add_mark_rules()
{
@@ -41,7 +41,7 @@ add_mark_rules()
init()
{
local netns
- for netns in "$ns1" "$ns2" "$ns_sbox";do
+ for netns in "$ns1" "$ns2" "$ns3";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
@@ -80,7 +80,7 @@ init()
cleanup()
{
local netns
- for netns in "$ns1" "$ns2" "$ns_sbox"; do
+ for netns in "$ns1" "$ns2" "$ns3"; do
ip netns del $netns
done
rm -f "$cin" "$cout"
@@ -223,7 +223,7 @@ do_mptcp_sockopt_tests()
return
fi
- ip netns exec "$ns_sbox" ./mptcp_sockopt
+ ip netns exec "$ns3" ./mptcp_sockopt
lret=$?
if [ $lret -ne 0 ]; then
@@ -234,7 +234,7 @@ do_mptcp_sockopt_tests()
fi
mptcp_lib_result_pass "sockopt v4"
- ip netns exec "$ns_sbox" ./mptcp_sockopt -6
+ ip netns exec "$ns3" ./mptcp_sockopt -6
lret=$?
if [ $lret -ne 0 ]; then
@@ -265,7 +265,7 @@ run_tests()
do_tcpinq_test()
{
- ip netns exec "$ns_sbox" ./mptcp_inq "$@"
+ ip netns exec "$ns3" ./mptcp_inq "$@"
local lret=$?
if [ $lret -ne 0 ];then
ret=$lret
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH mptcp-next 1/5] selftests: mptcp: unify namespace names to ns1/2/3/4
2024-02-19 9:29 ` [PATCH mptcp-next 1/5] selftests: mptcp: unify namespace names to ns1/2/3/4 Geliang Tang
@ 2024-02-19 12:21 ` Matthieu Baerts
0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:21 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
Hi Geliang,
On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Most scripts use ns1, ns2, ns3 and ns4 as namespace names, but ns and
> ns_sbox are used in diag.sh and mptcp_sockopt.sh. To maintain consistency
> with other scripts, this patch renames these variables:
If it is just for consistency, I don't think we should rename these
variables:
> ns -> ns1 in diag.sh
For me, adding a number as suffix means there are multiple netns
interacting with each others. Here, there is only one, that would be
confusing.
If this rename is not needed to fix something, I would then prefer to
avoid such modifications: that will "annoy" us in case of backports →
more risks of having conflicts, but mainly backports done by the stable
team will not detect issues where the wrong variable is used (the stable
team only compiles the .c code).
> ns_sbox -> ns3 in mptcp_sockopt.sh
To be honest, I'm not a big fan of having numbers as suffix: when
looking at the names, it is not clear what is the difference between
'ns1' and 'ns2' for example. We should have probably used "_client",
"_server", "_router", "_shaper", etc. from the beginning. Too late now.
Hence, I would prefer here to keep 'ns_sbox', otherwise we might think
that ns3 is connected to ns1 and/or ns2, which is not the case.
WDYT?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
2024-02-19 9:29 ` [PATCH mptcp-next 1/5] selftests: mptcp: unify namespace names to ns1/2/3/4 Geliang Tang
@ 2024-02-19 9:29 ` Geliang Tang
2024-02-19 12:21 ` Matthieu Baerts
2024-02-19 9:29 ` [PATCH mptcp-next 3/5] selftests: mptcp: add mptcp_lib_cleanup helper Geliang Tang
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2024-02-19 9:29 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Add helpers mptcp_lib_ns_init() and mptcp_lib_ns_exit() in mptcp_lib.sh to
init all namespaces ns1, ns2, ns3 and ns4. 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 | 8 +--
.../selftests/net/mptcp/mptcp_connect.sh | 20 ++-----
.../testing/selftests/net/mptcp/mptcp_join.sh | 19 +-----
.../testing/selftests/net/mptcp/mptcp_lib.sh | 59 +++++++++++++++++++
.../selftests/net/mptcp/mptcp_sockopt.sh | 15 +----
.../testing/selftests/net/mptcp/pm_netlink.sh | 8 +--
.../selftests/net/mptcp/simult_flows.sh | 18 +-----
.../selftests/net/mptcp/userspace_pm.sh | 12 +---
8 files changed, 75 insertions(+), 84 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index e91dde816543..40fc04e8e592 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -3,9 +3,7 @@
. "$(dirname "${0}")/mptcp_lib.sh"
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
+mptcp_lib_ns_init 1
ksft_skip=4
test_cnt=1
timeout_poll=30
@@ -30,7 +28,7 @@ cleanup()
{
ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
- ip netns del $ns1
+ mptcp_lib_ns_exit 1
}
mptcp_lib_check_mptcp
@@ -205,8 +203,6 @@ wait_connected()
}
trap cleanup EXIT
-ip netns add $ns1
-ip -n $ns1 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 ea52110c3fbc..00527f4c3b98 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -121,12 +121,7 @@ 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"
+mptcp_lib_ns_init
TEST_COUNT=0
TEST_GROUP=""
@@ -138,11 +133,7 @@ cleanup()
rm -f "$sin" "$sout"
rm -f "$capout"
- 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
}
mptcp_lib_check_mptcp
@@ -158,11 +149,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,6 +237,8 @@ fi
check_mptcp_disabled()
{
+ : "${rndh:?}"
+
local disabled_ns="ns_disabled-$rndh"
ip netns add ${disabled_ns} || exit $ksft_skip
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a8d08554efc9..7e354ff5d717 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -23,8 +23,6 @@ tmpfile=""
cout=""
check_output_err=""
capout=""
-ns1=""
-ns2=""
ksft_skip=4
iptables="iptables"
ip6tables="ip6tables"
@@ -86,21 +84,12 @@ 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 2
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
- ip netns exec $netns sysctl -q net.ipv4.conf.default.rp_filter=0
if $checksum; then
ip netns exec $netns sysctl -q net.mptcp.checksum_enabled=1
fi
@@ -145,11 +134,7 @@ cleanup_partial()
{
rm -f "$capout"
- local netns
- for netns in "$ns1" "$ns2"; do
- ip netns del $netns
- rm -f /tmp/$netns.{nstat,out}
- done
+ mptcp_lib_ns_exit 2
}
init() {
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 6d9a2af85a8d..bfc535594675 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -350,3 +350,62 @@ mptcp_lib_check_tools() {
esac
done
}
+
+rndh=""
+ns1=""
+ns2=""
+ns3=""
+ns4=""
+
+mptcp_lib_ns_init() {
+ : "${rndh?}"
+ : "${ns1?}"
+ : "${ns2?}"
+ : "${ns3?}"
+ : "${ns4?}"
+
+ local nr="${1:-4}"
+ local i=1
+ local sec
+
+ sec=$(date +%s)
+ rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
+
+ ns1="ns1-$rndh"
+ ns2="ns2-$rndh"
+ ns3="ns3-$rndh"
+ ns4="ns4-$rndh"
+
+ local netns
+ for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
+ if [ "$i" -gt "$nr" ]; then
+ break
+ fi
+ ip netns add "$netns" || exit ${KSFT_SKIP}
+ ip -net "$netns" link set lo up
+
+ 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
+ i=$((i + 1))
+ done
+}
+
+mptcp_lib_ns_exit() {
+ : "${ns1:?}"
+ : "${ns2:?}"
+ : "${ns3:?}"
+ : "${ns4:?}"
+
+ local nr="${1:-4}"
+ local i=1
+
+ local netns
+ for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
+ if [ "$i" -gt "$nr" ]; then
+ break
+ fi
+ ip netns del "$netns"
+ rm -f /tmp/"$netns".{nstat,out}
+ i=$((i + 1))
+ done
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index eb06c3f6184b..5d8881d11fb0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -14,11 +14,7 @@ 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"
-ns3="ns3-$rndh"
+mptcp_lib_ns_init 3
add_mark_rules()
{
@@ -42,11 +38,7 @@ init()
{
local netns
for netns in "$ns1" "$ns2" "$ns3";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
done
local i
@@ -79,10 +71,7 @@ init()
cleanup()
{
- local netns
- for netns in "$ns1" "$ns2" "$ns3"; do
- ip netns del $netns
- done
+ mptcp_lib_ns_exit 3
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 cb6ea67e688b..2ea80f1b0aee 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -24,15 +24,13 @@ while getopts "$optstring" option;do
esac
done
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
+mptcp_lib_ns_init 1
err=$(mktemp)
cleanup()
{
rm -f $err
- ip netns del $ns1
+ mptcp_lib_ns_exit 1
}
mptcp_lib_check_mptcp
@@ -40,8 +38,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..a45b6ad41aea 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -3,11 +3,7 @@
. "$(dirname "${0}")/mptcp_lib.sh"
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
-ns3="ns3-$rndh"
+mptcp_lib_ns_init 3
capture=false
ksft_skip=4
timeout_poll=30
@@ -36,10 +32,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 3
}
mptcp_lib_check_mptcp
@@ -65,13 +58,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
-
ip link add ns1eth1 netns "$ns1" type veth peer name ns2eth1 netns "$ns2"
ip link add ns1eth2 netns "$ns1" type veth peer name ns2eth2 netns "$ns2"
ip link add ns2eth3 netns "$ns2" type veth peer name ns3eth1 netns "$ns3"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 629fc5d0ecc5..71caa6b449a8 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -50,10 +50,7 @@ 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"
+mptcp_lib_ns_init 2
ret=0
test_name=""
@@ -118,10 +115,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 2
rm -rf $file $client_evts $server_evts
@@ -132,8 +126,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers
2024-02-19 9:29 ` [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers Geliang Tang
@ 2024-02-19 12:21 ` Matthieu Baerts
0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:21 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
Hi Geliang,
On 19/02/2024 10:29, 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
> init all namespaces ns1, ns2, ns3 and ns4. Then every test script can
> invoke these helpers and use all namespaces.
Good idea to move duplicated code to the _lib.sh file.
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
(...)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 6d9a2af85a8d..bfc535594675 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -350,3 +350,62 @@ mptcp_lib_check_tools() {
> esac
> done
> }
> +
> +rndh=""
I guess you "export" it because it is needed for the captured files,
right? If yes, why not using the "${ns}" variables in a dedicated commit
before to ease this refactoring?
> +ns1=""
> +ns2=""
> +ns3=""
> +ns4=""
I really don't think it is a good idea to hide these variables: if you
open 'mptcp_connect.sh', it will be confusing not to see where these
variables have been declared. If you declare variables (or functions) in
the lib, they should be prefixed MPTCP_LIB_ (or mptcp_lib_ for the
functions) I think.
Instead, I think you should keep the ns* variables declared at the top
of each file, but call "mptcp_lib_ns_init" with the variables to
initiate, e.g.
ns1=""
ns2=""
ns_disabled=""
(...)
mptcp_lib_ns_init ns1 ns2 ns_disabled
in mptcp_lib_ns_init(), you can do something like:
(...)
local ns_i
for ns_i in "${@}"; do
eval "${ns_i}=${ns_i}-${rndh}"
ip netns add "${!ns_i}" || (...)
(so you can use the version with '!' in the for loop)
> +mptcp_lib_ns_init() {
> + : "${rndh?}"
> + : "${ns1?}"
> + : "${ns2?}"
> + : "${ns3?}"
> + : "${ns4?}"
> +
> + local nr="${1:-4}"
> + local i=1
> + local sec
> +
> + sec=$(date +%s)
> + rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
> +
> + ns1="ns1-$rndh"
> + ns2="ns2-$rndh"
> + ns3="ns3-$rndh"
> + ns4="ns4-$rndh"
In this mptcp_lib.sh, probably good to continue to use {} around
variables ('${rndh}' instead of '$rndh') like in the rest of the file:
clearer, and we avoid typos.
> +
> + local netns
> + for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
> + if [ "$i" -gt "$nr" ]; then
> + break
> + fi
(see above)
> + ip netns add "$netns" || exit ${KSFT_SKIP}
> + ip -net "$netns" link set lo up
> +
> + 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
Mmh, is it OK to do that for all netns? We were not doing that before
for diag.sh, but I suppose that's because it was not communicating with
other netns, right? So fine to do that everywhere?
That's a behaviour change. Please always clearly mention behaviour
changes in the commit message when you do such refactoring, where we
would expect no behaviour changes.
While at it, maybe we want to add:
ip netns exec "${!ns_i}" sysctl -q net.mptcp.enabled=1
That should be the default value. If not, good to override. (I think it
is needed for some RHEL versions.) If yes, please add that in the commit
message as well.
> + i=$((i + 1))
> + done
> +}
> +
> +mptcp_lib_ns_exit() {
> + : "${ns1:?}"
> + : "${ns2:?}"
> + : "${ns3:?}"
> + : "${ns4:?}"
> +
> + local nr="${1:-4}"
> + local i=1
> +
> + local netns
> + for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
> + if [ "$i" -gt "$nr" ]; then
> + break
> + fi
Same here, best to clearer to pass the variables names and accept a
variable number of parameters:
mptcp_lib_ns_exit "${ns1}" "${ns2}"
local ns_i
for ns_i in "${@}"; do
ip netns del "${ns_i}"
> + ip netns del "$netns"
> + rm -f /tmp/"$netns".{nstat,out}
Here, it is also a behaviour change. I guess that's fine, but still,
please mention that in the commit message.
> + i=$((i + 1))
> + done
> +}
(...)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH mptcp-next 3/5] selftests: mptcp: add mptcp_lib_cleanup helper
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
2024-02-19 9:29 ` [PATCH mptcp-next 1/5] selftests: mptcp: unify namespace names to ns1/2/3/4 Geliang Tang
2024-02-19 9:29 ` [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers Geliang Tang
@ 2024-02-19 9:29 ` Geliang Tang
2024-02-19 12:21 ` Matthieu Baerts
2024-02-19 9:29 ` [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper Geliang Tang
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2024-02-19 9:29 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
This patch adds a new helper mptcp_lib_cleanup() in mptcp_lib.sh, it's
a public cleanup interface, being invoked in every cleanup() in all
scripts.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/net/mptcp/diag.sh | 1 +
tools/testing/selftests/net/mptcp/mptcp_connect.sh | 1 +
tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
tools/testing/selftests/net/mptcp/mptcp_lib.sh | 4 ++++
tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 1 +
tools/testing/selftests/net/mptcp/pm_netlink.sh | 1 +
tools/testing/selftests/net/mptcp/simult_flows.sh | 1 +
tools/testing/selftests/net/mptcp/userspace_pm.sh | 1 +
8 files changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 40fc04e8e592..bd6c8def8b5b 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -29,6 +29,7 @@ cleanup()
ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
mptcp_lib_ns_exit 1
+ mptcp_lib_cleanup
}
mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 00527f4c3b98..5273650a78f2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -134,6 +134,7 @@ cleanup()
rm -f "$capout"
mptcp_lib_ns_exit
+ mptcp_lib_cleanup
}
mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 7e354ff5d717..ceaaf155cc32 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -166,6 +166,7 @@ cleanup()
rm -f "$tmpfile"
rm -rf $evts_ns1 $evts_ns2
rm -f $check_output_err
+ mptcp_lib_cleanup
cleanup_partial
}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index bfc535594675..01b0e43eaa80 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -409,3 +409,7 @@ mptcp_lib_ns_exit() {
i=$((i + 1))
done
}
+
+mptcp_lib_cleanup() {
+ echo "cleanup"
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 5d8881d11fb0..f82553c56512 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -74,6 +74,7 @@ cleanup()
mptcp_lib_ns_exit 3
rm -f "$cin" "$cout"
rm -f "$sin" "$sout"
+ mptcp_lib_cleanup
}
mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 2ea80f1b0aee..5da9ccafc47e 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -31,6 +31,7 @@ cleanup()
{
rm -f $err
mptcp_lib_ns_exit 1
+ mptcp_lib_cleanup
}
mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index a45b6ad41aea..e9820aec7ffa 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -33,6 +33,7 @@ cleanup()
rm -f "$capout"
mptcp_lib_ns_exit 3
+ mptcp_lib_cleanup
}
mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 71caa6b449a8..dab9115738d7 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -119,6 +119,7 @@ cleanup()
rm -rf $file $client_evts $server_evts
+ mptcp_lib_cleanup
_printf "Done\n"
}
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH mptcp-next 3/5] selftests: mptcp: add mptcp_lib_cleanup helper
2024-02-19 9:29 ` [PATCH mptcp-next 3/5] selftests: mptcp: add mptcp_lib_cleanup helper Geliang Tang
@ 2024-02-19 12:21 ` Matthieu Baerts
0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:21 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
Hi Geliang,
On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch adds a new helper mptcp_lib_cleanup() in mptcp_lib.sh, it's
> a public cleanup interface, being invoked in every cleanup() in all
> scripts.
When looking at the commit alone, it is difficult to understand why it
is needed. In such cases, to help reviewers and dev later, please add
something like: This will be used in the next commit(s) for XXX.
(...)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index bfc535594675..01b0e43eaa80 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -409,3 +409,7 @@ mptcp_lib_ns_exit() {
> i=$((i + 1))
> done
> }
> +
> +mptcp_lib_cleanup() {
> + echo "cleanup"
Do we want to print this everywhere?
> +}
(...)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
` (2 preceding siblings ...)
2024-02-19 9:29 ` [PATCH mptcp-next 3/5] selftests: mptcp: add mptcp_lib_cleanup helper Geliang Tang
@ 2024-02-19 9:29 ` Geliang Tang
2024-02-19 12:21 ` Matthieu Baerts
2024-02-19 9:29 ` [PATCH mptcp-next 5/5] selftests: mptcp: add mptcp_lib_evts_* helpers Geliang Tang
2024-02-19 12:21 ` [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Matthieu Baerts
5 siblings, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2024-02-19 9:29 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Geliang Tang <tanggeliang@kylinos.cn>
Unify check_output() in mptcp_join.sh and check() in pm_netlink.sh into
a new public function mptcp_lib_check_output() in mptcp_lib.sh. And use
mptcp_lib_print_ok() and _err() in it to print test results with colors.
Use this new helper instead of check_output() and check().
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../testing/selftests/net/mptcp/mptcp_join.sh | 21 +-----------
.../testing/selftests/net/mptcp/mptcp_lib.sh | 32 +++++++++++++++++++
.../testing/selftests/net/mptcp/pm_netlink.sh | 24 ++++----------
3 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ceaaf155cc32..f75d22dbb28d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -21,7 +21,6 @@ cinfail=""
cinsent=""
tmpfile=""
cout=""
-check_output_err=""
capout=""
ksft_skip=4
iptables="iptables"
@@ -151,7 +150,6 @@ init() {
cout=$(mktemp)
evts_ns1=$(mktemp)
evts_ns2=$(mktemp)
- check_output_err=$(mktemp)
trap cleanup EXIT
@@ -165,7 +163,6 @@ cleanup()
rm -f "$sin" "$sout" "$cinsent" "$cinfail"
rm -f "$tmpfile"
rm -rf $evts_ns1 $evts_ns2
- rm -f $check_output_err
mptcp_lib_cleanup
cleanup_partial
}
@@ -3345,26 +3342,10 @@ userspace_pm_get_addr()
check_output()
{
- local cmd="$1"
- local expected="$2"
local msg="$3"
- local out=`$cmd 2>$check_output_err`
- local cmd_ret=$?
printf "%-42s" "$msg"
- if [ $cmd_ret -ne 0 ]; then
- mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
- cat $check_output_err
- ret=${KSFT_FAIL}
- return $cmd_ret
- elif [ "$out" = "$expected" ]; then
- mptcp_lib_print_ok "[ OK ]"
- return 0
- else
- mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
- ret=${KSFT_FAIL}
- return 1
- fi
+ mptcp_lib_check_output "${1}" "${2}"
}
userspace_tests()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 01b0e43eaa80..fd2ed3b5137c 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -410,6 +410,38 @@ mptcp_lib_ns_exit() {
done
}
+check_output_err=$(mktemp)
+
+mptcp_lib_check_output() {
+ : "${check_output_err:?}"
+ : "${ret:?}"
+
+ local cmd="$1"
+ local expected="$2"
+ local out
+ local cmd_ret
+
+ out=$($cmd 2>"$check_output_err")
+ cmd_ret=$?
+
+ if [ $cmd_ret -ne 0 ]; then
+ mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
+ cat "$check_output_err"
+ ret=${KSFT_FAIL}
+ return $cmd_ret
+ elif [ "$out" = "$expected" ]; then
+ mptcp_lib_print_ok "[ OK ]"
+ return 0
+ else
+ mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
+ ret=${KSFT_FAIL}
+ return 1
+ fi
+}
+
mptcp_lib_cleanup() {
+ : "${check_output_err:?}"
+
echo "cleanup"
+ rm -f "$check_output_err"
}
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 5da9ccafc47e..eb3440d2620e 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -25,11 +25,9 @@ while getopts "$optstring" option;do
done
mptcp_lib_ns_init 1
-err=$(mktemp)
cleanup()
{
- rm -f $err
mptcp_lib_ns_exit 1
mptcp_lib_cleanup
}
@@ -43,26 +41,18 @@ ip netns exec $ns1 sysctl -q net.mptcp.enabled=1
check()
{
- local cmd="$1"
- local expected="$2"
local msg="$3"
- local out=`$cmd 2>$err`
- local cmd_ret=$?
printf "%-50s" "$msg"
- if [ $cmd_ret -ne 0 ]; then
- echo "[FAIL] command execution '$cmd' stderr "
- cat $err
- mptcp_lib_result_fail "${msg} # error ${cmd_ret}"
- ret=1
- elif [ "$out" = "$expected" ]; then
- echo "[ OK ]"
+ # ${*} doesn't work here since there're spaces in some arguments.
+ mptcp_lib_check_output "${1}" "${2}"
+ local rc=$?
+ if [ ${rc} -eq 0 ]; then
mptcp_lib_result_pass "${msg}"
- else
- echo -n "[FAIL] "
- echo "expected '$expected' got '$out'"
+ elif [ ${rc} -eq 1 ]; then
mptcp_lib_result_fail "${msg} # different output"
- ret=1
+ else
+ mptcp_lib_result_fail "${msg} # error ${rc}"
fi
}
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper
2024-02-19 9:29 ` [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper Geliang Tang
@ 2024-02-19 12:21 ` Matthieu Baerts
0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:21 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Unify check_output() in mptcp_join.sh and check() in pm_netlink.sh into
> a new public function mptcp_lib_check_output() in mptcp_lib.sh. And use
> mptcp_lib_print_ok() and _err() in it to print test results with colors.
>
> Use this new helper instead of check_output() and check().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../testing/selftests/net/mptcp/mptcp_join.sh | 21 +-----------
> .../testing/selftests/net/mptcp/mptcp_lib.sh | 32 +++++++++++++++++++
> .../testing/selftests/net/mptcp/pm_netlink.sh | 24 ++++----------
> 3 files changed, 40 insertions(+), 37 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ceaaf155cc32..f75d22dbb28d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -21,7 +21,6 @@ cinfail=""
> cinsent=""
> tmpfile=""
> cout=""
> -check_output_err=""
Same as in a previous patch: to avoid some confusions, I think we should
keep global variables declared here and not in mptcp_lib.sh (or they
need to be prefixed).
(same in userspace_pm.sh)
> capout=""
> ksft_skip=4
> iptables="iptables"
> @@ -151,7 +150,6 @@ init() {
> cout=$(mktemp)
> evts_ns1=$(mktemp)
> evts_ns2=$(mktemp)
> - check_output_err=$(mktemp)
>
> trap cleanup EXIT
>
> @@ -165,7 +163,6 @@ cleanup()
> rm -f "$sin" "$sout" "$cinsent" "$cinfail"
> rm -f "$tmpfile"
> rm -rf $evts_ns1 $evts_ns2
> - rm -f $check_output_err
> mptcp_lib_cleanup
> cleanup_partial
> }
I think it is clearer to keep the assignment and cleanup here, than
hiding it in mptcp_lib.sh and doing that everywhere, even when not required.
(same in userspace_pm.sh)
> @@ -3345,26 +3342,10 @@ userspace_pm_get_addr()
>
> check_output()
> {
> - local cmd="$1"
> - local expected="$2"
Best to keep that, it is useful to act as a doc. Otherwise, we have to
look at mptcp_lib.sh to find out what is $1 and $2.
(same in userspace_pm.sh)
> local msg="$3"
> - local out=`$cmd 2>$check_output_err`
> - local cmd_ret=$?
>
> printf "%-42s" "$msg"
> - if [ $cmd_ret -ne 0 ]; then
> - mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
> - cat $check_output_err
> - ret=${KSFT_FAIL}
> - return $cmd_ret
> - elif [ "$out" = "$expected" ]; then
> - mptcp_lib_print_ok "[ OK ]"
> - return 0
> - else
> - mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
> - ret=${KSFT_FAIL}
> - return 1
> - fi
> + mptcp_lib_check_output "${1}" "${2}"
(see above: best to pass "${check_output_err}" to mptcp_lib_check_output)
(same in userspace_pm.sh)
> }
>
> userspace_tests()
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 01b0e43eaa80..fd2ed3b5137c 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -410,6 +410,38 @@ mptcp_lib_ns_exit() {
> done
> }
>
> +check_output_err=$(mktemp)
> +
> +mptcp_lib_check_output() {
> + : "${check_output_err:?}"
> + : "${ret:?}"
> +
> + local cmd="$1"
In mptcp_lib.sh, please always use {} as in the rest of the file. (same
below).
> + local expected="$2"
(see above: "check_output_err" will not be passed as argument)
> + local out
> + local cmd_ret
> +
> + out=$($cmd 2>"$check_output_err")
> + cmd_ret=$?
> +
> + if [ $cmd_ret -ne 0 ]; then
> + mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
> + cat "$check_output_err"
> + ret=${KSFT_FAIL}
Mmh, I don't think it is a good idea to assign a global variable
declared in another file from here.
Can you not return ${ret} instead? So we would do something like:
mptcp_lib_check_output (...) || ret=${?}
or:
if mptcp_lib_check_output (...); then
# OK (...)
else
ret=1
(...)
fi
(do you need the value of "${cmd_ret}" later? I think just having 0 or
${KSFT_FAIL} is enough, no?)
> + return $cmd_ret
> + elif [ "$out" = "$expected" ]; then
> + mptcp_lib_print_ok "[ OK ]"
> + return 0
> + else
> + mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
> + ret=${KSFT_FAIL}
> + return 1
> + fi
> +}
> +
> mptcp_lib_cleanup() {
> + : "${check_output_err:?}"
> +
> echo "cleanup"
> + rm -f "$check_output_err"
> }
As mentioned above, I don't think we should do that here. Maybe we don't
need patch 3/5 then?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH mptcp-next 5/5] selftests: mptcp: add mptcp_lib_evts_* helpers
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
` (3 preceding siblings ...)
2024-02-19 9:29 ` [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper Geliang Tang
@ 2024-02-19 9:29 ` Geliang Tang
2024-02-19 12:22 ` Matthieu Baerts
2024-02-19 12:21 ` [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Matthieu Baerts
5 siblings, 1 reply; 12+ messages in thread
From: Geliang Tang @ 2024-02-19 9:29 UTC (permalink / raw)
To: mptcp; +Cc: 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 | 62 +++++++-----------
.../testing/selftests/net/mptcp/mptcp_lib.sh | 63 +++++++++++++++++++
.../selftests/net/mptcp/userspace_pm.sh | 31 ++-------
3 files changed, 92 insertions(+), 64 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f75d22dbb28d..d4798495a17d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -33,10 +33,6 @@ ip_mptcp=0
check_invert=0
validate_checksum=false
init=0
-evts_ns1=""
-evts_ns2=""
-evts_ns1_pid=0
-evts_ns2_pid=0
last_test_failed=0
last_test_skipped=0
last_test_ignored=1
@@ -148,8 +144,7 @@ init() {
cin=$(mktemp)
cinsent=$(mktemp)
cout=$(mktemp)
- evts_ns1=$(mktemp)
- evts_ns2=$(mktemp)
+ mptcp_lib_evts_init
trap cleanup EXIT
@@ -162,7 +157,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
mptcp_lib_cleanup
cleanup_partial
}
@@ -427,12 +422,7 @@ 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}"
}
reset_with_tcp_filter()
@@ -602,12 +592,6 @@ wait_mpj()
done
}
-kill_events_pids()
-{
- mptcp_lib_kill_wait $evts_ns1_pid
- mptcp_lib_kill_wait $evts_ns2_pid
-}
-
pm_nl_set_limits()
{
local ns=$1
@@ -2877,9 +2861,9 @@ add_addr_ports_tests()
chk_add_nr 1 1 1
chk_rm_nr 1 1 invert
- verify_listener_events $evts_ns1 $LISTENER_CREATED $AF_INET 10.0.2.1 10100
- verify_listener_events $evts_ns1 $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
- kill_events_pids
+ verify_listener_events $server_evts $LISTENER_CREATED $AF_INET 10.0.2.1 10100
+ verify_listener_events $server_evts $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
+ mptcp_lib_evts_kill
fi
# subflow and signal with port, remove
@@ -3252,10 +3236,10 @@ fail_tests()
# $1: ns ; $2: addr ; $3: id
userspace_pm_add_addr()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local tk
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
tk=$(mptcp_lib_evts_get_info token "$evts")
ip netns exec $1 ./pm_nl_ctl ann $2 token $tk id $3
@@ -3265,11 +3249,11 @@ userspace_pm_add_addr()
# $1: ns ; $2: id
userspace_pm_rm_addr()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local tk
local cnt
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
tk=$(mptcp_lib_evts_get_info token "$evts")
cnt=$(rm_addr_count ${1})
@@ -3280,10 +3264,10 @@ userspace_pm_rm_addr()
# $1: ns ; $2: addr ; $3: id
userspace_pm_add_sf()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local tk da dp
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
tk=$(mptcp_lib_evts_get_info token "$evts")
da=$(mptcp_lib_evts_get_info daddr4 "$evts")
dp=$(mptcp_lib_evts_get_info dport "$evts")
@@ -3296,13 +3280,13 @@ userspace_pm_add_sf()
# $1: ns ; $2: addr $3: event type
userspace_pm_rm_sf()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local t=${3:-1}
local ip
local tk da dp sp
local cnt
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
[ -n "$(mptcp_lib_evts_get_info "saddr4" "$evts" $t)" ] && ip=4
[ -n "$(mptcp_lib_evts_get_info "saddr6" "$evts" $t)" ] && ip=6
tk=$(mptcp_lib_evts_get_info token "$evts")
@@ -3319,10 +3303,10 @@ userspace_pm_rm_sf()
# $1: ns
userspace_pm_dump()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local tk
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
tk=$(mptcp_lib_evts_get_info token "$evts")
ip netns exec $1 ./pm_nl_ctl dump token $tk
@@ -3331,10 +3315,10 @@ userspace_pm_dump()
# $1: ns ; $2: id
userspace_pm_get_addr()
{
- local evts=$evts_ns1
+ local evts=$server_evts
local tk
- [ "$1" == "$ns2" ] && evts=$evts_ns2
+ [ "$1" == "$ns2" ] && evts=$client_evts
tk=$(mptcp_lib_evts_get_info token "$evts")
ip netns exec $1 ./pm_nl_ctl get $2 token $tk
@@ -3468,7 +3452,7 @@ userspace_tests()
chk_rm_nr 2 2 invert
chk_mptcp_info subflows 0 subflows 0
chk_subflows_total 1 1
- kill_events_pids
+ mptcp_lib_evts_kill
mptcp_lib_kill_wait $tests_pid
fi
@@ -3505,7 +3489,7 @@ userspace_tests()
chk_rm_nr 1 1
chk_mptcp_info subflows 0 subflows 0
chk_subflows_total 1 1
- kill_events_pids
+ mptcp_lib_evts_kill
mptcp_lib_kill_wait $tests_pid
fi
@@ -3529,7 +3513,7 @@ userspace_tests()
chk_join_nr 1 1 1
chk_mptcp_info subflows 1 subflows 1
chk_subflows_total 2 2
- kill_events_pids
+ mptcp_lib_evts_kill
mptcp_lib_kill_wait $tests_pid
fi
@@ -3553,7 +3537,7 @@ userspace_tests()
chk_rst_nr 0 0 invert
chk_mptcp_info subflows 1 subflows 1
chk_subflows_total 1 1
- kill_events_pids
+ mptcp_lib_evts_kill
mptcp_lib_kill_wait $tests_pid
fi
@@ -3579,7 +3563,7 @@ userspace_tests()
chk_rst_nr 0 0 invert
chk_mptcp_info subflows 1 subflows 1
chk_subflows_total 1 1
- kill_events_pids
+ mptcp_lib_evts_kill
mptcp_lib_kill_wait $tests_pid
fi
}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index fd2ed3b5137c..67ceefe70059 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -439,6 +439,69 @@ mptcp_lib_check_output() {
fi
}
+server_evts=""
+client_evts=""
+server_evts_pid=0
+client_evts_pid=0
+
+# server_evts(_pid) and client_evts(_pid) are needed
+# by mptcp_lib_evts_init, _start, _kill and _remove.
+mptcp_lib_evts_init() {
+ : "${server_evts?}"
+ : "${client_evts?}"
+
+ if [ -z "${server_evts}" ]; then
+ server_evts=$(mktemp)
+ fi
+ if [ -z "${client_evts}" ]; then
+ client_evts=$(mktemp)
+ fi
+}
+
+# $1 ns1, $2 ns2
+mptcp_lib_evts_start() {
+ : "${server_evts:?}"
+ : "${client_evts:?}"
+ : "${server_evts_pid:?}"
+ : "${client_evts_pid:?}"
+
+ local ns_1="${1}"
+ local ns_2="${2}"
+
+ :>"$server_evts"
+ :>"$client_evts"
+
+ if [ "${server_evts_pid}" -ne 0 ]; then
+ mptcp_lib_kill_wait "${server_evts_pid}"
+ fi
+ ip netns exec "${ns_1}" ./pm_nl_ctl events >> "${server_evts}" 2>&1 &
+ server_evts_pid=$!
+
+ if [ "${client_evts_pid}" -ne 0 ]; then
+ mptcp_lib_kill_wait "${client_evts_pid}"
+ fi
+ ip netns exec "${ns_2}" ./pm_nl_ctl events >> "${client_evts}" 2>&1 &
+ client_evts_pid=$!
+}
+
+mptcp_lib_evts_kill() {
+ : "${server_evts_pid:?}"
+ : "${client_evts_pid:?}"
+
+ mptcp_lib_kill_wait "${server_evts_pid}"
+ mptcp_lib_kill_wait "${client_evts_pid}"
+
+ server_evts_pid=0
+ client_evts_pid=0
+}
+
+mptcp_lib_evts_remove() {
+ : "${server_evts:?}"
+ : "${client_evts:?}"
+
+ rm -rf "${server_evts}" "${client_evts}"
+}
+
mptcp_lib_cleanup() {
: "${check_output_err:?}"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index dab9115738d7..57fb990cefe9 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -30,10 +30,6 @@ AF_INET=2
AF_INET6=10
file=""
-server_evts=""
-client_evts=""
-server_evts_pid=0
-client_evts_pid=0
client4_pid=0
server4_pid=0
client6_pid=0
@@ -109,15 +105,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
mptcp_lib_ns_exit 2
- rm -rf $file $client_evts $server_evts
+ rm -rf $file
+ mptcp_lib_evts_remove
mptcp_lib_cleanup
_printf "Done\n"
@@ -175,24 +172,8 @@ 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
+ mptcp_lib_evts_start "${ns1}" "${ns2}"
sleep 0.5
# Run the server
--
2.40.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH mptcp-next 5/5] selftests: mptcp: add mptcp_lib_evts_* helpers
2024-02-19 9:29 ` [PATCH mptcp-next 5/5] selftests: mptcp: add mptcp_lib_evts_* helpers Geliang Tang
@ 2024-02-19 12:22 ` Matthieu Baerts
0 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:22 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
Hi Geliang,
On 19/02/2024 10:29, 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 | 62 +++++++-----------
> .../testing/selftests/net/mptcp/mptcp_lib.sh | 63 +++++++++++++++++++
> .../selftests/net/mptcp/userspace_pm.sh | 31 ++-------
> 3 files changed, 92 insertions(+), 64 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f75d22dbb28d..d4798495a17d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -33,10 +33,6 @@ ip_mptcp=0
> check_invert=0
> validate_checksum=false
> init=0
> -evts_ns1=""
> -evts_ns2=""
> -evts_ns1_pid=0
> -evts_ns2_pid=0
> last_test_failed=0
> last_test_skipped=0
> last_test_ignored=1
> @@ -148,8 +144,7 @@ init() {
> cin=$(mktemp)
> cinsent=$(mktemp)
> cout=$(mktemp)
> - evts_ns1=$(mktemp)
> - evts_ns2=$(mktemp)
> + mptcp_lib_evts_init
>
> trap cleanup EXIT
>
> @@ -162,7 +157,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
> mptcp_lib_cleanup
> cleanup_partial
> }
Same here, I think it is best to keep these global variables declared
here and pass their names to mptcp_lib_(...) functions.
By doing that, you can keep a different name in mptcp_join.sh and
userspace_pm.sh, and reduce the modifications here below.
(...)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2
2024-02-19 9:29 [PATCH mptcp-next 0/5] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
` (4 preceding siblings ...)
2024-02-19 9:29 ` [PATCH mptcp-next 5/5] selftests: mptcp: add mptcp_lib_evts_* helpers Geliang Tang
@ 2024-02-19 12:21 ` Matthieu Baerts
5 siblings, 0 replies; 12+ messages in thread
From: Matthieu Baerts @ 2024-02-19 12:21 UTC (permalink / raw)
To: Geliang Tang, mptcp; +Cc: Geliang Tang
Hi Geliang,
On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Depends on:
> - dump for userspace pm
> - add helpers and vars in mptcp_lib.sh, part 1
>
> Geliang Tang (5):
> selftests: mptcp: unify namespace names to ns1/2/3/4
> selftests: mptcp: add mptcp_lib_ns_* helpers
> selftests: mptcp: add mptcp_lib_cleanup helper
> selftests: mptcp: add mptcp_lib_check_output helper
> selftests: mptcp: add mptcp_lib_evts_* helpers
Good idea to reduce duplicated code between selftests. Still, I think we
should not go "too far", and hide the use of some variables and files.
For example, I think it is important not to have hidden global
variables, or declare it on one file, and use it elsewhere (except if
they are prefixed with the name of the file: MPTCP_LIB_xxx).
Please see my comments on the individual patches.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 12+ messages in thread