Re: [PATCH mptcp-next v11 4/8] selftests: mptcp: sockopt: print every test result

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 03/03/2024 12:52, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Only total test results are printed out in mptcp_sockopt.sh:
> 
> PASS: all packets had packet mark set
> PASS: SOL_MPTCP getsockopt has expected information
> PASS: TCP_INQ cmsg/ioctl -t tcp
> PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> PASS: TCP_INQ cmsg/ioctl -r tcp
> PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp
> 
> They mismatch with the test results:
> 
> ok 1 - mptcp_sockopt: mark ipv4
> ok 2 - mptcp_sockopt: transfer ipv4
> ok 3 - mptcp_sockopt: mark ipv6
> ok 4 - mptcp_sockopt: transfer ipv6
> ok 5 - mptcp_sockopt: sockopt v4
> ok 6 - mptcp_sockopt: sockopt v6
> ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
> ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
> ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
> ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
> ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp
> 
> 'mptcp_sockopt.sh' now display more detailed results + why (what you had
> in a former patch from v6, merged here). It no longer displays 'PASS:',
> because it is duplicated info now that the detailed are displayed:
> 
> Transfer v4                                       [ OK ]
> Mark v4                                           [ OK ]
> Transfer v6                                       [ OK ]
> Mark v6                                           [ OK ]

Small detail: I see above, in the TAP output, that we first have 'mark',
then 'transfer', while here, it is the opposite: 'Transfer', then 'Mark'.

While at it, can you invert the two please to keep the same order for
both please? Especially when later we add the test number.

I guess the TAP output is wrong (not logical) → so we should move:

  mptcp_lib_result_code "${rets}" "transfer ${ip}"

before checking the mark.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index bf22174c022c..f7f643779992 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -113,6 +113,11 @@ check_mark()
>  	return 0
>  }
>  
> +print_title()
> +{
> +	printf "%-50s" "${@}"
> +}
> +
>  do_transfer()
>  {
>  	local listener_ns="$1"
> @@ -162,6 +167,7 @@ do_transfer()
>  	wait $spid
>  	local rets=$?
>  
> +	print_title "Transfer ${ip:2}"
>  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
>  		echo " client exit code $retc, server $rets" 1>&2
>  		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
> @@ -175,7 +181,9 @@ do_transfer()
>  		ret=1
>  		return 1

Here, in case of error, you will not print '[FAIL]', right?

>  	fi
> +	echo "[ OK ]"

Mmh, be careful that the transfer can be marked as failed later, please
check where we call "mptcp_lib_check_transfer".

In fact, I think we should move ...

  mptcp_lib_check_transfer $cin $sout "file received by server"
  rets=$?
  mptcp_lib_result_code "${rets}" "transfer ${ip}"

... before this "echo "[ OK ]", no?


So having something like:

  print_title "Transfer ${ip:2}"
  if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
      echo "[FAIL]"
      (...)
      return 1
  elif ! mptcp_lib_check_transfer $cin $sout "(...)"; then
      rets=1
      mptcp_lib_result_fail "transfer ${ip}"
  else
      echo "[ OK ]"
      mptcp_lib_result_pass "transfer ${ip}"
  fi


By doing that, we fix the TAP order, and we correctly print "OK" even if
there was an issue during the transfer

>  
> +	print_title "Mark ${ip:2}"
>  	if [ $local_addr = "::" ];then
>  		check_mark $listener_ns 6 || retc=1
>  		check_mark $connector_ns 6 || retc=1
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.