Re: [PATCH mptcp-next v10 12/12] selftests: mptcp: add path_manager sysctl tests

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/03/2025 12:01, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch checks if the newly added net.mptcp.path_manager is mapped
> successfully from or to the old net.mptcp.pm_type. And add a new helper
> set_path_manager() to set the newly added net.mptcp.path_manager.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> 
> fix

(typo)

> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/userspace_pm.sh       | 58 ++++++++++++++++++-
>  1 file changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 3651f73451cf..35ba4edc0fa2 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -118,9 +118,61 @@ trap cleanup EXIT
>  
>  # Create and configure network namespaces for testing

I see that you didn't move the "print_title" and "print_test" here. I
think you should do that to be able to print test_fail properly, no?

>  mptcp_lib_ns_init ns1 ns2
> -for i in "$ns1" "$ns2" ;do
> -	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> -done
> +
> +set_path_manager()
> +{
> +	local ns=$1
> +	local pm=$2
> +
> +	if ! ip netns exec ${ns} sysctl net.mptcp.available_path_managers |
> +	     grep -wq "${pm}"; then
> +		test_fail "path manager ${pm} not found"
> +		return 1
> +	fi
> +	ip netns exec ${ns} sysctl -q net.mptcp.path_manager="${pm}"
> +}
> +

Maybe add a comment here to explain what's the goal, e.g.

  # check path_manager and pm_type sysctl mapping

> +if [ -f /proc/sys/net/mptcp/path_manager ]; then
> +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
> +	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
> +	if [ "${pm_name}" != "kernel" ]; then
> +		test_fail "unexpected pm_name: ${pm_name}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
I don't think this step should be moved below, because here we don't
change values: pm_type=0 is the default value.

> +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=1
> +	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
> +	if [ "${pm_name}" != "userspace" ]; then
> +		test_fail "unexpected pm_name: ${pm_name}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi

I don't think you need this step if you keep the for-loop out of the
'else', like before, see below. It implicitly check that setting pm_type
to 1 also changes "path_manager", otherwise the rest of the tests will fail.

> +
> +	set_path_manager "$ns1" "kernel"
> +	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
> +	if [ "${pm_type}" != "0" ]; then
> +		test_fail "unexpected pm_type: ${pm_type}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
> +
> +	set_path_manager "$ns1" "userspace"
> +	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
> +	if [ "${pm_type}" != "1" ]; then
> +		test_fail "unexpected pm_type: ${pm_type}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
> +
> +	set_path_manager "$ns2" "userspace"
> +	print_test "check path_manager and pm_type sysctl mapping"
> +	test_pass
> +else
> +	for i in "$ns1" "$ns2"; do
> +		ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> +	done
> +fi

I do think there is no need to do repeating tests here, Just having
these 3 steps are enough to cover all cases directly and indirectly,
easier to maintain without the repeat, and only focus on the mapping
aspect, not on the final values we set (so keeping the 'pm_type=1' at
the end for all kernel versions is easier).

In other words:

  print_title "Init"
  print_test "Created network namespaces ns1, ns2"
  mptcp_lib_ns_init ns1 ns2

  # check path_manager and pm_type sysctl mapping
  if [ -f /proc/sys/net/mptcp/path_manager ]; then
      ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=userspace
      pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
      if [ "${pm_type}" != "1" ]; then
          test_fail "unexpected pm_type: ${pm_type}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi

      ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
      pm="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
      if [ "${pm}" != "kernel" ]; then
          test_fail "unexpected path-manager: ${pm}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi
  fi

  for i in "$ns1" "$ns2"; do
      ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
  done

  (...)

Small and enough, no?

(If you want, you can also add an extra test setting "path_manager" to
"error", and checking that "pm_type" is unchanged, e.g. place this in
the middle of the two.)

      ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=error
2>/dev/null
      pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
      if [ "${pm_type}" != "1" ]; then
          test_fail "unexpected pm_type after error: ${pm_type}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi

And no need to have the same tests on ns2. Easier to keep the for-loop
setting pm_type=1 like before.

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