Re: [PATCH mptcp-next v13 09/14] selftests: mptcp: dump userspace addrs list

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 16/02/2024 08:42, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper userspace_pm_dump() to dump addresses
> for the userspace PM. Use this helper to check whether an ID 0 subflow
> is listed in the output of dump command after creating an ID 0 subflow
> in "userspace pm create id 0 subflow" test. Dump userspace PM addresses
> list in "userspace pm add & remove address" test and in "userspace pm
> create destroy subflow" test.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 6f7e83f61707..fb9b3ee9fee0 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3350,6 +3350,18 @@ userspace_pm_rm_sf()
>  	wait_rm_sf $1 "${cnt}"
>  }
>  
> +# $1: ns
> +userspace_pm_dump()
> +{
> +	local evts=$evts_ns1
> +	local tk
> +
> +	[ "$1" == "$ns2" ] && evts=$evts_ns2
> +	tk=$(mptcp_lib_evts_get_info token "$evts")
> +
> +	ip netns exec $1 ./pm_nl_ctl dump token $tk
> +}
> +
>  check_output()
>  {
>  	local cmd="$1"
> @@ -3465,10 +3477,24 @@ userspace_tests()
>  		chk_mptcp_info subflows 2 subflows 2
>  		chk_subflows_total 3 3
>  		chk_mptcp_info add_addr_signal 2 add_addr_accepted 2
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then

It is important to mark tests that are skipped, otherwise we might miss
an issue with the check skipping the tests.

In mptcp_join.sh, you can see 'mptcp_lib_kallsyms_has' is usually called
from a 'continue_if': it is useful to skip the whole subtest. But here,
we only want to skip one check of a subtest, not the whole subtest. We
should then simply add: 'else print_skip (...)'.

→ see below

> +			check_output "userspace_pm_dump $ns1" \
> +				     $'id 10 flags signal 10.0.2.1\nid 20 flags signal 10.0.3.1' \
> +				     "      dump addrs signal"
> +		fi

Maybe easier to move the kallsyms check + other stuff in a new helper,
not to repeat that 6 times? Here you would have just one line, something
like that:

  userspace_pm_chk_dump_addr "${ns1}" "<expected>" "signal"

And in this helper, you would do something like:

  local ns="${1}"
  local exp="${2}"
  local check="${3}"

  print_check "dump addrs ${check}"

  if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
      check_output "userspace_pm_dump ${ns}" "${exp}"
  else
      print_skip
  fi

WDYT?

Same below.

>  		userspace_pm_rm_addr $ns1 10
>  		userspace_pm_rm_sf $ns1 "::ffff:10.0.2.1" $SUB_ESTABLISHED
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +			check_output "userspace_pm_dump $ns1" \
> +				     "id 20 flags signal 10.0.3.1" \
> +				     "      dump addrs after rm_addr 10"
> +		fi
>  		userspace_pm_rm_addr $ns1 20
>  		userspace_pm_rm_sf $ns1 10.0.3.1 $SUB_ESTABLISHED
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +			check_output "userspace_pm_dump $ns1" \
> +				     "" "      dump addrs after rm_addr 20"
> +		fi
>  		chk_rm_nr 2 2 invert
>  		chk_mptcp_info subflows 0 subflows 0
>  		chk_subflows_total 1 1
> @@ -3489,8 +3515,18 @@ userspace_tests()
>  		chk_join_nr 1 1 1
>  		chk_mptcp_info subflows 1 subflows 1
>  		chk_subflows_total 2 2
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +			check_output "userspace_pm_dump $ns2" \
> +				     "id 20 flags subflow 10.0.3.2" \
> +				     "      dump addrs subflow"
> +		fi
>  		userspace_pm_rm_addr $ns2 20
>  		userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +			check_output "userspace_pm_dump $ns2" \
> +				     "" \
> +				     "      dump addrs after rm_addr 20"
> +		fi
>  		chk_rm_nr 1 1
>  		chk_mptcp_info subflows 0 subflows 0
>  		chk_subflows_total 1 1
> @@ -3510,6 +3546,11 @@ userspace_tests()
>  		chk_mptcp_info subflows 0 subflows 0
>  		chk_subflows_total 1 1
>  		userspace_pm_add_sf $ns2 10.0.3.2 0
> +		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +			check_output "userspace_pm_dump $ns2" \
> +				     "id 0 flags subflow 10.0.3.2" \
> +				     "      dump addrs id 0 subflow"
> +		fi
>  		chk_join_nr 1 1 1
>  		chk_mptcp_info subflows 1 subflows 1
>  		chk_subflows_total 2 2

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