| Message ID | 20220708090646.34927-1-adam@wowsignal.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/vm: fix va_128TBswitch.sh permissions | expand |
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]"
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.)
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.
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=$? So, I think tools/testing/selftests/vm/ is a bit interesting in that run_vmtests.sh kind of circumvents the "standard" model for running kselftests. In the "standard" model (which I'm putting in quotes because there's no formal, prescriptive way to use kselftests), testcases are specified as Makefile targets that are put into one of several possible Make variables to inform how they should be built and run. This is described in more details in [0]. [0]: https://docs.kernel.org/dev-tools/kselftest.html?highlight=kselftest#contributing-new-tests-details As it relates to vmtests, we have the following relevant variables: TEST_GEN_FILES -------------- Contains targets that generate executable ELF binaries that are _not_ automatically run by the kselftests runner, but which are built. These are the ELF executables that we run in run_vmtests.sh, sometimes with multiple invocations using different parameters (for e.g. ksm_tests). TEST_GEN_PROGS -------------- Contains targets that generate executable ELF binaries that _are_ automatically run by the kselftest runner. Note that some of the executable targets in the vm suite are not included in run_vmtests.sh, e.g. soft-dirty, and split_huge_page_test. madv_populate actually is, which looks like a bug to me (which was introduced in 749d999c06c2 ("selftests: vm: bring common functions to a new file") and which I'll send out a fix for). TEST_PROGS ---------- Contains test shell scripts. These are automatically invoked by th kselftest runner as well, and (as far as I understand) from the perspective of kselftest, are the exact same as TEST_GEN_PROGS with the exception that of course they're not compiled. In the kselftest runner, if the executable bit is not set, the runner will echo out a warning and then try to parse the interpreter from the first line of the file to figure out how to invoke it. My two cents are that the correct way to structure vm selftests is something like the following: 1. For any testcase that's compiled directly into an ELF binary, and which doesn't need to be invoked with any arguments, convert it to a TEST_GEN_PROGS target in tools/testing/selftests/vm/Makefile, and remove it from being invoked from run_vmtests.sh. 2. For any testcase that's compiled directly into an ELF binary, but which _does_ need to be invoked with multiple arguments and/or needs to be invoked in some other special way such as va_128TBswitch, leave it in TEST_GEN_FILES, and instead add a separate executable shell script wraps it, and which lives in TEST_PROGS. This would be a nice cleanup, because it would let us remove some of the custom logic at the top of run_vmtests.sh that only applies to some of the testcases. 3. Once (1) and (2) are done for all relevant targets, remove run_vmtests.sh and instead rely on the kselftest runner. This would allow us to remove all custom runner logic from tools/testing/selftests/vm, and to instead just rely on the kselftest runner to do the heavy lifting for us. As it relates specifically to Adam's patch, IMO setting that script include the executable bit does make sense, as it's in-line with the guidance for kselftests in general, and I don't think we can assume that all of the shell scripts will be bin/sh. FWIW, I'm happy to do the above cleanup if nobody objects (after Adam's patch lands). Thanks, David
On Tue, Jul 12, 2022 at 09:17:45AM -0700, David Vernet 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=$? > > So, I think tools/testing/selftests/vm/ is a bit interesting in that > run_vmtests.sh kind of circumvents the "standard" model for running > kselftests. In the "standard" model (which I'm putting in quotes because > there's no formal, prescriptive way to use kselftests), testcases are > specified as Makefile targets that are put into one of several possible > Make variables to inform how they should be built and run. This is > described in more details in [0]. > > [0]: https://docs.kernel.org/dev-tools/kselftest.html?highlight=kselftest#contributing-new-tests-details > > As it relates to vmtests, we have the following relevant variables: > > TEST_GEN_FILES > -------------- > > Contains targets that generate executable ELF binaries that are _not_ > automatically run by the kselftests runner, but which are built. These are > the ELF executables that we run in run_vmtests.sh, sometimes with multiple > invocations using different parameters (for e.g. ksm_tests). > > TEST_GEN_PROGS > -------------- > > Contains targets that generate executable ELF binaries that _are_ > automatically run by the kselftest runner. Note that some of the executable > targets in the vm suite are not included in run_vmtests.sh, e.g. > soft-dirty, and split_huge_page_test. madv_populate actually is, which > looks like a bug to me (which was introduced in 749d999c06c2 ("selftests: > vm: bring common functions to a new file") and which I'll send out a fix > for). > > TEST_PROGS > ---------- > > Contains test shell scripts. These are automatically invoked by th > kselftest runner as well, and (as far as I understand) from the perspective > of kselftest, are the exact same as TEST_GEN_PROGS with the exception that > of course they're not compiled. In the kselftest runner, if the executable > bit is not set, the runner will echo out a warning and then try to parse > the interpreter from the first line of the file to figure out how to invoke > it. > > My two cents are that the correct way to structure vm selftests is > something like the following: > > 1. For any testcase that's compiled directly into an ELF binary, and which > doesn't need to be invoked with any arguments, convert it to a > TEST_GEN_PROGS target in tools/testing/selftests/vm/Makefile, and remove > it from being invoked from run_vmtests.sh. > 2. For any testcase that's compiled directly into an ELF binary, but which > _does_ need to be invoked with multiple arguments and/or needs to be > invoked in some other special way such as va_128TBswitch, leave it in > TEST_GEN_FILES, and instead add a separate executable shell script wraps > it, and which lives in TEST_PROGS. This would be a nice cleanup, because > it would let us remove some of the custom logic at the top of > run_vmtests.sh that only applies to some of the testcases. > 3. Once (1) and (2) are done for all relevant targets, remove > run_vmtests.sh and instead rely on the kselftest runner. > > This would allow us to remove all custom runner logic from > tools/testing/selftests/vm, and to instead just rely on the kselftest > runner to do the heavy lifting for us. > > As it relates specifically to Adam's patch, IMO setting that script include > the executable bit does make sense, as it's in-line with the guidance for > kselftests in general, and I don't think we can assume that all of the > shell scripts will be bin/sh. > > FWIW, I'm happy to do the above cleanup if nobody objects (after Adam's > patch lands). > > Thanks, > David Andrew, have you had a chance to think about this? At the moment, the missing permission bit is causing the test suite to spit out an error. The original patch that introduced the file marked is as executable, but that got dropped at some point in the automation that merged it. This makes the situation worse than it was before the patch that introduced the wrapper: now no one can run the test. Thanks, Adam
On Fri, 22 Jul 2022 10:11:59 +0200 Adam Sindelar <adam@wowsignal.io> wrote: > Andrew, have you had a chance to think about this? At the moment, the > missing permission bit is causing the test suite to spit out an error. I added "selftests/vm: fix va_128TBswitch.sh permissions" to the mm-stable branch.
diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
old mode 100644
new mode 100755
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