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 for Android: free password hash cracker in your pocket

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

Message-ID: <20180316202920.3dc280be@epycfail>
Date:   Fri, 16 Mar 2018 20:29:20 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     David Ahern <dsahern@...il.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 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; }

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? 

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_06

-- 
Stefano