Hi Geliang,
On 09/08/2023 09:06, Geliang Tang wrote:
> This patch adds selftests for userpsace PM to remove id 0 subflow and
> id 0 address.
>
> A new helper userspace_pm_rm_id_0_subflow_or_address_ns2() is added,
> in it use
>
> ./pm_nl_ctl dsf token $tk id 0
>
> to remove id 0 subflow, and use
>
> ./pm_nl_ctl rem token $tk id 0
>
> to remove id 0 address.
See my comment on patch 2/4: OK for the "./pm_nl_ctl rem token $tk id 0"
but not for "dsf id 0" for -net to fix issue #379.
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> .../testing/selftests/net/mptcp/mptcp_join.sh | 49 +++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ee1f89a872b3..52f081738c36 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3320,6 +3320,15 @@ userspace_pm_rm_sf_addr_ns2()
> wait_rm_sf $ns2 1
> }
>
> +userspace_pm_rm_id_0_subflow_or_address_ns2()
It might be useful to either add a comment about what the parameter are
(# $1: command (rem/dsf)) and/or use an dedicated local env var (local
cmd=${1}) otherwise we need to read the code to know which parameters
need to be given.
> +{
> + local tk
> +
> + tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
> + ip netns exec $ns2 ./pm_nl_ctl "$1" token $tk id 0
> + sleep 0.5
> +}
> +
> userspace_tests()
> {
> # userspace pm type prevents add_addr
> @@ -3434,6 +3443,46 @@ userspace_tests()
> kill_events_pids
> wait $tests_pid
> fi
> +
> + # userspace pm remove id 0 subflow
> + if reset_with_events "userspace pm remove id 0 subflow" &&
> + continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> + set_userspace_pm $ns2
> + pm_nl_set_limits $ns1 0 2
> + speed=10 \
> + run_tests $ns1 $ns2 10.0.1.1 &
> + local tests_pid=$!
> + wait_mpj $ns2
> + userspace_pm_add_sf 10.0.1.2 0
Is it not going to create a subflow using the same IPs as the original
subflow? Is it on purpose? It looks confusing.
> + userspace_pm_add_sf 10.0.3.2 20
> + chk_join_nr 2 2 2
> + chk_mptcp_info subflows 2 subflows 2
> + userspace_pm_rm_id_0_subflow_or_address_ns2 dsf
Linked to my comment on patch 2/4, here we should do something like:
> ip netns exec $ns2 ./pm_nl_ctl dsf token $tk lip $sa lport $sp rip $da rport $dp token $tk
where the source IP + port and destination IP + port are the ones from
the initial connection (10.0.1.2 -> 10.0.1.1?)
WDYT?
> + chk_mptcp_info subflows 1 subflows 1
> + chk_rm_nr 0 2
> + kill_events_pids
> + wait $tests_pid
> + fi
> +
> + # userspace pm remove id 0 address
> + if reset_with_events "userspace pm remove id 0 address" &&
> + continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> + set_userspace_pm $ns2
> + pm_nl_set_limits $ns1 0 2
> + speed=10 \
> + run_tests $ns1 $ns2 10.0.1.1 &
> + local tests_pid=$!
> + wait_mpj $ns2
> + userspace_pm_add_sf 10.0.1.2 0
(same here: are you not creating a new subflow with the same IPs as the
original one? It might be confusing, no? But maybe on purpose? In this
case, please add a comment)
> + userspace_pm_add_sf 10.0.3.2 20
> + chk_join_nr 2 2 2
> + chk_mptcp_info subflows 2 subflows 2
> + userspace_pm_rm_id_0_subflow_or_address_ns2 rem
> + chk_mptcp_info subflows 1 subflows 1
> + chk_rm_nr 2 0
> + kill_events_pids
> + wait $tests_pid
> + fi
> }
>
> endpoint_tests()
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net