Re: [PATCH mptcp-next v5 10/12] selftests: mptcp: export event macros in mptcp_lib

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 10:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> SUB_ESTABLISHED, LISTENER_CREATED, LISTENER_CLOSED, AF_INET and AF_INET6
> are defined in both mptcp_join.sh and userspace_pm.sh, export all event
> macros into mptcp_lib.sh. Add MPTCP_LIB_ prefix for the first three and
> add readonly for the last two.

Why not doing that (prefix + readonly) for both?

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index ed0013b47edb..5feb11801469 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -2,6 +2,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  # Some variables are used below but indirectly, see check_expected_one()
> +# MPTCP_LIB_ANNOUNCED and AF_INET

Better to "export" them then instead

>  #shellcheck disable=SC2034
>  
>  readonly KSFT_PASS=0
> @@ -11,6 +12,16 @@ readonly KSFT_SKIP=4
>  # shellcheck disable=SC2155 # declare and assign separately
>  readonly KSFT_TEST="${MPTCP_LIB_KSFT_TEST:-$(basename "${0}" .sh)}"
>  
> +MPTCP_LIB_ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
> +MPTCP_LIB_REMOVED=7          # MPTCP_EVENT_REMOVED
> +MPTCP_LIB_SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> +MPTCP_LIB_SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
> +MPTCP_LIB_LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> +MPTCP_LIB_LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED

Please use the "readonly" attribute, and align the "#".

Also, probably clearer to add '_EVENT', e.g.

  export MPTCP_LIB_EVENT_ANNOUNCED=6

> +
> +readonly AF_INET=2
> +readonly AF_INET6=10

Prefix with 'MPTCP_LIB_' + export?

> +
>  MPTCP_LIB_SUBTESTS=()
>  MPTCP_LIB_SUBTESTS_DUPLICATED=0
>  
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 3f475ae44d0b..6d358b7e47e9 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -19,16 +19,6 @@ if ! mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
>  fi
>  mptcp_lib_check_tools ip
>  
> -ANNOUNCED=6        # MPTCP_EVENT_ANNOUNCED
> -REMOVED=7          # MPTCP_EVENT_REMOVED
> -SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> -SUB_CLOSED=11      # MPTCP_EVENT_SUB_CLOSED
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10

Instead of modifying hundreds of lines, probably best to do:

  ANNOUNCED=${MPTCP_LIB_EVENT_ANNOUNCED}
  (...)

Same in mptcp_join.sh? (Maybe that's fine, there are just a few there)

(...)

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