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