Re: [PATCH mptcp-next v20 03/10] selftests: mptcp: check subflow and addr infos

[Geliang Tang <geliang.tang@suse.com>]

On Mon, Jun 19, 2023 at 05:01:46PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 16/06/2023 09:30, Geliang Tang wrote:
> > This patch adds more arguments for chk_mptcp_info() to check subflows,
> > add_addr_signal and add_addr_accepted infos of different namespaces. And
> > invokes chk_mptcp_info() to check subflows infos of userspace PM tests
> > and endpoint tests.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 48 +++++++++++--------
> >  1 file changed, 28 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 3baa6ac3b03e..3811a6c3dc5d 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -1832,31 +1832,32 @@ chk_subflow_nr()
> >  
> >  chk_mptcp_info()
> >  {
> > -	local nr_info=$1
> > -	local info
> > +	local info1=$1
> > +	local nr1=$2
> > +	local info2=$3
> > +	local nr2=$4
> >  	local cnt1
> >  	local cnt2
> >  	local dump_stats
> >  
> > -	if [[ $nr_info = "subflows_"* ]]; then
> > -		info="subflows"
> > -		nr_info=${nr_info:9}
> > -	else
> > -		echo "[fail] unsupported argument: $nr_info"
> > -		fail_test
> > -		return 1
> > -	fi
> > +	printf "%-${nr_blank}s %-30s" " " "mptcp_info $info1:$info2=$nr1:$nr2"
> >  
> > -	printf "%-${nr_blank}s %-30s" " " "mptcp_info $info=$nr_info"
> > +	cnt1="$(ss -N $ns1 -inmHM)"
> > +	cnt2="$(ss -N $ns2 -inmHM)"
> 
> (I don't think we need "-m" but it was already there before)
> 
> > +	if [ -z "${cnt1}" ] || [ -z "${cnt2}" ]; then
> > +		mptcp_lib_fail_if_expected_feature "$info1:$info2 SS counter"
> > +		echo "[skip]"
> > +		return
> 
> If I'm not mistaken, here, we are going to skip the check if:
> - "ss -M" is not supported
> - there is no (more) active MPTCP connection
> 
> I think that's not correct to skip the checks here because some features
> are missing for these reasons:
> 
> - For "ss -M" support, it is probably best to do that in check_tools()
> function, similar to what is done in diag.sh:
> 
>   if ! ss -h | grep -q MPTCP; then (...)
> 
> - if there is no more active MPTCP connection, we should then assume the
> counter is 0 and not skip the check because maybe we have an issue if we
> expect to have active connections.
> 
> In both cases, it is not linked to the info we are trying to grab and
> the printed message that expected features are missing is confusing (and
> wrong).
> 
> => So I think it would be better to:
>   - add a new patch checking if 'ss' supports MPTCP (can be done in
> another series)
>   - here: just assume the counter is 0 if 'ss' output is empty
> 
> Also, I just remembered 'ss' will only display the counters that are
> different from 0... That's a nice thing to do to reduce the number of
> info displayed by ss but it means we cannot use 'ss' to know if the
> counter is supported by old kernels... On the other hand, all the
> counters you are getting for the moment (subflow, add_addr_signal +
> accepted) are present since kernel v5.9 so before the first LTS version
> with MPTCP support so we are good. If one day we want to get new
> counters, we will first need to check if the running kernel supports
> such counters. We can do that later. So no need to skip the tests here
> if it looks like features are missing.
> 
> => In other words, sorry for the confusion but I think it would be
> better to go back to what you were doing before: without the 'if' and by
> retrieving info in one step:
> 
>   cnt1=$(ss -N $ns1 -inmHM | grep "$info1:" |
>          sed -n 's/.*\('"$info1"':\)\([[:digit:]]*\).*$/\2/p;q')
>   # 'ss' only display active connections and counters that are not 0.
>   [ -z "$cnt1" ] && cnt1=0
> 
> If there is no other modifications in this v20, I can do the
> modification myself if that's OK for you. Or I can apply a Squash-to
> patch if you prefer.

Thanks Matt, please update this for me when merging it.

-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net