Re: [PATCH mptcp-next 8/8] selftests: mptcp: flush userspace addrs list

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 21/02/2024 7:31 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper userspace_pm_flush() to flush all addresses
> for the userspace PM. Invoke it in userspace pm dump address and subflow
> tests. And use dump commands to check if the userspace pm local address
> list is empty after addresses flushing.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 46 ++++++++++++++++---
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index aedc5698f26a..9f1476f0e2ae 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3374,6 +3374,34 @@ userspace_pm_get_addr()
>  	ip netns exec $1 ./pm_nl_ctl get $2 token $tk
>  }
>  
> +# $1: ns ; $2: addr
> +userspace_pm_flush()
> +{
> +	if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
> +		local ns=$1
> +		local line
> +
> +		userspace_pm_dump $ns | while read -r line; do

This will create a subshell: it means that variables that are changed
here below will only be valid in this restricted scope. In other words,
if there is a failure below, the 'failure' message will be printed, but
'ret=1' will only be set here in this scope, so the test will not be
marked as failed: the check will be ignored. You cannot do it like that.

What you can do is something like that to avoid a subshell:

  while read -r line; do (...); done <<< $(cmd)

but...

> +			local arr=($line)
> +			local nr=0
> +			local id
> +			local addr
> +			local i
> +			for i in "${arr[@]}"; do
> +				if [ $i = "id" ]; then
> +					id=${arr[$nr+1]}
> +				fi
> +				nr=$((nr + 1))
> +			done
> +			addr=${arr[$nr-1]}

Can you not use read to do the parsing if it is always the same output:

  read -r _ id _ _ addr

> +			userspace_pm_rm_addr $ns $id
> +			userspace_pm_rm_sf $ns "$addr" $SUB_ESTABLISHED
> +		done
> +	else
> +		print_skip

The skip doesn't make sense here: there is no 'check' in progress.

But that's not it, the behaviour is wrong too: if the feature is not
supported, it means that the subflows and addresses will not be removed,
then the rest of the test will be wrong on kernels not supporting the
features because the counters will be different.

Maybe we could do that in a new dedicated test where everything is
skipped if "dump addresses" is not supported. But, does this test
increase the code coverage? I understand that doing the flush is a good
"summary", but it looks like this has already been validated, no? So
maybe easier to drop this?

> +	fi
> +}
> +
>  userspace_pm_chk_dump_addr()
>  {
>  	local ns="${1}"

(...)

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