Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Geliang Tang <geliang@kernel.org>]

On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 08/08/2024 04:38, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for these patches.
> > 
> > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > The checksum and fail counters might not be available. Then no
> > > need
> > > to
> > > display an extra message with missing info.
> > > 
> > > While at it, fix the indentation around, which is wrong since the
> > > same
> > > commit.
> > > 
> > > Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
> > > counter not supported")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-----
> > > -
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index 01c1e0871aca..a1f80dac59a7 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1112,7 +1112,7 @@ chk_csum_nr()
> > >  
> > >  	print_check "sum"
> > >  	count=$(mptcp_lib_get_counter ${ns1}
> > > "MPTcpExtDataCsumErr")
> > > -	if [ "$count" != "$csum_ns1" ]; then
> > > +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
> > 
> > We have checked [ -z "$count" ] in mptcp_lib_get_counter() already,
> > so
> > I think no need to double check it here. Can we just let
> > mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case,
> > then
> > we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
> > chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
> 
> I don't think we can do that: mptcp_lib_get_counter() will return
> nothing if the counter doesn't exist: if the kernel being used
> doesn't
> support it. If it is our CI running the tests, there will be a
> failure
> thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
> CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.
> 
> So here, we need to check if '$count' is not empty, before comparing
> it
> with '$csum_ns1'. If it is empty, the check will be skipped (see 4
> lines
> below), but we don't want to print a useless "extra message" in this
> case, with just "ns1=".

If so, let's keep this patch as is. I'll try to fix it later.

> 
> Cheers,
> Matt