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

[Geliang Tang <geliang@kernel.org>]

On Wed, 2024-02-28 at 10:13 +0100, Matthieu Baerts wrote:
> 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 ]

Yes, this is much better. Will update it in next version.

> 
> 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)

's/ipv4/v4' is better, will update them.

Thanks,
-Geliang

> 
> 
> 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