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

[Geliang Tang <geliang@kernel.org>]

On Fri, 2024-08-09 at 13:32 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/08/2024 04:30, Geliang Tang wrote:
> > 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.
> 
> I don't think there is anything to fix there. We need these two
> checks:
> one for our CI to detect missing features, and the other one to skip
> the
> test if the (old) kernel (ran by another CI) doesn't support the
> counter.

Yes, indeed, you are right.

> 
> Cheers,
> Matt