From: Matthieu Baerts <matttbe@kernel.org>
To: Geliang Tang <geliang@kernel.org>, mptcp@lists.linux.dev
Cc: Geliang Tang <tanggeliang@kylinos.cn>
Subject: Re: [PATCH mptcp-next v3 2/3] selftests: mptcp: add mptcp_lib_ns_* helpers
Date: Wed, 21 Feb 2024 13:16:24 +0100 [thread overview]
Message-ID: <03676f8b-341a-4185-9fd2-46dcd0da21d0@kernel.org> (raw)
In-Reply-To: <fef66a388379bd499df11a08a9492bb583210f47.1708499011.git.tanggeliang@kylinos.cn>
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.
next prev parent reply other threads:[~2024-02-21 12:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-21 7:06 [PATCH mptcp-next v3 0/3] add helpers and vars in mptcp_lib.sh, part 2 Geliang Tang
2024-02-21 7:06 ` [PATCH mptcp-next v3 1/3] selftests: mptcp: connect: add local variable rndh Geliang Tang
2024-02-21 12:15 ` Matthieu Baerts
2024-02-21 7:06 ` [PATCH mptcp-next v3 2/3] selftests: mptcp: add mptcp_lib_ns_* helpers Geliang Tang
2024-02-21 12:16 ` Matthieu Baerts [this message]
2024-02-21 7:06 ` [PATCH mptcp-next v3 3/3] selftests: mptcp: add mptcp_lib_evts_* helpers Geliang Tang
2024-02-21 12:16 ` Matthieu Baerts
2024-02-21 14:54 ` selftests: mptcp: add mptcp_lib_evts_* helpers: Tests Results MPTCP CI
2024-02-21 12:14 ` [PATCH mptcp-next v3 0/3] add helpers and vars in mptcp_lib.sh, part 2 Matthieu Baerts
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=03676f8b-341a-4185-9fd2-46dcd0da21d0@kernel.org \
--to=matttbe@kernel.org \
--cc=geliang@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=tanggeliang@kylinos.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox