[PATCH v2 0/8] selftests: Move test output to diagnostic lines

[PATCH v2 0/8] selftests: Move test output to diagnostic lines

[Kees Cook]

This refactors the selftest Makefiles to extract the test running logic
to be reused between "run_tests" and "emit_tests", while also fixing
up the test output to be TAP version 13 compliant:
- added "plan" line
- fixed result line syntax
- moved all test output to be "# "-prefixed as TAP "diagnostic" lines

The prefixing code includes a fallback mode for limited execution
environments.

Additionally, the plan lines are fixed for all callers of kselftest.h.

-Kees

v2:
- fix external make variable "summary=1" through-out series (shuah)
- fix plan line output for all kselftest.h users

Kees Cook (8):
  selftests: Extract single-test shell logic from lib.mk
  selftests: Use runner.sh for emit targets
  selftests: Extract logic for multiple test runs
  selftests: Add plan line and fix result line syntax
  selftests: Distinguish between missing and non-executable
  selftests: Move test output to diagnostic lines
  selftests: Remove KSFT_TAP_LEVEL
  selftests: Add test plan API to kselftest.h and adjust callers

 tools/testing/selftests/.gitignore            |  1 -
 tools/testing/selftests/Makefile              | 24 ++----
 .../selftests/breakpoints/breakpoint_test.c   | 15 +++-
 .../breakpoints/breakpoint_test_arm64.c       |  3 +-
 .../breakpoints/step_after_suspend_test.c     |  8 ++
 .../selftests/capabilities/test_execve.c      |  6 +-
 .../futex/functional/futex_requeue_pi.c       |  1 +
 .../futex_requeue_pi_mismatched_ops.c         |  1 +
 .../futex_requeue_pi_signal_restart.c         |  1 +
 .../futex_wait_private_mapped_file.c          |  1 +
 .../futex/functional/futex_wait_timeout.c     |  1 +
 .../futex_wait_uninitialized_heap.c           |  1 +
 .../futex/functional/futex_wait_wouldblock.c  |  1 +
 tools/testing/selftests/kselftest.h           | 17 +++-
 tools/testing/selftests/kselftest/prefix.pl   | 23 +++++
 tools/testing/selftests/kselftest/runner.sh   | 86 +++++++++++++++++++
 tools/testing/selftests/lib.mk                | 64 +++-----------
 .../selftests/membarrier/membarrier_test.c    |  1 +
 tools/testing/selftests/pidfd/pidfd_test.c    |  1 +
 tools/testing/selftests/sigaltstack/sas.c     |  1 +
 tools/testing/selftests/sync/sync_test.c      |  1 +
 21 files changed, 178 insertions(+), 80 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl
 create mode 100644 tools/testing/selftests/kselftest/runner.sh

-- 
2.17.1

[PATCH v2 1/8] selftests: Extract single-test shell logic from lib.mk

[Kees Cook]

In order to improve the reusability of the kselftest test running logic,
this extracts the single-test logic from lib.mk into kselftest/runner.sh
which lib.mk can call directly. No changes in output.

As part of the change, this moves the "summary" Makefile logic around
to set a new "logfile" output. This will be used again in the future
"emit_tests" target as well.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/.gitignore          |  1 -
 tools/testing/selftests/kselftest/runner.sh | 32 ++++++++++++++++++
 tools/testing/selftests/lib.mk              | 37 ++++-----------------
 3 files changed, 39 insertions(+), 31 deletions(-)
 create mode 100644 tools/testing/selftests/kselftest/runner.sh

diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore
index 91750352459d..8059ce834247 100644
--- a/tools/testing/selftests/.gitignore
+++ b/tools/testing/selftests/.gitignore
@@ -1,4 +1,3 @@
-kselftest
 gpiogpio-event-mon
 gpiogpio-hammer
 gpioinclude/
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
new file mode 100644
index 000000000000..e1117d703887
--- /dev/null
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Runs a set of tests in a given subdirectory.
+export skip_rc=4
+export logfile=/dev/stdout
+
+run_one()
+{
+	TEST="$1"
+	NUM="$2"
+
+	BASENAME_TEST=$(basename $TEST)
+
+	TEST_HDR_MSG="selftests: "`basename $PWD`:" $BASENAME_TEST"
+	echo "$TEST_HDR_MSG"
+	echo "========================================"
+	if [ ! -x "$TEST" ]; then
+		echo "$TEST_HDR_MSG: Warning: file $TEST is not executable, correct this."
+		echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]"
+	else
+		cd `dirname $TEST` > /dev/null
+		(./$BASENAME_TEST >> "$logfile" 2>&1 &&
+		echo "ok 1..$test_num $TEST_HDR_MSG [PASS]") ||
+		(if [ $? -eq $skip_rc ]; then	\
+			echo "not ok 1..$test_num $TEST_HDR_MSG [SKIP]"
+		else
+			echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]"
+		fi)
+		cd - >/dev/null
+	fi
+}
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 5979fdc4f36c..9d2b3c303bfa 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -14,6 +14,7 @@ ifeq (0,$(MAKELEVEL))
 	endif
     endif
 endif
+selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
 
 # The following are built by lib.mk common compile rules.
 # TEST_CUSTOM_PROGS should be used by tests that require
@@ -65,43 +66,19 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
 endif
 
 .ONESHELL:
-define RUN_TEST_PRINT_RESULT
-	TEST_HDR_MSG="selftests: "`basename $$PWD`:" $$BASENAME_TEST";	\
-	echo $$TEST_HDR_MSG;					\
-	echo "========================================";	\
-	if [ ! -x $$TEST ]; then	\
-		echo "$$TEST_HDR_MSG: Warning: file $$BASENAME_TEST is not executable, correct this.";\
-		echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \
-	else					\
-		cd `dirname $$TEST` > /dev/null; \
-		if [ "X$(summary)" != "X" ]; then	\
-			(./$$BASENAME_TEST > /tmp/$$BASENAME_TEST 2>&1 && \
-			echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") || \
-			(if [ $$? -eq $$skip ]; then	\
-				echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]";				\
-			else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]";					\
-			fi;)			\
-		else				\
-			(./$$BASENAME_TEST &&	\
-			echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") ||						\
-			(if [ $$? -eq $$skip ]; then \
-				echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]"; \
-			else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]";				\
-			fi;)		\
-		fi;				\
-		cd - > /dev/null;		\
-	fi;
-endef
-
 define RUN_TESTS
 	@export KSFT_TAP_LEVEL=`echo 1`;		\
 	test_num=`echo 0`;				\
-	skip=`echo 4`;					\
+	. $(selfdir)/kselftest/runner.sh;		\
 	echo "TAP version 13";				\
 	for TEST in $(1); do				\
 		BASENAME_TEST=`basename $$TEST`;	\
 		test_num=`echo $$test_num+1 | bc`;	\
-		$(call RUN_TEST_PRINT_RESULT,$(TEST),$(BASENAME_TEST),$(test_num),$(skip))						\
+		if [ "X$(summary)" != "X" ]; then	\
+		        logfile="/tmp/$$BASENAME_TEST";	\
+			cat /dev/null > "$$logfile";	\
+		fi;					\
+		run_one "$$BASENAME_TEST" "$$test_num";	\
 	done;
 endef
 
-- 
2.17.1

[PATCH v2 2/8] selftests: Use runner.sh for emit targets

[Kees Cook]

This reuses the new runner.sh for the emit targets instead of manually
running each test via run_kselftest.sh.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/Makefile | 11 +++++------
 tools/testing/selftests/lib.mk   | 15 ++-------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index f2ebf8cf4686..c5f9f736cdbd 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -176,7 +176,8 @@ ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh
 install:
 ifdef INSTALL_PATH
 	@# Ask all targets to install their files
-	mkdir -p $(INSTALL_PATH)
+	mkdir -p $(INSTALL_PATH)/kselftest
+	install -m 744 kselftest/runner.sh $(INSTALL_PATH)/kselftest/
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
 		make OUTPUT=$$BUILD_TARGET -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install; \
@@ -186,15 +187,13 @@ ifdef INSTALL_PATH
 	echo "#!/bin/sh" > $(ALL_SCRIPT)
 	echo "BASE_DIR=\$$(realpath \$$(dirname \$$0))" >> $(ALL_SCRIPT)
 	echo "cd \$$BASE_DIR" >> $(ALL_SCRIPT)
+	echo ". ./kselftest/runner.sh" >> $(ALL_SCRIPT)
 	echo "ROOT=\$$PWD" >> $(ALL_SCRIPT)
 	echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT)
-	echo "  OUTPUT=\$$BASE_DIR/output.log" >> $(ALL_SCRIPT)
-	echo "  cat /dev/null > \$$OUTPUT" >> $(ALL_SCRIPT)
-	echo "else" >> $(ALL_SCRIPT)
-	echo "  OUTPUT=/dev/stdout" >> $(ALL_SCRIPT)
+	echo "  logfile=\$$BASE_DIR/output.log" >> $(ALL_SCRIPT)
+	echo "  cat /dev/null > \$$logfile" >> $(ALL_SCRIPT)
 	echo "fi" >> $(ALL_SCRIPT)
 	echo "export KSFT_TAP_LEVEL=1" >> $(ALL_SCRIPT)
-	echo "export skip=4" >> $(ALL_SCRIPT)
 
 	for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 9d2b3c303bfa..6b2d026a94ea 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -116,24 +116,13 @@ else
 	$(error Error: set INSTALL_PATH to use install)
 endif
 
-define EMIT_TESTS
+emit_tests:
 	@test_num=`echo 0`;				\
 	for TEST in $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS); do \
 		BASENAME_TEST=`basename $$TEST`;	\
 		test_num=`echo $$test_num+1 | bc`;	\
-		TEST_HDR_MSG="selftests: "`basename $$PWD`:" $$BASENAME_TEST";	\
-		echo "echo $$TEST_HDR_MSG";	\
-		if [ ! -x $$TEST ]; then	\
-			echo "echo \"$$TEST_HDR_MSG: Warning: file $$BASENAME_TEST is not executable, correct this.\"";		\
-			echo "echo \"not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]\""; \
-		else
-			echo "(./$$BASENAME_TEST >> \$$OUTPUT 2>&1 && echo \"ok 1..$$test_num $$TEST_HDR_MSG [PASS]\") || (if [ \$$? -eq \$$skip ]; then echo \"not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]\"; else echo \"not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]\"; fi;)"; \
-		fi;		\
+		echo "run_one \"$$BASENAME_TEST\" \"$$test_num\"";	\
 	done;
-endef
-
-emit_tests:
-	$(EMIT_TESTS)
 
 # define if isn't already. It is undefined in make O= case.
 ifeq ($(RM),)
-- 
2.17.1

[PATCH v2 3/8] selftests: Extract logic for multiple test runs

[Kees Cook]

This moves the logic for running multiple tests into a single "run_many"
function of runner.sh. Both "run_tests" and "emit_tests" are modified to
use it. Summary handling is now controlled by the "per_test_logging"
shell flag.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/Makefile            |  6 ++---
 tools/testing/selftests/kselftest/runner.sh | 25 ++++++++++++++++++---
 tools/testing/selftests/lib.mk              | 25 +++++++--------------
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index c5f9f736cdbd..4ac1d1c7ce5b 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -193,16 +193,14 @@ ifdef INSTALL_PATH
 	echo "  logfile=\$$BASE_DIR/output.log" >> $(ALL_SCRIPT)
 	echo "  cat /dev/null > \$$logfile" >> $(ALL_SCRIPT)
 	echo "fi" >> $(ALL_SCRIPT)
-	echo "export KSFT_TAP_LEVEL=1" >> $(ALL_SCRIPT)
 
 	for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		echo "echo ; echo TAP version 13" >> $(ALL_SCRIPT);	\
-		echo "echo Running tests in $$TARGET" >> $(ALL_SCRIPT); \
-		echo "echo ========================================" >> $(ALL_SCRIPT); \
 		echo "[ -w /dev/kmsg ] && echo \"kselftest: Running tests in $$TARGET\" >> /dev/kmsg" >> $(ALL_SCRIPT); \
 		echo "cd $$TARGET" >> $(ALL_SCRIPT); \
+		echo -n "run_many" >> $(ALL_SCRIPT); \
 		make -s --no-print-directory OUTPUT=$$BUILD_TARGET -C $$TARGET emit_tests >> $(ALL_SCRIPT); \
+		echo "" >> $(ALL_SCRIPT);	    \
 		echo "cd \$$ROOT" >> $(ALL_SCRIPT); \
 	done;
 
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index e1117d703887..f12b0a631273 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -2,17 +2,20 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 # Runs a set of tests in a given subdirectory.
+export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
+export per_test_logging=
 
 run_one()
 {
-	TEST="$1"
-	NUM="$2"
+	DIR="$1"
+	TEST="$2"
+	NUM="$3"
 
 	BASENAME_TEST=$(basename $TEST)
 
-	TEST_HDR_MSG="selftests: "`basename $PWD`:" $BASENAME_TEST"
+	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
 	echo "$TEST_HDR_MSG"
 	echo "========================================"
 	if [ ! -x "$TEST" ]; then
@@ -30,3 +33,19 @@ run_one()
 		cd - >/dev/null
 	fi
 }
+
+run_many()
+{
+	echo "TAP version 13"
+	DIR=$(basename "$PWD")
+	test_num=0
+	for TEST in "$@"; do
+		BASENAME_TEST=$(basename $TEST)
+		test_num=$(( test_num + 1 ))
+		if [ -n "$per_test_logging" ]; then
+			logfile="/tmp/$BASENAME_TEST"
+			cat /dev/null > "$logfile"
+		fi
+		run_one "$DIR" "$TEST" "$test_num"
+	done
+}
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 6b2d026a94ea..28b8ffedfdf1 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -67,19 +67,11 @@ endif
 
 .ONESHELL:
 define RUN_TESTS
-	@export KSFT_TAP_LEVEL=`echo 1`;		\
-	test_num=`echo 0`;				\
-	. $(selfdir)/kselftest/runner.sh;		\
-	echo "TAP version 13";				\
-	for TEST in $(1); do				\
-		BASENAME_TEST=`basename $$TEST`;	\
-		test_num=`echo $$test_num+1 | bc`;	\
-		if [ "X$(summary)" != "X" ]; then	\
-		        logfile="/tmp/$$BASENAME_TEST";	\
-			cat /dev/null > "$$logfile";	\
-		fi;					\
-		run_one "$$BASENAME_TEST" "$$test_num";	\
-	done;
+	@. $(selfdir)/kselftest/runner.sh;	\
+	if [ "X$(summary)" != "X" ]; then       \
+		per_test_logging=1;		\
+	fi;                                     \
+	run_many $(1)
 endef
 
 run_tests: all
@@ -117,12 +109,11 @@ else
 endif
 
 emit_tests:
-	@test_num=`echo 0`;				\
 	for TEST in $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS); do \
 		BASENAME_TEST=`basename $$TEST`;	\
-		test_num=`echo $$test_num+1 | bc`;	\
-		echo "run_one \"$$BASENAME_TEST\" \"$$test_num\"";	\
-	done;
+		echo "	\\";				\
+		echo -n "	\"$$BASENAME_TEST\"";	\
+	done;						\
 
 # define if isn't already. It is undefined in make O= case.
 ifeq ($(RM),)
-- 
2.17.1

[PATCH v2 4/8] selftests: Add plan line and fix result line syntax

[Kees Cook]

The TAP version 13 spec requires a "plan" line, which has been missing.
Since we always know how many tests we're going to run, emit the count on
the plan line. This also fixes the result lines to remove the "1.." prefix
which is against spec, and to mark skips with the correct "# SKIP" suffix.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest.h         |  4 ++--
 tools/testing/selftests/kselftest/runner.sh | 10 ++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 47e1d995c182..9f4147a6fdbc 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -111,7 +111,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
 	ksft_cnt.ksft_xskip++;
 
 	va_start(args, msg);
-	printf("ok %d # skip ", ksft_test_num());
+	printf("not ok %d # SKIP ", ksft_test_num());
 	vprintf(msg, args);
 	va_end(args);
 }
@@ -172,7 +172,7 @@ static inline int ksft_exit_skip(const char *msg, ...)
 		va_list args;
 
 		va_start(args, msg);
-		printf("1..%d # Skipped: ", ksft_test_num());
+		printf("not ok %d # SKIP ", ksft_test_num());
 		vprintf(msg, args);
 		va_end(args);
 	} else {
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index f12b0a631273..e0621974e01e 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -20,15 +20,15 @@ run_one()
 	echo "========================================"
 	if [ ! -x "$TEST" ]; then
 		echo "$TEST_HDR_MSG: Warning: file $TEST is not executable, correct this."
-		echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]"
+		echo "not ok $test_num $TEST_HDR_MSG"
 	else
 		cd `dirname $TEST` > /dev/null
 		(./$BASENAME_TEST >> "$logfile" 2>&1 &&
-		echo "ok 1..$test_num $TEST_HDR_MSG [PASS]") ||
+		echo "ok $test_num $TEST_HDR_MSG") ||
 		(if [ $? -eq $skip_rc ]; then	\
-			echo "not ok 1..$test_num $TEST_HDR_MSG [SKIP]"
+			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
 		else
-			echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]"
+			echo "not ok $test_num $TEST_HDR_MSG"
 		fi)
 		cd - >/dev/null
 	fi
@@ -39,6 +39,8 @@ run_many()
 	echo "TAP version 13"
 	DIR=$(basename "$PWD")
 	test_num=0
+	total=$(echo "$@" | wc -w)
+	echo "1..$total"
 	for TEST in "$@"; do
 		BASENAME_TEST=$(basename $TEST)
 		test_num=$(( test_num + 1 ))
-- 
2.17.1

[PATCH v2 5/8] selftests: Distinguish between missing and non-executable

[Kees Cook]

If a test was missing (e.g. wrong architecture, etc), the test runner
would incorrectly claim the test was non-executable. This adds an
existence check to report correctly.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest/runner.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index e0621974e01e..a66fb64e61e9 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -19,7 +19,12 @@ run_one()
 	echo "$TEST_HDR_MSG"
 	echo "========================================"
 	if [ ! -x "$TEST" ]; then
-		echo "$TEST_HDR_MSG: Warning: file $TEST is not executable, correct this."
+		echo -n "$TEST_HDR_MSG: Warning: file $TEST is "
+		if [ ! -e "$TEST" ]; then
+			echo "missing!"
+		else
+			echo "not executable, correct this."
+		fi
 		echo "not ok $test_num $TEST_HDR_MSG"
 	else
 		cd `dirname $TEST` > /dev/null
-- 
2.17.1

[PATCH v2 6/8] selftests: Move test output to diagnostic lines

[Kees Cook]

This changes the selftest output so that each test's output is prefixed
with "# " as a TAP "diagnostic line".

This creates a bit of a kernel-specific TAP dialect where the diagnostics
precede the results. The TAP spec isn't entirely clear about this, though,
so I think it's the correct solution so as to keep interactive runs making
sense. If the output _followed_ the result line in the spec-suggested
YAML form, each test would dump all of its output at once instead of as
it went, making debugging harder.

This does, however, solve the recursive TAP output problem, as sub-tests
will simply be prefixed by "# ". Parsing sub-tests becomes a simple
problem of just removing the first two characters of a given top-level
test's diagnostic output, and parsing the results.

Note that the shell construct needed to both get an exit code from
the first command in a pipe and still filter the pipe (to add the "# "
prefix) uses a POSIX solution rather than the bash "pipefail" option
which is not supported by dash.

Since some test environments may have a very minimal set of utilities
available, the new prefixing code will fall back to doing line-at-a-time
prefixing if perl and/or stdbuf are not available.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/Makefile            |  1 +
 tools/testing/selftests/kselftest.h         |  2 +-
 tools/testing/selftests/kselftest/prefix.pl | 23 +++++++++++++
 tools/testing/selftests/kselftest/runner.sh | 37 ++++++++++++++++++---
 tools/testing/selftests/lib.mk              |  3 +-
 5 files changed, 60 insertions(+), 6 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 4ac1d1c7ce5b..64699f59b95f 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -178,6 +178,7 @@ ifdef INSTALL_PATH
 	@# Ask all targets to install their files
 	mkdir -p $(INSTALL_PATH)/kselftest
 	install -m 744 kselftest/runner.sh $(INSTALL_PATH)/kselftest/
+	install -m 744 kselftest/prefix.pl $(INSTALL_PATH)/kselftest/
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
 		make OUTPUT=$$BUILD_TARGET -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install; \
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 9f4147a6fdbc..7f078e79a9fa 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -63,7 +63,7 @@ static inline void ksft_print_header(void)
 
 static inline void ksft_print_cnts(void)
 {
-	printf("Pass %d Fail %d Xfail %d Xpass %d Skip %d Error %d\n",
+	printf("# Pass %d Fail %d Xfail %d Xpass %d Skip %d Error %d\n",
 		ksft_cnt.ksft_pass, ksft_cnt.ksft_fail,
 		ksft_cnt.ksft_xfail, ksft_cnt.ksft_xpass,
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
diff --git a/tools/testing/selftests/kselftest/prefix.pl b/tools/testing/selftests/kselftest/prefix.pl
new file mode 100755
index 000000000000..ec7e48118183
--- /dev/null
+++ b/tools/testing/selftests/kselftest/prefix.pl
@@ -0,0 +1,23 @@
+#!/usr/bin/perl
+# SPDX-License-Identifier: GPL-2.0
+# Prefix all lines with "# ", unbuffered. Command being piped in may need
+# to have unbuffering forced with "stdbuf -i0 -o0 -e0 $cmd".
+use strict;
+
+binmode STDIN;
+binmode STDOUT;
+
+STDOUT->autoflush(1);
+
+my $needed = 1;
+while (1) {
+	my $char;
+	my $bytes = sysread(STDIN, $char, 1);
+	exit 0 if ($bytes == 0);
+	if ($needed) {
+		print "# ";
+		$needed = 0;
+	}
+	print $char;
+	$needed = 1 if ($char eq "\n");
+}
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index a66fb64e61e9..b9f74e5a2ee5 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -7,6 +7,34 @@ export skip_rc=4
 export logfile=/dev/stdout
 export per_test_logging=
 
+# 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
+	echo "Error: BASE_DIR must be set before sourcing." >&2
+	exit 1
+fi
+
+# If Perl is unavailable, we must fall back to line-at-a-time prefixing
+# with sed instead of unbuffered output.
+tap_prefix()
+{
+	if [ ! -x /usr/bin/perl ]; then
+		sed -e 's/^/# /'
+	else
+		"$BASE_DIR"/kselftest/prefix.pl
+	fi
+}
+
+# If stdbuf is unavailable, we must fall back to line-at-a-time piping.
+tap_unbuffer()
+{
+	if ! which stdbuf >/dev/null ; then
+		"$@"
+	else
+		stdbuf -i0 -o0 -e0 "$@"
+	fi
+}
+
 run_one()
 {
 	DIR="$1"
@@ -16,10 +44,9 @@ run_one()
 	BASENAME_TEST=$(basename $TEST)
 
 	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
-	echo "$TEST_HDR_MSG"
-	echo "========================================"
+	echo "# $TEST_HDR_MSG"
 	if [ ! -x "$TEST" ]; then
-		echo -n "$TEST_HDR_MSG: Warning: file $TEST is "
+		echo -n "# Warning: file $TEST is "
 		if [ ! -e "$TEST" ]; then
 			echo "missing!"
 		else
@@ -28,7 +55,9 @@ run_one()
 		echo "not ok $test_num $TEST_HDR_MSG"
 	else
 		cd `dirname $TEST` > /dev/null
-		(./$BASENAME_TEST >> "$logfile" 2>&1 &&
+		(((((tap_unbuffer ./$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	\
 			echo "not ok $test_num $TEST_HDR_MSG # SKIP"
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 28b8ffedfdf1..098dd0065fb1 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -67,7 +67,8 @@ endif
 
 .ONESHELL:
 define RUN_TESTS
-	@. $(selfdir)/kselftest/runner.sh;	\
+	@BASE_DIR="$(selfdir)";			\
+	. $(selfdir)/kselftest/runner.sh;	\
 	if [ "X$(summary)" != "X" ]; then       \
 		per_test_logging=1;		\
 	fi;                                     \
-- 
2.17.1

[PATCH v2 7/8] selftests: Remove KSFT_TAP_LEVEL

[Kees Cook]

Since sub-testing can now be detected by indentation level, this removes
KSFT_TAP_LEVEL so that subtests report their TAP header for later parsing.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/Makefile            | 6 ------
 tools/testing/selftests/kselftest/runner.sh | 1 -
 2 files changed, 7 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 64699f59b95f..9f05448e5e4b 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -86,12 +86,6 @@ else
 	endif
 endif
 
-# KSFT_TAP_LEVEL is used from KSFT framework to prevent nested TAP header
-# printing from tests. Applicable to run_tests case where run_tests adds
-# TAP header prior running tests and when a test program invokes another
-# with system() call. Export it here to cover override RUN_TESTS defines.
-export KSFT_TAP_LEVEL=`echo 1`
-
 # Prepare for headers install
 top_srcdir ?= ../../..
 include $(top_srcdir)/scripts/subarch.include
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index b9f74e5a2ee5..eff3ee303d0d 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -2,7 +2,6 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 # Runs a set of tests in a given subdirectory.
-export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
 export per_test_logging=
-- 
2.17.1

[PATCH v2 8/8] selftests: Add test plan API to kselftest.h and adjust callers

[Kees Cook]

The test plan for TAP needs to be declared immediately after the header.
This adds the test plan API to kselftest.h and updates all callers to
declare their expected test counts.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../selftests/breakpoints/breakpoint_test.c       | 15 ++++++++++++---
 .../selftests/breakpoints/breakpoint_test_arm64.c |  3 ++-
 .../breakpoints/step_after_suspend_test.c         |  8 ++++++++
 .../testing/selftests/capabilities/test_execve.c  |  6 ++++--
 .../selftests/futex/functional/futex_requeue_pi.c |  1 +
 .../functional/futex_requeue_pi_mismatched_ops.c  |  1 +
 .../functional/futex_requeue_pi_signal_restart.c  |  1 +
 .../functional/futex_wait_private_mapped_file.c   |  1 +
 .../futex/functional/futex_wait_timeout.c         |  1 +
 .../functional/futex_wait_uninitialized_heap.c    |  1 +
 .../futex/functional/futex_wait_wouldblock.c      |  1 +
 tools/testing/selftests/kselftest.h               | 13 +++++++++++--
 .../selftests/membarrier/membarrier_test.c        |  1 +
 tools/testing/selftests/pidfd/pidfd_test.c        |  1 +
 tools/testing/selftests/sigaltstack/sas.c         |  1 +
 tools/testing/selftests/sync/sync_test.c          |  1 +
 16 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/breakpoints/breakpoint_test.c b/tools/testing/selftests/breakpoints/breakpoint_test.c
index 901b85ea6a59..8f3655e59020 100644
--- a/tools/testing/selftests/breakpoints/breakpoint_test.c
+++ b/tools/testing/selftests/breakpoints/breakpoint_test.c
@@ -21,6 +21,8 @@
 
 #include "../kselftest.h"
 
+#define COUNT_ISN_BPS	4
+#define COUNT_WPS	4
 
 /* Breakpoint access modes */
 enum {
@@ -220,7 +222,7 @@ static void trigger_tests(void)
 			if (!local && !global)
 				continue;
 
-			for (i = 0; i < 4; i++) {
+			for (i = 0; i < COUNT_ISN_BPS; i++) {
 				dummy_funcs[i]();
 				check_trapped();
 			}
@@ -292,7 +294,7 @@ static void launch_instruction_breakpoints(char *buf, int local, int global)
 {
 	int i;
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < COUNT_ISN_BPS; i++) {
 		set_breakpoint_addr(dummy_funcs[i], i);
 		toggle_breakpoint(i, BP_X, 1, local, global, 1);
 		ptrace(PTRACE_CONT, child_pid, NULL, 0);
@@ -314,7 +316,7 @@ static void launch_watchpoints(char *buf, int mode, int len,
 	else
 		mode_str = "read";
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < COUNT_WPS; i++) {
 		set_breakpoint_addr(&dummy_var[i], i);
 		toggle_breakpoint(i, mode, len, local, global, 1);
 		ptrace(PTRACE_CONT, child_pid, NULL, 0);
@@ -330,8 +332,15 @@ static void launch_watchpoints(char *buf, int mode, int len,
 static void launch_tests(void)
 {
 	char buf[1024];
+	unsigned int tests = 0;
 	int len, local, global, i;
 
+	tests += 3 * COUNT_ISN_BPS;
+	tests += sizeof(long) / 2 * 3 * COUNT_WPS;
+	tests += sizeof(long) / 2 * 3 * COUNT_WPS;
+	tests += 2;
+	ksft_set_plan(tests);
+
 	/* Instruction breakpoints */
 	for (local = 0; local < 2; local++) {
 		for (global = 0; global < 2; global++) {
diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_arm64.c b/tools/testing/selftests/breakpoints/breakpoint_test_arm64.c
index 2d95e5adde72..ab59d814341a 100644
--- a/tools/testing/selftests/breakpoints/breakpoint_test_arm64.c
+++ b/tools/testing/selftests/breakpoints/breakpoint_test_arm64.c
@@ -118,7 +118,7 @@ static bool set_watchpoint(pid_t pid, int size, int wp)
 	return false;
 }
 
-static bool run_test(int wr_size, int wp_size, int wr, int wp)
+static bool arun_test(int wr_size, int wp_size, int wr, int wp)
 {
 	int status;
 	siginfo_t siginfo;
@@ -214,6 +214,7 @@ int main(int argc, char **argv)
 	bool result;
 
 	ksft_print_header();
+	ksft_set_plan(213);
 
 	act.sa_handler = sigalrm;
 	sigemptyset(&act.sa_mask);
diff --git a/tools/testing/selftests/breakpoints/step_after_suspend_test.c b/tools/testing/selftests/breakpoints/step_after_suspend_test.c
index f82dcc1f8841..cf868b5e00f7 100644
--- a/tools/testing/selftests/breakpoints/step_after_suspend_test.c
+++ b/tools/testing/selftests/breakpoints/step_after_suspend_test.c
@@ -173,6 +173,7 @@ int main(int argc, char **argv)
 	int opt;
 	bool do_suspend = true;
 	bool succeeded = true;
+	unsigned int tests = 0;
 	cpu_set_t available_cpus;
 	int err;
 	int cpu;
@@ -191,6 +192,13 @@ int main(int argc, char **argv)
 		}
 	}
 
+	for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
+		if (!CPU_ISSET(cpu, &available_cpus))
+			continue;
+		tests++;
+	}
+	ksft_set_plan(tests);
+
 	if (do_suspend)
 		suspend();
 
diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
index 3ab39a61b95b..df0ef02b4036 100644
--- a/tools/testing/selftests/capabilities/test_execve.c
+++ b/tools/testing/selftests/capabilities/test_execve.c
@@ -430,8 +430,6 @@ int main(int argc, char **argv)
 {
 	char *tmp1, *tmp2, *our_path;
 
-	ksft_print_header();
-
 	/* Find our path */
 	tmp1 = strdup(argv[0]);
 	if (!tmp1)
@@ -445,6 +443,8 @@ int main(int argc, char **argv)
 	mpid = getpid();
 
 	if (fork_wait()) {
+		ksft_print_header();
+		ksft_set_plan(12);
 		ksft_print_msg("[RUN]\t+++ Tests with uid == 0 +++\n");
 		return do_tests(0, our_path);
 	}
@@ -452,6 +452,8 @@ int main(int argc, char **argv)
 	ksft_print_msg("==================================================\n");
 
 	if (fork_wait()) {
+		ksft_print_header();
+		ksft_set_plan(9);
 		ksft_print_msg("[RUN]\t+++ Tests with uid != 0 +++\n");
 		return do_tests(1, our_path);
 	}
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
index 54cd5c414e82..8d20957f7586 100644
--- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c
+++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
@@ -395,6 +395,7 @@ int main(int argc, char *argv[])
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Test requeue functionality\n", basename(argv[0]));
 	ksft_print_msg(
 		"\tArguments: broadcast=%d locked=%d owner=%d timeout=%ldns\n",
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi_mismatched_ops.c b/tools/testing/selftests/futex/functional/futex_requeue_pi_mismatched_ops.c
index 08187a16507f..742624c59ba7 100644
--- a/tools/testing/selftests/futex/functional/futex_requeue_pi_mismatched_ops.c
+++ b/tools/testing/selftests/futex/functional/futex_requeue_pi_mismatched_ops.c
@@ -79,6 +79,7 @@ int main(int argc, char *argv[])
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Detect mismatched requeue_pi operations\n",
 	       basename(argv[0]));
 
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c b/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c
index f0542a344d95..a0f5934707ff 100644
--- a/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c
+++ b/tools/testing/selftests/futex/functional/futex_requeue_pi_signal_restart.c
@@ -144,6 +144,7 @@ int main(int argc, char *argv[])
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Test signal handling during requeue_pi\n",
 	       basename(argv[0]));
 	ksft_print_msg("\tArguments: <none>\n");
diff --git a/tools/testing/selftests/futex/functional/futex_wait_private_mapped_file.c b/tools/testing/selftests/futex/functional/futex_wait_private_mapped_file.c
index 6216de828093..a458d42ff86e 100644
--- a/tools/testing/selftests/futex/functional/futex_wait_private_mapped_file.c
+++ b/tools/testing/selftests/futex/functional/futex_wait_private_mapped_file.c
@@ -98,6 +98,7 @@ int main(int argc, char **argv)
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg(
 		"%s: Test the futex value of private file mappings in FUTEX_WAIT\n",
 		basename(argv[0]));
diff --git a/tools/testing/selftests/futex/functional/futex_wait_timeout.c b/tools/testing/selftests/futex/functional/futex_wait_timeout.c
index bab3dfe1787f..04b95478059c 100644
--- a/tools/testing/selftests/futex/functional/futex_wait_timeout.c
+++ b/tools/testing/selftests/futex/functional/futex_wait_timeout.c
@@ -69,6 +69,7 @@ int main(int argc, char *argv[])
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Block on a futex and wait for timeout\n",
 	       basename(argv[0]));
 	ksft_print_msg("\tArguments: timeout=%ldns\n", timeout_ns);
diff --git a/tools/testing/selftests/futex/functional/futex_wait_uninitialized_heap.c b/tools/testing/selftests/futex/functional/futex_wait_uninitialized_heap.c
index 26975322545b..3a1d12a14921 100644
--- a/tools/testing/selftests/futex/functional/futex_wait_uninitialized_heap.c
+++ b/tools/testing/selftests/futex/functional/futex_wait_uninitialized_heap.c
@@ -100,6 +100,7 @@ int main(int argc, char **argv)
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Test the uninitialized futex value in FUTEX_WAIT\n",
 	       basename(argv[0]));
 
diff --git a/tools/testing/selftests/futex/functional/futex_wait_wouldblock.c b/tools/testing/selftests/futex/functional/futex_wait_wouldblock.c
index da15a63269b4..a34a6bbc30ce 100644
--- a/tools/testing/selftests/futex/functional/futex_wait_wouldblock.c
+++ b/tools/testing/selftests/futex/functional/futex_wait_wouldblock.c
@@ -65,6 +65,7 @@ int main(int argc, char *argv[])
 	}
 
 	ksft_print_header();
+	ksft_set_plan(1);
 	ksft_print_msg("%s: Test the unexpected futex value in FUTEX_WAIT\n",
 	       basename(argv[0]));
 
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 7f078e79a9fa..ec15c4f6af55 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -33,6 +33,7 @@ struct ksft_count {
 };
 
 static struct ksft_count ksft_cnt;
+static unsigned int ksft_plan;
 
 static inline int ksft_test_num(void)
 {
@@ -61,13 +62,21 @@ static inline void ksft_print_header(void)
 		printf("TAP version 13\n");
 }
 
+static inline void ksft_set_plan(unsigned int plan)
+{
+	ksft_plan = plan;
+	printf("1..%d\n", ksft_plan);
+}
+
 static inline void ksft_print_cnts(void)
 {
+	if (ksft_plan != ksft_test_num())
+		printf("# Planned tests != run tests (%u != %u)\n",
+			ksft_plan, ksft_test_num());
 	printf("# Pass %d Fail %d Xfail %d Xpass %d Skip %d Error %d\n",
 		ksft_cnt.ksft_pass, ksft_cnt.ksft_fail,
 		ksft_cnt.ksft_xfail, ksft_cnt.ksft_xpass,
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
-	printf("1..%d\n", ksft_test_num());
 }
 
 static inline void ksft_print_msg(const char *msg, ...)
@@ -172,7 +181,7 @@ static inline int ksft_exit_skip(const char *msg, ...)
 		va_list args;
 
 		va_start(args, msg);
-		printf("not ok %d # SKIP ", ksft_test_num());
+		printf("not ok %d # SKIP ", 1 + ksft_test_num());
 		vprintf(msg, args);
 		va_end(args);
 	} else {
diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..70b4ddbf126b 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -304,6 +304,7 @@ static int test_membarrier_query(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
+	ksft_set_plan(13);
 
 	test_membarrier_query();
 	test_membarrier();
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index d59378a93782..5bae1792e3d6 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -371,6 +371,7 @@ static int test_pidfd_send_signal_syscall_support(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
+	ksft_set_plan(4);
 
 	test_pidfd_send_signal_syscall_support();
 	test_pidfd_send_signal_simple_success();
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 228c2ae47687..ad0f8df2ca0a 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -109,6 +109,7 @@ int main(void)
 	int err;
 
 	ksft_print_header();
+	ksft_set_plan(3);
 
 	sigemptyset(&act.sa_mask);
 	act.sa_flags = SA_ONSTACK | SA_SIGINFO;
diff --git a/tools/testing/selftests/sync/sync_test.c b/tools/testing/selftests/sync/sync_test.c
index 7f7938263c5c..3824b66f41a0 100644
--- a/tools/testing/selftests/sync/sync_test.c
+++ b/tools/testing/selftests/sync/sync_test.c
@@ -86,6 +86,7 @@ int main(void)
 	int err;
 
 	ksft_print_header();
+	ksft_set_plan(3 + 7);
 
 	sync_api_supported();
 
-- 
2.17.1

Re: [PATCH v2 7/8] selftests: Remove KSFT_TAP_LEVEL

[shuah]

On 4/24/19 5:12 PM, Kees Cook wrote:
> Since sub-testing can now be detected by indentation level, this removes
> KSFT_TAP_LEVEL so that subtests report their TAP header for later parsing.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   tools/testing/selftests/Makefile            | 6 ------
>   tools/testing/selftests/kselftest/runner.sh | 1 -
>   2 files changed, 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 64699f59b95f..9f05448e5e4b 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -86,12 +86,6 @@ else
>   	endif
>   endif
>   
> -# KSFT_TAP_LEVEL is used from KSFT framework to prevent nested TAP header
> -# printing from tests. Applicable to run_tests case where run_tests adds
> -# TAP header prior running tests and when a test program invokes another
> -# with system() call. Export it here to cover override RUN_TESTS defines.
> -export KSFT_TAP_LEVEL=`echo 1`
> -
>   # Prepare for headers install
>   top_srcdir ?= ../../..
>   include $(top_srcdir)/scripts/subarch.include
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index b9f74e5a2ee5..eff3ee303d0d 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -2,7 +2,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   #
>   # Runs a set of tests in a given subdirectory.
> -export KSFT_TAP_LEVEL=1
>   export skip_rc=4
>   export logfile=/dev/stdout
>   export per_test_logging=
> 

Does this take into ksft_print_header() getenv logic to avoid printing
TAP headers from tests when they fork? e.g timers tests do that a lot.

thanks,
-- Shuah

Re: [PATCH v2 0/8] selftests: Move test output to diagnostic lines

[shuah]

On 4/24/19 5:12 PM,  wrote:
> This refactors the selftest Makefiles to extract the test running logic
> to be reused between "run_tests" and "emit_tests", while also fixing
> up the test output to be TAP version 13 compliant:
> - added "plan" line
> - fixed result line syntax
> - moved all test output to be "# "-prefixed as TAP "diagnostic" lines
> 
> The prefixing code includes a fallback mode for limited execution
> environments.
> 
> Additionally, the plan lines are fixed for all callers of kselftest.h.
> 
> -Kees
> 

Kees,

Just about to apply these to a topic branch to do testing and ran into
checkpatch errors:


WARNING: line over 80 characters - a few
WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
#141: FILE: tools/testing/selftests/kselftest/runner.sh:2:

Can fix them and resend - SPDX one is my main concern.

The plan is to apply these to linux-kselftest ksft-tap-refactor topic
first. I don't want to rush these until we do some testing.

thanks,
-- Shuah

Re: [PATCH v2 7/8] selftests: Remove KSFT_TAP_LEVEL

[Kees Cook]

On Thu, Apr 25, 2019 at 9:36 AM shuah <shuah@kernel.org> wrote:
>
> On 4/24/19 5:12 PM, Kees Cook wrote:
> > Since sub-testing can now be detected by indentation level, this removes
> > KSFT_TAP_LEVEL so that subtests report their TAP header for later parsing.
>
> Does this take into ksft_print_header() getenv logic to avoid printing
> TAP headers from tests when they fork? e.g timers tests do that a lot.

I didn't change the ksft_print_header() code, in case you want it back
in the future. But nothing sets that variable any more in my series:

$ git grep KSFT_TAP_LEVEL
tools/testing/selftests/kselftest.h:    if (!(getenv("KSFT_TAP_LEVEL")))

I don't see the timers tests using print_header() at all, actually...

$ cd tools/testing/kselftest/timers
$ git grep print_header | wc -l
0

-- 
Kees Cook

Re: [PATCH v2 0/8] selftests: Move test output to diagnostic lines

[Kees Cook]

On Thu, Apr 25, 2019 at 9:52 AM shuah <shuah@kernel.org> wrote:
>
> On 4/24/19 5:12 PM,  wrote:
> > This refactors the selftest Makefiles to extract the test running logic
> > to be reused between "run_tests" and "emit_tests", while also fixing
> > up the test output to be TAP version 13 compliant:
> > - added "plan" line
> > - fixed result line syntax
> > - moved all test output to be "# "-prefixed as TAP "diagnostic" lines
> >
> > The prefixing code includes a fallback mode for limited execution
> > environments.
> >
> > Additionally, the plan lines are fixed for all callers of kselftest.h.
> >
> > -Kees
> >
>
> Kees,
>
> Just about to apply these to a topic branch to do testing and ran into
> checkpatch errors:
>
>
> WARNING: line over 80 characters - a few

I only saw one, which is on a string which kernel coding style says to
leave unsplit:

WARNING: line over 80 characters
#55: FILE: tools/testing/selftests/kselftest/runner.sh:19:
+               echo "$TEST_HDR_MSG: Warning: file $TEST is not
executable, correct this."

> WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
> #141: FILE: tools/testing/selftests/kselftest/runner.sh:2:

WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
#37: FILE: tools/testing/selftests/kselftest/runner.sh:2:
 # SPDX-License-Identifier: GPL-2.0

This is a shell script. It can't be on line 1:

$ head -n3 tools/testing/selftests/kselftest/runner.sh
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0
#

That looks like a bug in checkpatch not resetting the expected line or
something.

> Can fix them and resend - SPDX one is my main concern.

These appear to be false positives; I don't think I need to fix them?
Let me know what you think.

> The plan is to apply these to linux-kselftest ksft-tap-refactor topic
> first. I don't want to rush these until we do some testing.

Absolutely. :)

Thanks!

-- 
Kees Cook

Re: [PATCH v2 7/8] selftests: Remove KSFT_TAP_LEVEL

[shuah]

On 4/25/19 10:56 AM, Kees Cook wrote:
> On Thu, Apr 25, 2019 at 9:36 AM shuah <shuah@kernel.org> wrote:
>>
>> On 4/24/19 5:12 PM, Kees Cook wrote:
>>> Since sub-testing can now be detected by indentation level, this removes
>>> KSFT_TAP_LEVEL so that subtests report their TAP header for later parsing.
>>
>> Does this take into ksft_print_header() getenv logic to avoid printing
>> TAP headers from tests when they fork? e.g timers tests do that a lot.
> 
> I didn't change the ksft_print_header() code, in case you want it back
> in the future. But nothing sets that variable any more in my series:
> 

Right. I want to see the impact of not setting this. I added this for
two reasons, one is to prevent nesting which goes away with your
refactor. I know there is another reason which I can't recall.


> $ git grep KSFT_TAP_LEVEL
> tools/testing/selftests/kselftest.h:    if (!(getenv("KSFT_TAP_LEVEL")))
> 
> I don't see the timers tests using print_header() at all, actually...
> 
> $ cd tools/testing/kselftest/timers
> $ git grep print_header | wc -l
> 0
> 

I don't see it in there either. I must be thinking about another test.

btw I like the changes. I just want to make sure it gets some road
testing first. :)

thanks,
-- Shuah

Re: [PATCH v2 0/8] selftests: Move test output to diagnostic lines

[Kees Cook]

On Thu, Apr 25, 2019 at 10:05 AM Kees Cook <keescook@chromium.org> wrote:
> WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
> #37: FILE: tools/testing/selftests/kselftest/runner.sh:2:
>  # SPDX-License-Identifier: GPL-2.0
>
> This is a shell script. It can't be on line 1:
>
> $ head -n3 tools/testing/selftests/kselftest/runner.sh
> #!/bin/sh
> # SPDX-License-Identifier: GPL-2.0
> #
>
> That looks like a bug in checkpatch not resetting the expected line or
> something.

It doesn't like patch 3 and doesn't notice that diff offset starts at line 2:

diff --git a/tools/testing/selftests/kselftest/runner.sh
b/tools/testing/selftests/kselftest/runner.sh
index e1117d703887..f12b0a631273 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -2,17 +2,20 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 # Runs a set of tests in a given subdirectory.
+export KSFT_TAP_LEVEL=1

Joe, looks like the problem is here:

                if ($realline == $checklicenseline) {
realline == 2, checklicenseline == 1
so this is skipped, including the "#!/" check to move checklicenseline to 2.
then:
                if ($realline != $checklicenseline &&
                    $rawline =~ /\bSPDX-License-Identifier:/ &&
realline == 2, checklicenseline == 1
throws warning.

Seems like checklicenseline should be unconditionally set to 2 for
".sh" files? I don't see a way to fix this for just missing the #!/
line from a context diff, though...

-- 
Kees Cook

Re: [PATCH v2 0/8] selftests: Move test output to diagnostic lines

[shuah]

On 4/25/19 11:05 AM, Kees Cook wrote:
> On Thu, Apr 25, 2019 at 9:52 AM shuah <shuah@kernel.org> wrote:
>>
>> On 4/24/19 5:12 PM,  wrote:
>>> This refactors the selftest Makefiles to extract the test running logic
>>> to be reused between "run_tests" and "emit_tests", while also fixing
>>> up the test output to be TAP version 13 compliant:
>>> - added "plan" line
>>> - fixed result line syntax
>>> - moved all test output to be "# "-prefixed as TAP "diagnostic" lines
>>>
>>> The prefixing code includes a fallback mode for limited execution
>>> environments.
>>>
>>> Additionally, the plan lines are fixed for all callers of kselftest.h.
>>>
>>> -Kees
>>>
>>
>> Kees,
>>
>> Just about to apply these to a topic branch to do testing and ran into
>> checkpatch errors:
>>
>>
>> WARNING: line over 80 characters - a few
> 
> I only saw one, which is on a string which kernel coding style says to
> leave unsplit:
> 
> WARNING: line over 80 characters
> #55: FILE: tools/testing/selftests/kselftest/runner.sh:19:
> +               echo "$TEST_HDR_MSG: Warning: file $TEST is not
> executable, correct this."
> 
>> WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
>> #141: FILE: tools/testing/selftests/kselftest/runner.sh:2:
> 
> WARNING: Misplaced SPDX-License-Identifier tag - use line 1 instead
> #37: FILE: tools/testing/selftests/kselftest/runner.sh:2:
>   # SPDX-License-Identifier: GPL-2.0
> 
> This is a shell script. It can't be on line 1:
> 
> $ head -n3 tools/testing/selftests/kselftest/runner.sh
> #!/bin/sh
> # SPDX-License-Identifier: GPL-2.0
> #
> 
> That looks like a bug in checkpatch not resetting the expected line or
> something.
> 
>> Can fix them and resend - SPDX one is my main concern.
> 
> These appear to be false positives; I don't think I need to fix them?
> Let me know what you think.
> 
>> The plan is to apply these to linux-kselftest ksft-tap-refactor topic
>> first. I don't want to rush these until we do some testing.
> 
> Absolutely. :)
> 
> Thanks!
> 

Kees,

Pushed all 8 patches to 
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest 
ksft-tap-refactor topic branch

I will have time to test tomorrow.

thanks,
-- Shuah

Re: [PATCH v2 0/8] selftests: Move test output to diagnostic lines

[Kees Cook]

On Thu, Apr 25, 2019 at 1:39 PM shuah <shuah@kernel.org> wrote:
> Pushed all 8 patches to
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
> ksft-tap-refactor topic branch
>
> I will have time to test tomorrow.

Awesome! I'm excited. :)

Next I want to update kselftest_harness.h to do header/plan printing...

-- 
Kees Cook