> -----Original Message-----
> From: shuah
>
> On 9/19/19 12:55 PM, Alexandre Belloni wrote:
> > On 19/09/2019 11:06:44-0700, Kees Cook wrote:
> >> Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
> >> test") solves the problem of kselftest_harness.h-using binary tests
> >> possibly hanging forever. However, scripts and other binaries can still
> >> hang forever. This adds a global timeout to each test script run.
> >>
>
> Timeout is good, but really tests should not hang. So we have to somehow
> indicate that the test needs to be fixed.
Well, a test hanging is something that might indicate either a failing
in the test, or the detection of an actual problem.
>
> This timeout is a band-aid and not real solution for the problem. This
> arbitrary value doesn't take into account that the test(s) in that
> particular directory (TARGET) could be running normally and working
> through all the tests.
Yes. Also, having a single timeout value doesn't reflect the need
to test on different hardware with potentially huge differences in
CPU speed (or other resource performance differences).
Since kselftest has a policy of having each test run quickly, this is somewhat
mitigated. But still, some embedded boards are running magnitudes
slower than your "average" enterprise Linux machine. Finding a timeout
that can handle all hardware cases is difficult.
-- Tim
>
> We need some way to differentiate the two cases.
Agreed.
>
> >> To make this configurable (e.g. as needed in the "rtc" test case),
> >> include a new per-test-directory "settings" file (similar to "config")
> >> that can contain kselftest-specific settings. The first recognized field
> >> is "timeout".
> >>
> >
> > Seems good to me. I was also wondering whether this is actually
> > reasonable to have tests running for so long. I wanted to discuss that
> > at LPC but I missed the session.
> >
>
> There is the individual test times and overall kselftest run time. We
> have lots of tests now and it does take long.
>
> >> Additionally, this splits the reporting for timeouts into a specific
> >> "TIMEOUT" not-ok (and adds exit code reporting in the remaining case).
> >>
> >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> >> ---
> >> tools/testing/selftests/kselftest/runner.sh | 36
> +++++++++++++++++++--
> >> tools/testing/selftests/rtc/settings | 1 +
> >> 2 files changed, 34 insertions(+), 3 deletions(-)
> >> create mode 100644 tools/testing/selftests/rtc/settings
> >>
> >> diff --git a/tools/testing/selftests/kselftest/runner.sh
> b/tools/testing/selftests/kselftest/runner.sh
> >> index 00c9020bdda8..84de7bc74f2c 100644
> >> --- a/tools/testing/selftests/kselftest/runner.sh
> >> +++ b/tools/testing/selftests/kselftest/runner.sh
> >> @@ -3,9 +3,14 @@
> >> #
> >> # Runs a set of tests in a given subdirectory.
> >> export skip_rc=4
> >> +export timeout_rc=124
> >> export logfile=/dev/stdout
> >> export per_test_logging=
> >>
> >> +# Defaults for "settings" file fields:
> >> +# "timeout" how many seconds to let each test run before failing.
> >> +export kselftest_default_timeout=45
> >> +
> >> # 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.
> >> if [ -z "$BASE_DIR" ]; then
> >> @@ -24,6 +29,16 @@ tap_prefix()
> >> fi
> >> }
> >>
> >> +tap_timeout()
> >> +{
> >> + # Make sure tests will time out if utility is available.
> >> + if [ -x /usr/bin/timeout ] ; then
> >> + /usr/bin/timeout "$kselftest_timeout" "$1"
> >> + else
> >> + "$1"
> >> + fi
> >> +}
> >> +
> >> run_one()
> >> {
> >> DIR="$1"
> >> @@ -32,6 +47,18 @@ run_one()
> >>
> >> BASENAME_TEST=$(basename $TEST)
> >>
> >> + # Reset any "settings"-file variables.
> >> + export kselftest_timeout="$kselftest_default_timeout"
> >> + # Load per-test-directory kselftest "settings" file.
> >> + settings="$BASE_DIR/$DIR/settings"
> >> + if [ -r "$settings" ] ; then
> >> + while read line ; do
> >> + field=$(echo "$line" | cut -d= -f1)
> >> + value=$(echo "$line" | cut -d= -f2-)
> >> + eval "kselftest_$field"="$value"
> >> + done < "$settings"
> >> + fi
> >> +
> >> TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
> >> echo "# $TEST_HDR_MSG"
> >> if [ ! -x "$TEST" ]; then
> >> @@ -44,14 +71,17 @@ 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") ||
> >> - (if [ $? -eq $skip_rc ]; then \
> >> + (rc=$?; \
> >> + if [ $rc -eq $skip_rc ]; then \
> >> echo "not ok $test_num $TEST_HDR_MSG # SKIP"
> >> + elif [ $rc -eq $timeout_rc ]; then \
> >> + echo "not ok $test_num $TEST_HDR_MSG #
> TIMEOUT"
> >> else
> >> - echo "not ok $test_num $TEST_HDR_MSG"
> >> + echo "not ok $test_num $TEST_HDR_MSG #
> exit=$rc"
> >> fi)
> >> cd - >/dev/null
> >> fi
> >> diff --git a/tools/testing/selftests/rtc/settings
> b/tools/testing/selftests/rtc/settings
> >> new file mode 100644
> >> index 000000000000..ba4d85f74cd6
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/rtc/settings
> >> @@ -0,0 +1 @@
> >> +timeout=90
> >> --
> >> 2.17.1
> >>
> >>
> >> --
> >> Kees Cook
> >
>
> thanks,
> -- Shuah
