Re: [PATCH mptcp-next 01/10] selftests: mptcp: export ip_mptcp to mptcp_lib

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 11/03/2024 02:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch exports ip_mptcp into mptcp_lib.sh as a public variable,
> named mptcp_lib_ip_mptcp. Add a helper mptcp_lib_set_ip_mptcp() to set
> it, and a helper mptcp_lib_is_ip_mptcp() to test whether it is set. Use
> these two helpers in mptcp_join.sh.
> 
> This patch is prepared for coming commits.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index d529b4b37af8..d84f2f4986a7 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -23,6 +23,7 @@ MPTCP_LIB_SUBTESTS=()
>  MPTCP_LIB_SUBTESTS_DUPLICATED=0
>  MPTCP_LIB_TEST_COUNTER=0
>  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
> +mptcp_lib_ip_mptcp=0

Please use capital letters for these global variables, like the other
ones here above.

>  # only if supported (or forced) and not disabled, see no-color.org
>  if { [ -t 1 ] || [ "${SELFTESTS_MPTCP_LIB_COLOR_FORCE:-}" = "1" ]; } &&
> @@ -505,3 +506,15 @@ mptcp_lib_verify_listener_events() {
>  	mptcp_lib_check_expected "type" "family" "saddr" "sport" || rc="${?}"
>  	return "${rc}"
>  }
> +
> +mptcp_lib_set_ip_mptcp() {
> +	: "${mptcp_lib_ip_mptcp:?}"

I don't think you need that, the variable is declared in the same file.
(and you are going to modify it anyway, no "read")

> +	mptcp_lib_ip_mptcp=1
> +}
> +
> +mptcp_lib_is_ip_mptcp() {
> +	: "${mptcp_lib_ip_mptcp:?}"

Same here, not needed I think.

> +
> +	[ ${mptcp_lib_ip_mptcp} -eq 1 ]

detail: because global variables could be modified elsewhere, I usually
find it safer to use double quotes, and compare strings:

  [ "${MPTCP_LIB_IP_MPTCP}" = "1" ]

But on the other hand, this global variable is not supposed to be
modified elsewhere... So up to you.

> +}

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