Re: [PATCH mptcp-next v3 2/2] selftests: mptcp: rm subflow with v4/v4mapped addr

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 07/02/2024 02:55, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Now both a v4 address and a v4-mapped address are supported when
> destroying a userspace pm subflow, this patch adds a second subflow
> to "userspace pm add & remove address" test, and two subflows could
> be removed two different ways, one with the v4mapped and one with v4.

Thank you for validating this.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 3a2abae5993e..69001d2a8dab 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -213,9 +213,12 @@ mptcp_lib_get_info_value() {
>  	grep "${2}" | sed -n 's/.*\('"${1}"':\)\([0-9a-f:.]*\).*$/\2/p;q'
>  }
>  
> -# $1: info name ; $2: evts_ns ; $3: event type
> +# $1: info name ; $2: evts_ns ; $3: event type; $4: addr
>  mptcp_lib_evts_get_info() {
> -	mptcp_lib_get_info_value "${1}" "^type:${3:-1}," < "${2}"
> +	local addr=${4:-""}
> +
> +	cat "${2}" | grep "${addr}" |
> +	mptcp_lib_get_info_value "${1}" "^type:${3:-1},"

When modifying shell scripts, please make sure 'shellcheck' is happy
with the modifications you did.

> $ shellcheck -x mptcp_lib.sh
> 
> In mptcp_lib.sh line 220:
>         cat "${2}" | grep "${addr}" |
>             ^----^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
> 
> For more information:
>   https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...

I then did this modification when applying the patch:

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 69001d2a8dab..3777d66fc56d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -213,12 +213,9 @@ mptcp_lib_get_info_value() {
>         grep "${2}" | sed -n 's/.*\('"${1}"':\)\([0-9a-f:.]*\).*$/\2/p;q'
>  }
>  
> -# $1: info name ; $2: evts_ns ; $3: event type; $4: addr
> +# $1: info name ; $2: evts_ns ; [$3: event type; [$4: addr]]
>  mptcp_lib_evts_get_info() {
> -       local addr=${4:-""}
> -
> -       cat "${2}" | grep "${addr}" |
> -       mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
> +       grep "${4:-}" "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
>  }
>  
>  # $1: PID

Note that it is not strictly needed to add ':-' in '${4:-}' (we don't
use 'set -u'), but it clearly shows the argument is optional.

I also edited the comment above the declaration of the function to
reflect that.

About 'shellcheck', please note that for the moment, not all scripts are
shellcheck compliant, only 'mptcp_join.sh', 'mptcp_lib.sh' and
'userspace_pm.sh'. For the others, it would be great if at the least the
new code is shellcheck compliant. I don't think it is needed to modify
the existing ones, they are not often modified and they work. (Except
maybe if there are some other big modifications needed.)

Also, some checks can be ignored with '# shellcheck disable=XXXX'
directive if needed:

  https://www.shellcheck.net/wiki/Ignore

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