Re: [PATCH mptcp-next 4/5] selftests: mptcp: add mptcp_lib_check_output helper

[Matthieu Baerts <matttbe@kernel.org>]

On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Unify check_output() in mptcp_join.sh and check() in pm_netlink.sh into
> a new public function mptcp_lib_check_output() in mptcp_lib.sh. And use
> mptcp_lib_print_ok() and _err() in it to print test results with colors.
> 
> Use this new helper instead of check_output() and check().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 21 +-----------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 32 +++++++++++++++++++
>  .../testing/selftests/net/mptcp/pm_netlink.sh | 24 ++++----------
>  3 files changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ceaaf155cc32..f75d22dbb28d 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -21,7 +21,6 @@ cinfail=""
>  cinsent=""
>  tmpfile=""
>  cout=""
> -check_output_err=""

Same as in a previous patch: to avoid some confusions, I think we should
keep global variables declared here and not in mptcp_lib.sh (or they
need to be prefixed).

(same in userspace_pm.sh)

>  capout=""
>  ksft_skip=4
>  iptables="iptables"
> @@ -151,7 +150,6 @@ init() {
>  	cout=$(mktemp)
>  	evts_ns1=$(mktemp)
>  	evts_ns2=$(mktemp)
> -	check_output_err=$(mktemp)
>  
>  	trap cleanup EXIT
>  
> @@ -165,7 +163,6 @@ cleanup()
>  	rm -f "$sin" "$sout" "$cinsent" "$cinfail"
>  	rm -f "$tmpfile"
>  	rm -rf $evts_ns1 $evts_ns2
> -	rm -f $check_output_err
>  	mptcp_lib_cleanup
>  	cleanup_partial
>  }

I think it is clearer to keep the assignment and cleanup here, than
hiding it in mptcp_lib.sh and doing that everywhere, even when not required.

(same in userspace_pm.sh)

> @@ -3345,26 +3342,10 @@ userspace_pm_get_addr()
>  
>  check_output()
>  {
> -	local cmd="$1"
> -	local expected="$2"

Best to keep that, it is useful to act as a doc. Otherwise, we have to
look at mptcp_lib.sh to find out what is $1 and $2.

(same in userspace_pm.sh)

>  	local msg="$3"
> -	local out=`$cmd 2>$check_output_err`
> -	local cmd_ret=$?
>  
>  	printf "%-42s" "$msg"
> -	if [ $cmd_ret -ne 0 ]; then
> -		mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
> -		cat $check_output_err
> -		ret=${KSFT_FAIL}
> -		return $cmd_ret
> -	elif [ "$out" = "$expected" ]; then
> -		mptcp_lib_print_ok "[ OK ]"
> -		return 0
> -	else
> -		mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
> -		ret=${KSFT_FAIL}
> -		return 1
> -	fi
> +	mptcp_lib_check_output "${1}" "${2}"

(see above: best to pass "${check_output_err}" to mptcp_lib_check_output)

(same in userspace_pm.sh)

>  }
>  
>  userspace_tests()
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 01b0e43eaa80..fd2ed3b5137c 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -410,6 +410,38 @@ mptcp_lib_ns_exit() {
>  	done
>  }
>  
> +check_output_err=$(mktemp)
> +
> +mptcp_lib_check_output() {
> +	: "${check_output_err:?}"
> +	: "${ret:?}"
> +
> +	local cmd="$1"

In mptcp_lib.sh, please always use {} as in the rest of the file. (same
below).

> +	local expected="$2"

(see above: "check_output_err" will not be passed as argument)

> +	local out
> +	local cmd_ret
> +
> +	out=$($cmd 2>"$check_output_err")
> +	cmd_ret=$?
> +
> +	if [ $cmd_ret -ne 0 ]; then
> +		mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "
> +		cat "$check_output_err"
> +		ret=${KSFT_FAIL}

Mmh, I don't think it is a good idea to assign a global variable
declared in another file from here.

Can you not return ${ret} instead? So we would do something like:

  mptcp_lib_check_output (...) || ret=${?}

or:

  if mptcp_lib_check_output (...); then
      # OK (...)
  else
      ret=1
      (...)
  fi

(do you need the value of "${cmd_ret}" later? I think just having 0 or
${KSFT_FAIL} is enough, no?)

> +		return $cmd_ret
> +	elif [ "$out" = "$expected" ]; then
> +		mptcp_lib_print_ok "[ OK ]"
> +		return 0
> +	else
> +		mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
> +		ret=${KSFT_FAIL}
> +		return 1
> +	fi
> +}
> +
>  mptcp_lib_cleanup() {
> +	: "${check_output_err:?}"
> +
>  	echo "cleanup"
> +	rm -f "$check_output_err"
>  }

As mentioned above, I don't think we should do that here. Maybe we don't
need patch 3/5 then?

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