Re: [mptcp-next v3 2/2] selftests: mptcp: add a test for mptcp_diag_dump_one

[Matthieu Baerts <matttbe@kernel.org>]

Hi Gang Yan,

On 21/02/2025 06:34, Gang Yan wrote:
> This patch introduces a new 'chk_diag' test in diag.sh. It retrieves
> the token for a specified MPTCP socket (msk) using the 'ss' command and
> then accesses the 'mptcp_diag_dump_one' in kernel via ./mptcp_diag
> to verify if the correct token is returned.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/524

Before closing this ticket, did you check the new code was also covering
the 1st bullet point mentioned there?

 * subflow_get_info_size() from diag.c is not covered

> Signed-off-by: Gang Yan <yangang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh | 27 +++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 2bd0c1eb70c5..5c10fe406d91 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -200,6 +200,32 @@ chk_msk_cestab()
>  		 "${expected}" "${msg}" ""
>  }
>  
> +chk_dump_one()
> +{
> +	local ss_token
> +	local token
> +	local msg
> +
> +	ss_token="$(ss -inmHMN $ns | grep 'token:' |\
> +		    head -n 1 |\
> +		    sed 's/.*token:\([0-9a-f]*\).*/\1/')"
> +
> +	token="$(ip netns exec $ns ./mptcp_diag -t $ss_token |\
> +		 awk -F':[ \t]+' '/^token/ {print $2}')"
> +
> +	msg="....chk dump_one"
> +
> +	mptcp_lib_print_title "$msg"
> +	if [ "$ss_token" != "$token" ]; then

detail: it sounds more logical to check if the result is correct first, no?

  if [ "$ss_token" = "$token" ]; then
      mptcp_lib_pr_ok
      mptcp_lib_result_pass "${msg}"
  else
      (...)
  fi

Also, just to be on the safe side, probably best to check that the
variables are not both empty, e.g. if both ss and mptcp_diag commands
are wrong or have issues.

  if [ -n "$ss_token" ] && [ "$ss_token" = "$token" ]; then
      mptcp_lib_pr_ok
      (...)
  fi

> +		mptcp_lib_pr_fail "expected $ss_token found $token"
> +		mptcp_lib_result_fail "${msg}"
> +		ret=${KSFT_FAIL}
> +	else
> +		mptcp_lib_pr_ok
> +		mptcp_lib_result_pass "${msg}"
> +	fi
> +}
> +
>  msk_info_get_value()
>  {
>  	local port="${1}"
> @@ -290,6 +316,7 @@ chk_msk_remote_key_nr 2 "....chk remote_key"
>  chk_msk_fallback_nr 0 "....chk no fallback"
>  chk_msk_inuse 2
>  chk_msk_cestab 2
> +chk_dump_one
>  flush_pids
>  
>  chk_msk_inuse 0 "2->0"
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.