Re: [PATCH mptcp-next v2 7/7] selftests: mptcp: add mptcp_lib_check_tools helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 16/02/2024 10:50, 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
> whether to check ip tool, ss tool and iptables tools.
> 
> This helper can be used in every scripts.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh     | 13 +--------
>  .../selftests/net/mptcp/mptcp_connect.sh      | 14 +--------
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 26 +----------------
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 29 +++++++++++++++++++
>  .../selftests/net/mptcp/mptcp_sockopt.sh      | 22 +-------------
>  .../testing/selftests/net/mptcp/pm_netlink.sh |  8 +----
>  .../selftests/net/mptcp/simult_flows.sh       |  8 +----
>  .../selftests/net/mptcp/userspace_pm.sh       |  8 +----
>  8 files changed, 36 insertions(+), 92 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 8573326d326a..7c68160ad3b9 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -33,18 +33,7 @@ cleanup()
>  	ip netns del $ns
>  }
>  
> -mptcp_lib_check_mptcp

(see below) Here, we don't need to check for mptcp_lib_check_kallsyms.

(...)

> mptcp_lib_check_tools "ip" "ss"

(you can remove the double quotes if you don't use variables)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 108a1e12436c..cdb8948749eb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -319,3 +319,32 @@ mptcp_lib_wait_local_port_listen() {
>  		sleep 0.1
>  	done
>  }
> +
> +mptcp_lib_check_tools() {
> +	mptcp_lib_check_mptcp
> +	mptcp_lib_check_kallsyms

I would not move that here, better to have an explicit check in each
selftest I think. Also, see above/below: some selftests don't need kallsyms.

> +	if [ "${1:-""}" == "ip" ]; then

Probably better to use a switch/case, so the order is not important, no?


  local tool

  for tool in "${@}"; do
      case "${tool}" in
      "ip")
          if ! ip -Version &> /dev/null; then
              (...)
          fi
          ;;
      "ss")
          (...)
          ;;
      "iptables"* | "ip6tables"*)  ## including extensions
          if ! "${tool}" -V &> /dev/null; then
          (...)
      *)
          mptcp_lib_print_warn "Unsupported tool: ${tool}"
          exit ${KSFT_FAIL}
          ;;
      esac
  done


So we can use this helper with parameters in any order, e.g.

  mptcp_lib_check_tools ss iptables-nft


> +		if ! ip -Version &> /dev/null; then
> +			mptcp_lib_print_warn "SKIP: Could not run test without ip tool"
> +			exit ${KSFT_SKIP}
> +		fi
> +	fi
> +
> +	if [ "${2:-""}" == "ss" ]; then
> +		if ! ss -h | grep -q MPTCP; then
> +			mptcp_lib_print_warn "SKIP: ss tool does not support MPTCP"
> +			exit ${KSFT_SKIP}
> +		fi
> +	fi
> +
> +	if [ "${3:-""}" == "iptables" ]; then
> +		if ! iptables -V &> /dev/null; then
> +			mptcp_lib_print_warn "SKIP: Could not run all tests without iptables tool"
> +			exit ${KSFT_SKIP}
> +		elif ! ip6tables -V &> /dev/null; then
> +			mptcp_lib_print_warn "SKIP: Could not run all tests without ip6tables tool"
> +			exit ${KSFT_SKIP}
> +		fi

I would split "iptables" and "ip6tables" cases, please see above: just
not to repeat the same err message, and maybe a selftest will only need one.

> +	fi
> +}

(...)

> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index ebfefae71e13..e7ab5eca3e75 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -35,13 +35,7 @@ cleanup()
>  	ip netns del $ns1
>  }
>  
> -mptcp_lib_check_mptcp

(see above) Here, we don't need to check for 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
> +mptcp_lib_check_tools "ip"
>  
>  trap cleanup EXIT
>  
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index e6e5b933a1b9..053f40bf25c1 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -42,13 +42,7 @@ cleanup()
>  	done
>  }
>  
> -mptcp_lib_check_mptcp

(see above) Here, we don't need to check for mptcp_lib_check_kallsyms.

(...)

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