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
+			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]}
+			userspace_pm_rm_addr $ns $id
+			userspace_pm_rm_sf $ns "$addr" $SUB_ESTABLISHED
+		done
+	else
+		print_skip
+	fi
+}
+
 userspace_pm_chk_dump_addr()
 {
 	local ns="${1}"
@@ -3518,25 +3546,29 @@ userspace_tests()
 	if reset_with_events "userspace pm create destroy subflow" &&
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns2
-		pm_nl_set_limits $ns1 0 1
+		pm_nl_set_limits $ns1 0 2
 		speed=5 \
 			run_tests $ns1 $ns2 10.0.1.1 &
 		local tests_pid=$!
 		wait_mpj $ns2
+		userspace_pm_add_sf $ns2 10.0.2.2 10
 		userspace_pm_add_sf $ns2 10.0.3.2 20
-		chk_join_nr 1 1 1
-		chk_mptcp_info subflows 1 subflows 1
-		chk_subflows_total 2 2
+		chk_join_nr 2 2 2
+		chk_mptcp_info subflows 2 subflows 2
+		chk_subflows_total 3 3
 		userspace_pm_chk_dump_addr "${ns2}" \
-			"id 20 flags subflow 10.0.3.2" \
+			$'id 10 flags subflow 10.0.2.2\nid 20 flags subflow 10.0.3.2' \
 			"subflow"
+		userspace_pm_chk_get_addr "${ns2}" "10" "id 10 flags subflow 10.0.2.2"
 		userspace_pm_chk_get_addr "${ns2}" "20" "id 20 flags subflow 10.0.3.2"
 		userspace_pm_rm_addr $ns2 20
 		userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns2}" \
-			"" \
+			"id 10 flags subflow 10.0.2.2" \
 			"after rm_addr 20"
-		chk_rm_nr 1 1
+		userspace_pm_flush $ns2
+		userspace_pm_chk_dump_addr "${ns2}" "" "after flush"
+		chk_rm_nr 2 2
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
 		kill_events_pids
-- 
2.40.1

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.