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

[Matthieu Baerts <matttbe@kernel.org>]

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.

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