On Wed, Jun 12, 2019 at 06:41:46PM +0200, Anders Roxell wrote:
> Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
> test") solves that binary tests doesn't hang forever. However, scripts
> can still hang forever, this adds an timeout to each test script run. This
> assumes that an individual test doesn't take longer than 30 seconds.
>
> Signed-off-by: Anders Roxell <anders.roxell@xxxxxxxxxx>
> ---
> tools/testing/selftests/kselftest/runner.sh | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 00c9020bdda8..cff7d2d83648 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -5,6 +5,7 @@
> export skip_rc=4
> export logfile=/dev/stdout
> export per_test_logging=
> +export TEST_TIMEOUT_DEFAULT=30
I would name this "kselftest_timeout" to avoid namespace collisions and
to drop the "default" name, since while setting it here is a default,
when used by other parts of the test environment, we'll be changing it
to non-default, so it's weird to have "default" in the name.
>
> # There isn't a shell-agnostic way to find the path of a sourced file,
> # so we must rely on BASE_DIR being set to find other tools.
> @@ -24,6 +25,14 @@ tap_prefix()
> fi
> }
>
> +tap_timeout()
> +{
> + if [ -x /usr/bin/timeout ] && [ -x "$BASENAME_TEST" ] \
The -x test on BASENAME_TEST is already done earlier in run_one(). Also,
this needs to be "$1" (the argument to tap_timeout).
> + && file $BASENAME_TEST |grep -q "shell script"; then
"file" seems pretty heavy here. How about "head -c2":
&& [ '#!' = "$(head -c2 "$1")" ]
> + echo -n "timeout $TEST_TIMEOUT_DEFAULT"
And this needs to actually _run_ it, not echo anything.
> + fi
> +}
I would expect tap_timeout() to be:
tap_timeout()
{
# If we have the "timeout" tool and the test is a script,
# set our timeout.
if [ -x /usr/bin/timeout ] &&
[ '#!' = "$(head -c2 "$1")" ] ; then
/usr/bin/timeout "$kselftest_timeout" "$1"
else
"$1"
fi
}
Another thought: should kselftest be changed so that this is the ONLY
timeout mechanism? i.e. remove the binary timeout code I originally
added, and just use this instead? Then we don't have to distinguish
between script and non-script, etc.
-Kees
> +
> run_one()
> {
> DIR="$1"
> @@ -44,7 +53,7 @@ run_one()
> echo "not ok $test_num $TEST_HDR_MSG"
> else
> cd `dirname $TEST` > /dev/null
> - (((((./$BASENAME_TEST 2>&1; echo $? >&3) |
> + ((((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |
> tap_prefix >&4) 3>&1) |
> (read xs; exit $xs)) 4>>"$logfile" &&
> echo "ok $test_num $TEST_HDR_MSG") ||
> --
> 2.11.0
>
--
Kees Cook
