netdev - Re: [PATCH net-next 3/9] selftests: pmtu: Introduce support for multiple tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <626b0eb0-846a-b00e-58a2-d5c2e3b24a95@gmail.com>
Date:   Fri, 16 Mar 2018 12:53:23 -0700
From:   David Ahern <dsahern@...il.com>
To:     Stefano Brivio <sbrivio@...hat.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 3/9] selftests: pmtu: Introduce support for
 multiple tests

On 3/16/18 12:29 PM, Stefano Brivio wrote:
> On Fri, 16 Mar 2018 11:06:07 -0700
> David Ahern <dsahern@...il.com> wrote:
> 
>> On 3/15/18 9:18 AM, Stefano Brivio wrote:
>>>  trap cleanup EXIT
>>>  
>>> -test_pmtu_vti6_exception
>>> +exitcode=0
>>> +for name in ${tests}; do
>>> +	echo "${name}: START"
>>> +	eval test_${name}
>>> +	ret=$?
>>> +	cleanup
>>> +
>>> +	if [ $ret -eq 0 ]; then echo "${name}: FAIL"; exitcode=1  
>>
>> ret = 0 == failure is counterintuitive for Linux.
> 
> However, in POSIX shell's AND and OR lists with function calls, a
> function returning zero behaves in a similar fashion to a C function
> evaluating to true, and a function returning non-zero behaves like a C
> function evaluating to false [1]:
> 
> 	a() {
> 		return 0
> 	}
> 
> 	b() {
> 		return 1
> 	}
> 
> 	a && echo this gets printed
> 	b && echo and this does not
> 
> This might be equally counter-intuitive for somebody. If one does a lot
> of explicit error checking with early returns, my return convention is
> also rather practical. E.g. in the setup() function I can do:
> 
> 	eval setup_${arg} && echo "  ${arg} not supported" && return 0
> 
> instead of:
> 
> 	eval setup_${arg} || { echo "  ${arg} not supported" && return 0; }

I think it is weird to have 'a && b' where b is done when a fails as
opposed to succeeds hence the comment. I think a common convention
across scripts is important but having the tests is more so. Just a
suggestion.

> 
> Still, I went ahead and reversed return codes in the whole script, and
> it doesn't look *too* bad with the setup() function from patch 3/9. It
> would have been quite ugly earlier.
> 
> So I don't have a strong preference. If you still prefer that I reverse
> my return codes, I will re-spin the series (and probably I'll need a
> first patch that reverses the existing logic, too).
> 
>>> +	elif [ $ret -eq 1 ]; then echo "${name}: PASS"
>>> +	elif [ $ret -eq 2 ]; then echo "${name}: SKIP"  
>>
>> I use printf in other scripts so that the pass/fail verdict lineup. e.g.,
>> printf "        %-60s  [PASS]\n" "${name}"
> 
> I avoided 'printf' so far because it's not a built-in utility on some
> shells (e.g. csh), being a "recent" addition to the POSIX Base
> Specifications (issue 4, while 'echo' is from issue 2), and it might be
> unavailable on some (embedded?) systems.
> 
> I don't have a strong preference about this either. It's a very minor
> portability concern vs. a very minor readability improvement, what do
> you think? 

Look at fib_tests and fib-onlink-tests. As the number of tests grows,
output consistency makes your life easier. With printf:

...
TEST: IPv4 linkdown flag set                                  [ OK ]
TEST: IPv6 linkdown flag set                                  [ OK ]
TEST: Directly connected nexthop, unicast address             [ OK ]
TEST: Directly connected nexthop, unicast address with device [ OK ]
TEST: Gateway is linklocal address                            [ OK ]
TEST: Gateway is linklocal address, no device                 [ OK ]
TEST: Gateway can not be local unicast address                [ OK ]
TEST: Gateway can not be local unicast address, with device   [ OK ]
TEST: Gateway can not be a local linklocal address            [ OK ]
TEST: Gateway can be local address in a VRF                   [FAIL]
TEST: Gateway can be local address in a VRF, with device      [FAIL]
TEST: Gateway can be local linklocal address in a VRF         [ OK ]
TEST: Redirect to VRF lookup                                  [ OK ]
...

the FAIL cases jump out versus echo

...
TEST: IPv4 linkdown flag set [ OK ]
TEST: IPv6 linkdown flag set [ OK ]
TEST: Directly connected nexthop, unicast address [ OK ]
TEST: Directly connected nexthop, unicast address with device [ OK ]
TEST: Gateway is linklocal address [ OK ]
TEST: Gateway is linklocal address, no device [ OK ]
TEST: Gateway can not be local unicast address [ OK ]
TEST: Gateway can not be local unicast address, with device [ OK ]
TEST: Gateway can not be a local linklocal address [ OK ]
TEST: Gateway can be local address in a VRF [FAIL]
TEST: Gateway can be local address in a VRF, with device [FAIL]
TEST: Gateway can be local linklocal address in a VRF [ OK ]
TEST: Redirect to VRF lookup [ OK ]
...

where your mind has a lot more work to do to find the tests broken by a
change.

That is also why I chose OK versus PASS -- ok at 2 letters, fail at 4
the failures really stand out.