Re: [PATCH mptcp-next v13 08/14] selftests: mptcp: add check_output helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 16/02/2024 08:42, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Similar to check() in pm_netlink.sh, add a new helper check_output()
> in mptcp_join.sh to check the output of the given commands.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index b807e2a42c61..6f7e83f61707 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -21,6 +21,7 @@ cinfail=""
>  cinsent=""
>  tmpfile=""
>  cout=""
> +check_output_err=""
>  capout=""
>  ns1=""
>  ns2=""
> @@ -182,6 +183,7 @@ init() {
>  	cout=$(mktemp)
>  	evts_ns1=$(mktemp)
>  	evts_ns2=$(mktemp)
> +	check_output_err=$(mktemp)
>  
>  	trap cleanup EXIT
>  
> @@ -195,6 +197,7 @@ cleanup()
>  	rm -f "$sin" "$sout" "$cinsent" "$cinfail"
>  	rm -f "$tmpfile"
>  	rm -rf $evts_ns1 $evts_ns2
> +	rm -f $check_output_err
>  	cleanup_partial
>  }
>  
> @@ -3347,6 +3350,30 @@ userspace_pm_rm_sf()
>  	wait_rm_sf $1 "${cnt}"
>  }
>  
> +check_output()
> +{
> +	local cmd="$1"
> +	local expected="$2"
> +	local msg="$3"
> +	local out=`$cmd 2>$check_output_err`

This causes the following warnings from shellcheck:

> $ shellcheck -x mptcp_join.sh
> 
> In mptcp_join.sh line 3365:
>         local out=`$cmd 2>$check_output_err`
>               ^-^ SC2155 (warning): Declare and assign separately to avoid masking return values.
>                   ^------------------------^ SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
> 
> Did you mean: 
>         local out=$($cmd 2>$check_output_err)

I guess it should be:

  local out cmd_ret=0

  out=$(${cmd} 2>${check_output_err}) || cmd_ret=${?}

Please check you don't introduce new shellcheck warnings. There are
shellcheck plugins for various text editors.

> +	local cmd_ret=$?
> +
> +	printf "%-42s" "$msg"

You should not need this. Can you not use print_check() instead?

And in fact, you should not call 'print_check' from here, but from the
caller, where you would have 'print_skip', see patches 9 and 14/14.

> +	if [ $cmd_ret -ne 0 ]; then
> +		mptcp_lib_print_err "[FAIL] command execution '$cmd' stderr "

Here, you should call fail_test() instead, and not set ret=<FAIL>: there
is a single function setting ret to a different value, best not to
change that.

Also, fail_test() will print more info if needed, and mark the test as
failed.

> +		cat $check_output_err
> +		ret=${KSFT_FAIL}
> +		return $cmd_ret
> +	elif [ "$out" = "$expected" ]; then
> +		mptcp_lib_print_ok "[ OK ]"

And here you should call print_ok().

> +		return 0
> +	else
> +		mptcp_lib_print_err "[FAIL] expected '$expected' got '$out'"
> +		ret=${KSFT_FAIL}
> +		return 1

Same here: fail_test().

> +	fi
> +}
> +
>  userspace_tests()
>  {
>  	# userspace pm type prevents add_addr

(looking at the modifications that are specific to mptcp_join.sh, you
might have to drop "selftests: mptcp: add mptcp_lib_check_output helper"
from "add helpers and vars in mptcp_lib.sh, part 2" series. But of
course, still OK to use mptcp_lib_print_XXX from pm_netlink.sh and other
.sh scripts to add colours!)

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