Re: [PATCH mptcp-next v5 09/12] selftests: mptcp: extract mptcp_lib_check_expected

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Extract the main part of check_expected() in userspace_pm.sh to a new
> function mptcp_lib_check_expected() in mptcp_lib.sh. It will be used
> in both mptcp_john.sh and userspace_pm.sh.
> 
> check_expected_one() is moved into mptcp_lib.sh too as a sub function
> of mptcp_lib_check_expected().

Out of curiosity: why a subfunction?

(see below)

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 48 +++++++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 39 ++-------------
>  2 files changed, 52 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index df495658f043..ed0013b47edb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -1,6 +1,9 @@
>  #! /bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  
> +# Some variables are used below but indirectly, see check_expected_one()
> +#shellcheck disable=SC2034

Please move that just above the lines or functions having these
false-positive errors to reduce the scope. Otherwise, this rule will be
applied everywhere, and maybe missing valid issues.

In other words, don't declare it here, but in mptcp_join.sh, where used.
For userspace_pm.sh, you can keep it where it is, I suppose.

> +
>  readonly KSFT_PASS=0
>  readonly KSFT_FAIL=1
>  readonly KSFT_SKIP=4
> @@ -419,3 +422,48 @@ mptcp_lib_print_test_counter() {
>  
>  	printf "%02u ${fmt}" "$((++counter))" "${msg}"
>  }
> +
> +# $@: all var names to check
> +mptcp_lib_check_expected() {
> +	# $1: var name ; $2: prev ret
> +	check_expected_one() {

You still need to prefix it with mptcp_lib_ because you could override
functions with the same name declared elsewhere: when
mptcp_lib_check_expected(), this subfunction will be created globally,
potentially overriding other ones with the same name.

> +		local var="${1}"
> +		local exp="e_${var}"
> +		local prev_ret="${2}"
> +
> +		if [ "${!var}" = "${!exp}" ]

Please keep the same code style as in the rest of this file: "then" and
"do" are on the same line as "if" / "for".

> +		then
> +			return 0
> +		fi
> +
> +		if [ "${prev_ret}" = "0" ]
> +		then
> +			return 2
> +		fi
> +
> +		printf "\tExpected value for '%s': '%s', got '%s'.\n" \
> +			"${var}" "${!exp}" "${!var}"
> +		return 1
> +	}
> +
> +	local rc=0
> +	local var
> +
> +	for var in "${@}"
> +	do
> +		check_expected_one "${var}" "${rc}" || rc="${?}"
> +		if [ "${rc}" -eq 2 ]
> +		then
> +			break
> +		fi
> +	done
> +	unset -f check_expected_one
> +
> +	if [ ${rc} -eq 0 ]
> +	then
> +		mptcp_lib_print_ok "[ OK ]"
> +		return 0
> +	fi
> +
> +	return "${rc}"
> +}
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 473639a8009f..3f475ae44d0b 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -5,7 +5,7 @@
>  # code but we accept it.
>  #shellcheck disable=SC2086
>  
> -# Some variables are used below but indirectly, see check_expected_one()
> +# Some variables are used below but indirectly

Maybe point to check_expected now?

>  #shellcheck disable=SC2034
>  
>  . "$(dirname "${0}")/mptcp_lib.sh"
> @@ -233,46 +233,15 @@ make_connection()
>  	fi
>  }
>  
> -# $1: var name ; $2: prev ret
> -check_expected_one()
> -{
> -	local var="${1}"
> -	local exp="e_${var}"
> -	local prev_ret="${2}"
> -
> -	if [ "${!var}" = "${!exp}" ]
> -	then
> -		return 0
> -	fi
> -
> -	if [ "${prev_ret}" = "0" ]
> -	then
> -		return 2
> -	fi
> -
> -	_printf "\tExpected value for '%s': '%s', got '%s'.\n" \
> -		"${var}" "${!exp}" "${!var}"
> -	return 1
> -}
> -
>  # $@: all var names to check
>  check_expected()
>  {
>  	local rc=0
> -	local var
>  
> -	for var in "${@}"
> -	do
> -		check_expected_one "${var}" "${rc}" || rc="${?}"
> -		if [ "${rc}" -eq 2 ]
> -		then
> -			break
> -		fi
> -	done
> -
> -	if [ ${rc} -eq 0 ]
> +	mptcp_lib_check_expected "${@}" || rc="${?}"
> +	if [ "${rc}" -eq 0 ]
>  	then
> -		test_pass
> +		mptcp_lib_result_pass "${test_name}"
>  		return 0
>  	elif [ "${rc}" -eq 2 ]
>  	then

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