Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Adam Sindelar <adam@wowsignal.io>]

On Sat, Jul 09, 2022 at 10:14:27AM +0200, Adam Sindelar wrote:
> On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> > On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> > 
> > > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > > previous patch, somehow.
> > > 
> > > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > > paging")
> > > 
> > > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > > ---
> > >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> > >  1 file changed, 0 insertions(+), 0 deletions(-)
> > >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > > 
> > > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > > old mode 100644
> > > new mode 100755
> > 
> > Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> > They're invoked via `/bin/sh foo.sh', which is more robust.
> > 
> > Can we hunt down and fix the invoking code?  Might be as simple as
> > 
> > --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> > +++ a/tools/testing/selftests/vm/run_vmtests.sh
> > @@ -144,7 +144,7 @@ run_test() {
> >  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
> >  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
> >  
> > -		"$@"
> > +		/bin/sh "$@"
> >  		local ret=$?
> >  		if [ $ret -eq 0 ]; then
> >  			echo "[PASS]"
> > _
> > 
> 
> I think that would impose the choice of shell on the test scripts. About
> half of them start with '#!/bin/sh' and the other half with
> '#!/bin/bash'.
> 
> Maybe that's something we'd want to do anyway, but it seems like it
> could have subtle and unintended side effects if the goal is to fix a
> failing test.
> 
> (It would also invoke the ELF binaries through /bin/sh, but that
> probably doesn't matter, since sh will I think exec right away.)
> 

Having thought about it: invoking the tests using your draft fails to
run the tests that are ELF binaries. `/bin/sh -c "@"` does run everything
but doesn't pass arguments properly. (On my system imposing /bin/sh over
/bin/bash doesn't matter, but I think it's possible for /bin/sh and
whatever shell a script has in its shebang to be incompatible, e.g. with
zsh.)

This might be my own ignorance, but I don't see an obvious way to make
the invoking code correctly handle all tests without branching based on
the contents of the file. We could look inside and act on the shebang,
but then we're reimplementing execve.

To advance a counterargument: as you say, not all *.sh files in
tools/testing/selftests/vm are executable, but all test programs
(arguments to `run_test`) are executable. Currently the test programs
are treated the same whether they're ELF binaries of shell scripts.
Having some test programs not be executable would require treating some
test programs differently from others for the first time.