Re: [PATCH mptcp-next v3 2/2] selftests: mptcp: add mptcp_lib_check_tools helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for the new version.

On 19/02/2024 09:22, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch exports check_tools() helper from mptcp_join.sh into
> mptcp_lib.sh as a public one mptcp_lib_check_tools(). The arguments
> "ip", "ss", and "iptables" are passed into this helper to indicate

'ip6tables' can be passed too since v3.

> whether to check ip tool, ss tool and iptables tools.
> 
> This helper can be used in every scripts.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index d797b27b7626..d9df6eb6a52a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -152,34 +152,12 @@ cleanup_partial()
>  	done
>  }
>  
> -check_tools()
> -{
> -	mptcp_lib_check_mptcp
> -	mptcp_lib_check_kallsyms
> -
> -	if ! ip -Version &> /dev/null; then
> -		echo "SKIP: Could not run test without ip tool"
> -		exit $ksft_skip
> -	fi
> -
> -	if ! ss -h | grep -q MPTCP; then
> -		echo "SKIP: ss tool does not support MPTCP"
> -		exit $ksft_skip
> -	fi
> -
> -	if ! "${iptables}" -V &> /dev/null; then
> -		echo "SKIP: Could not run all tests without ${iptables} tool"
> -		exit $ksft_skip
> -	elif ! "${ip6tables}" -V &> /dev/null; then
> -		echo "SKIP: Could not run all tests without ${ip6tables} tool"
> -		exit $ksft_skip
> -	fi
> -}
> -
>  init() {
>  	init=1
>  
> -	check_tools
> +	mptcp_lib_check_mptcp
> +	mptcp_lib_check_kallsyms
> +	mptcp_lib_check_tools ip ss iptables

You should use "${iptables}" -- just in case we switch to iptables-nft
or something else later -- and add "${ip6tables}" now that the check is
split.

>  	sin=$(mktemp)
>  	sout=$(mktemp)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 108a1e12436c..b92c5278730b 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -319,3 +319,34 @@ mptcp_lib_wait_local_port_listen() {
>  		sleep 0.1
>  	done
>  }
> +
> +mptcp_lib_check_tools() {
> +	local tool
> +
> +	for tool in "${@}"; do
> +		case "${tool}" in
> +		"ip")
> +			if ! ip -Version &> /dev/null; then
> +				mptcp_lib_print_warn "SKIP: Could not run test without ip tool"
> +				exit ${KSFT_SKIP}
> +			fi
> +			;;
> +		"ss")
> +			if ! ss -h | grep -q MPTCP; then
> +				mptcp_lib_print_warn "SKIP: ss tool does not support MPTCP"
> +				exit ${KSFT_SKIP}
> +			fi
> +			;;
> +		"iptables"* | "ip6tables"*)
> +			if ! "${tool}" -V &> /dev/null; then
> +				mptcp_lib_print_warn "SKIP: Could not run all tests without ${tool} tool"
> +				exit ${KSFT_SKIP}
> +			fi
> +			;;
> +		*)
> +			mptcp_lib_print_warn "Unsupported tool: ${tool}"
> +			exit ${KSFT_SKIP}

If we are here, it means we have an internal error: we are checking a
tool we don't support. I think you should then use 'mptcp_lib_print_err'
and exit an error "${KSFT_FAIL}".

If you prefer, you can also print a more explicit message, e.g.

  Internal error: unsupported tool: ${tool}

Up to you, this error should not happen.

> +			;;
> +		esac
> +	done
> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index dac8e1fc7143..edb71f44be45 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -89,20 +89,7 @@ cleanup()
>  
>  mptcp_lib_check_mptcp
>  mptcp_lib_check_kallsyms
> -
> -ip -Version > /dev/null 2>&1
> -if [ $? -ne 0 ];then
> -	echo "SKIP: Could not run test without ip tool"
> -	exit $ksft_skip
> -fi
> -
> -if ! "${iptables}" -V &> /dev/null; then
> -	echo "SKIP: Could not run all tests without ${iptables} tool"
> -	exit $ksft_skip
> -elif ! "${ip6tables}" -V &> /dev/null; then
> -	echo "SKIP: Could not run all tests without ${ip6tables} tool"
> -	exit $ksft_skip
> -fi
> +mptcp_lib_check_tools ip iptables

Same here: use "${iptables}" instead, and add "${ip6tables}".

>  
>  check_mark()
>  {

(...)

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