Re: [PATCH mptcp-next v5 02/12] selftests: mptcp: sockopt: print every test result

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 28/02/2024 08:57, Geliang Tang wrote:
> On Mon, 2024-02-26 at 13:40 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 26/02/2024 10:43, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> Only total test results are printed out in mptcp_sockopt.sh:
>>>
>>>  PASS: all packets had packet mark set
>>>  PASS: SOL_MPTCP getsockopt has expected information
>>>  PASS: TCP_INQ cmsg/ioctl -t tcp
>>>  PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>>>  PASS: TCP_INQ cmsg/ioctl -r tcp
>>>  PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>>>
>>> This patch prints more info for every test result in each test
>>> group:
>>>
>>> transfer ipv4                                                [ OK ]
>>> mark ipv4                                                    [ OK ]
>>> transfer ipv6                                                [ OK ]
>>> mark ipv6                                                    [ OK ]
>>> PASS: all packets had packet mark set
>>> sockopt v4                                                   [ OK ]
>>> sockopt v6                                                   [ OK ]
>>> PASS: SOL_MPTCP getsockopt has expected information
>>> TCP_INQ: -t tcp                                              [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -t tcp
>>> TCP_INQ: -6 -t tcp                                           [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -6 -t tcp
>>> TCP_INQ: -r tcp                                              [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -r tcp
>>> TCP_INQ: -6 -r tcp                                           [ OK ]
>>> PASS: TCP_INQ cmsg/ioctl -6 -r tcp
>>> TCP_INQ: -r tcp -t tcp                                       [ OK ]
>>
>> Please clearly explain why this is interesting, even if it looks
>> obvious.
>>
>> Here, I don't know if this patch makes sense or not: to me, the
>> output
>> is now confusing because there is a mix of '[ OK ]' and 'PASS'.
>> Maybe:
>>
>> - remove all the "PASS: xxx"? → I don't see what it brings more
>> - (or convert existing "PASS: xxx" and "FAIL: xxx" to use "[ OK ]"?
>> But
>> there are fewer details)
> 
> This patch uses to match the output of this script:
> 
> INFO: PASS: all packets had packet mark set
> INFO: PASS: SOL_MPTCP getsockopt has expected information
> INFO: PASS: TCP_INQ cmsg/ioctl -t tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -6 -t tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -r tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -6 -r tcp
> INFO: PASS: TCP_INQ cmsg/ioctl -r tcp -t tcp
> 
> with the test results of it:
> 
> ok 1 - mptcp_sockopt: mark ipv4
> ok 2 - mptcp_sockopt: transfer ipv4
> ok 3 - mptcp_sockopt: mark ipv6
> ok 4 - mptcp_sockopt: transfer ipv6
> ok 5 - mptcp_sockopt: sockopt v4
> ok 6 - mptcp_sockopt: sockopt v6
> ok 7 - mptcp_sockopt: TCP_INQ: -t tcp
> ok 8 - mptcp_sockopt: TCP_INQ: -6 -t tcp
> ok 9 - mptcp_sockopt: TCP_INQ: -r tcp
> ok 10 - mptcp_sockopt: TCP_INQ: -6 -r tcp
> ok 11 - mptcp_sockopt: TCP_INQ: -r tcp -t tcp
> 
> This patch is prepared for the coming printing counter patch, otherwise
> the two output results may have different serial numbers, which can be
> confusing.

Yes, I agree that it makes sense to print the individual results. But
what I wanted to say is that maybe we no longer need these "INFO: PASS:"
lines in the output?

e.g. only printing this:

# 01 Transfer ipv4                                     [ OK ]
# 02 Mark ipv4                                         [ OK ]
# 03 Transfer ipv6                                     [ OK ]
# 04 Mark ipv6                                         [ OK ]
# 05 SOL_MPTCP sockopt v4                              [ OK ]
# 06 SOL_MPTCP sockopt v6                              [ OK ]
# 07 TCP_INQ: -t tcp                                   [ OK ]
# 08 TCP_INQ: -6 -t tcp                                [ OK ]
# 09 TCP_INQ: -r tcp                                   [ OK ]
# 10 TCP_INQ: -6 -r tcp                                [ OK ]
# 11 TCP_INQ: -r tcp -t tcp                            [ OK ]

I think these 'INFO: PASS:' lines don't bring more info, no? If they do,
we can always modify what is printed above.

(While at it, it might look nicer to do: s/ipv4/IPv4/ (or s/ipv4/v4) if
we easily control that, I didn't check)


BTW, thank you for the replies on the different patches, it helps here
because there were a lot of comments on this v5.

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