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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Wed, Feb 14, 2024 at 11:18:04AM +0100, Matthieu Baerts wrote:
> 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.

Sure, I'll use shellcheck to check my script modifications next time.

> 
> > $ 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:

Thanks a lot.

-Geliang

> 
> > 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.