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

[Matthieu Baerts <matttbe@kernel.org>]

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

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