All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] selftests: Move test output to diagnostic lines
@ 2019-04-09 23:55 ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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.

-Kees

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

 tools/testing/selftests/.gitignore          |  1 -
 tools/testing/selftests/Makefile            | 18 +++--
 tools/testing/selftests/kselftest/prefix.pl | 23 ++++++
 tools/testing/selftests/kselftest/runner.sh | 80 +++++++++++++++++++++
 tools/testing/selftests/lib.mk              | 61 +++-------------
 5 files changed, 119 insertions(+), 64 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl
 create mode 100644 tools/testing/selftests/kselftest/runner.sh

-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 0/6] selftests: Move test output to diagnostic lines
@ 2019-04-09 23:55 ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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.

-Kees

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

 tools/testing/selftests/.gitignore          |  1 -
 tools/testing/selftests/Makefile            | 18 +++--
 tools/testing/selftests/kselftest/prefix.pl | 23 ++++++
 tools/testing/selftests/kselftest/runner.sh | 80 +++++++++++++++++++++
 tools/testing/selftests/lib.mk              | 61 +++-------------
 5 files changed, 119 insertions(+), 64 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl
 create mode 100644 tools/testing/selftests/kselftest/runner.sh

-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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 removes the unused "summary" Makefile variable
(and tests). However, future merging with the "emit_tests" target needs
to be able to redirect output, so a new "logfile" variable is introduced.

Signed-off-by: Kees Cook <keescook at chromium.org>
---
 tools/testing/selftests/.gitignore          |  1 -
 tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
 tools/testing/selftests/lib.mk              | 33 ++-------------------
 3 files changed, 34 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..77d5510ac4c5
--- /dev/null
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+#
+# 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 8b0f16409ed7..7da79fe0bb78 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc
 ifeq (0,$(MAKELEVEL))
 OUTPUT := $(shell pwd)
 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
@@ -31,43 +32,15 @@ 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))						\
+		run_one "$$BASENAME_TEST" "$$test_num";	\
 	done;
 endef
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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 removes the unused "summary" Makefile variable
(and tests). However, future merging with the "emit_tests" target needs
to be able to redirect output, so a new "logfile" variable is introduced.

Signed-off-by: Kees Cook <keescook at chromium.org>
---
 tools/testing/selftests/.gitignore          |  1 -
 tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
 tools/testing/selftests/lib.mk              | 33 ++-------------------
 3 files changed, 34 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..77d5510ac4c5
--- /dev/null
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+#
+# 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 8b0f16409ed7..7da79fe0bb78 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc
 ifeq (0,$(MAKELEVEL))
 OUTPUT := $(shell pwd)
 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
@@ -31,43 +32,15 @@ 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))						\
+		run_one "$$BASENAME_TEST" "$$test_num";	\
 	done;
 endef
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 2/6] selftests: Use runner.sh for emit targets
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at 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 971fc8428117..45327e921169 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -131,7 +131,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; \
@@ -141,15 +142,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 7da79fe0bb78..8a4fad5d3934 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -78,24 +78,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 2/6] selftests: Use runner.sh for emit targets
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at 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 971fc8428117..45327e921169 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -131,7 +131,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; \
@@ -141,15 +142,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 7da79fe0bb78..8a4fad5d3934 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -78,24 +78,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 3/6] selftests: Extract logic for multiple test runs
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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.

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

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 45327e921169..72178d28f9c6 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -148,16 +148,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 77d5510ac4c5..51139f42a6ca 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -1,17 +1,19 @@
 #!/bin/sh
 #
 # Runs a set of tests in a given subdirectory.
+export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
 
 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
@@ -29,3 +31,14 @@ run_one()
 		cd - >/dev/null
 	fi
 }
+
+run_many()
+{
+	echo "TAP version 13"
+	DIR=$(basename "$PWD")
+	test_num=0
+	for TEST in "$@"; do
+		test_num=$(( test_num + 1 ))
+		run_one "$DIR" "$TEST" "$test_num"
+	done
+}
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 8a4fad5d3934..8be13d117101 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -33,15 +33,8 @@ 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`;	\
-		run_one "$$BASENAME_TEST" "$$test_num";	\
-	done;
+	run_many $(1)
 endef
 
 run_tests: all
@@ -79,12 +72,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 3/6] selftests: Extract logic for multiple test runs
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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.

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

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 45327e921169..72178d28f9c6 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -148,16 +148,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 77d5510ac4c5..51139f42a6ca 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -1,17 +1,19 @@
 #!/bin/sh
 #
 # Runs a set of tests in a given subdirectory.
+export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
 
 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
@@ -29,3 +31,14 @@ run_one()
 		cd - >/dev/null
 	fi
 }
+
+run_many()
+{
+	echo "TAP version 13"
+	DIR=$(basename "$PWD")
+	test_num=0
+	for TEST in "$@"; do
+		test_num=$(( test_num + 1 ))
+		run_one "$DIR" "$TEST" "$test_num"
+	done
+}
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 8a4fad5d3934..8be13d117101 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -33,15 +33,8 @@ 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`;	\
-		run_one "$$BASENAME_TEST" "$$test_num";	\
-	done;
+	run_many $(1)
 endef
 
 run_tests: all
@@ -79,12 +72,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 4/6] selftests/runner: Add plan line and fix result line syntax
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at chromium.org>
---
 tools/testing/selftests/kselftest/runner.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 51139f42a6ca..7f8d6b127693 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -18,15 +18,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
@@ -37,6 +37,8 @@ run_many()
 	echo "TAP version 13"
 	DIR=$(basename "$PWD")
 	test_num=0
+	total=$(echo "$@" | wc -w)
+	echo "1..$total"
 	for TEST in "$@"; do
 		test_num=$(( test_num + 1 ))
 		run_one "$DIR" "$TEST" "$test_num"
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 4/6] selftests/runner: Add plan line and fix result line syntax
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at chromium.org>
---
 tools/testing/selftests/kselftest/runner.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 51139f42a6ca..7f8d6b127693 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -18,15 +18,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
@@ -37,6 +37,8 @@ run_many()
 	echo "TAP version 13"
 	DIR=$(basename "$PWD")
 	test_num=0
+	total=$(echo "$@" | wc -w)
+	echo "1..$total"
 	for TEST in "$@"; do
 		test_num=$(( test_num + 1 ))
 		run_one "$DIR" "$TEST" "$test_num"
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 5/6] selftests/runner: Distinguish between missing and non-executable
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at 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 7f8d6b127693..5793660dbe3f 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -17,7 +17,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 5/6] selftests/runner: Distinguish between missing and non-executable
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at 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 7f8d6b127693..5793660dbe3f 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -17,7 +17,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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 6/6] selftests: Move test output to diagnostic lines
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at chromium.org>
---
 tools/testing/selftests/Makefile            |  1 +
 tools/testing/selftests/kselftest/prefix.pl | 23 +++++++++++++
 tools/testing/selftests/kselftest/runner.sh | 37 ++++++++++++++++++---
 tools/testing/selftests/lib.mk              |  1 +
 4 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 72178d28f9c6..2e92a9727a4d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -133,6 +133,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/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 5793660dbe3f..f8b545a6e0e9 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -5,6 +5,34 @@ export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
 
+# 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"
@@ -14,10 +42,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
@@ -26,7 +53,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 8be13d117101..5c6505bec8be 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -33,6 +33,7 @@ endif
 
 .ONESHELL:
 define RUN_TESTS
+	BASE_DIR="$(selfdir)";				\
 	. $(selfdir)/kselftest/runner.sh;		\
 	run_many $(1)
 endef
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 6/6] selftests: Move test output to diagnostic lines
@ 2019-04-09 23:55   ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-09 23:55 UTC (permalink / raw)


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 at chromium.org>
---
 tools/testing/selftests/Makefile            |  1 +
 tools/testing/selftests/kselftest/prefix.pl | 23 +++++++++++++
 tools/testing/selftests/kselftest/runner.sh | 37 ++++++++++++++++++---
 tools/testing/selftests/lib.mk              |  1 +
 4 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100755 tools/testing/selftests/kselftest/prefix.pl

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 72178d28f9c6..2e92a9727a4d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -133,6 +133,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/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 5793660dbe3f..f8b545a6e0e9 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -5,6 +5,34 @@ export KSFT_TAP_LEVEL=1
 export skip_rc=4
 export logfile=/dev/stdout
 
+# 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"
@@ -14,10 +42,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
@@ -26,7 +53,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 8be13d117101..5c6505bec8be 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -33,6 +33,7 @@ endif
 
 .ONESHELL:
 define RUN_TESTS
+	BASE_DIR="$(selfdir)";				\
 	. $(selfdir)/kselftest/runner.sh;		\
 	run_many $(1)
 endef
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:11     ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-16 23:11 UTC (permalink / raw)


Hi Kees,

Thanks for the patch.

On 4/9/19 5:55 PM, Kees Cook wrote:
> 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 removes the unused "summary" Makefile variable
> (and tests). However, future merging with the "emit_tests" target needs
> to be able to redirect output, so a new "logfile" variable is introduced.

Shouldn't the selftests/Makefile need update for "summary" removal??

> 
> Signed-off-by: Kees Cook <keescook at chromium.org>
> ---
>   tools/testing/selftests/.gitignore          |  1 -
>   tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
>   tools/testing/selftests/lib.mk              | 33 ++-------------------
>   3 files changed, 34 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/

Please don't include this .gitignore change here. These are generated
in tools/gpio and this .gitignore isn't the right place for them.

> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> new file mode 100644
> index 000000000000..77d5510ac4c5
> --- /dev/null
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +#
> +# 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 8b0f16409ed7..7da79fe0bb78 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc
>   ifeq (0,$(MAKELEVEL))
>   OUTPUT := $(shell pwd)
>   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
> @@ -31,43 +32,15 @@ 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))						\
> +		run_one "$$BASENAME_TEST" "$$test_num";	\
>   	done;
>   endef
>   
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:11     ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-16 23:11 UTC (permalink / raw)


Hi Kees,

Thanks for the patch.

On 4/9/19 5:55 PM, Kees Cook wrote:
> 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 removes the unused "summary" Makefile variable
> (and tests). However, future merging with the "emit_tests" target needs
> to be able to redirect output, so a new "logfile" variable is introduced.

Shouldn't the selftests/Makefile need update for "summary" removal??

> 
> Signed-off-by: Kees Cook <keescook at chromium.org>
> ---
>   tools/testing/selftests/.gitignore          |  1 -
>   tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
>   tools/testing/selftests/lib.mk              | 33 ++-------------------
>   3 files changed, 34 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/

Please don't include this .gitignore change here. These are generated
in tools/gpio and this .gitignore isn't the right place for them.

> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> new file mode 100644
> index 000000000000..77d5510ac4c5
> --- /dev/null
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +#
> +# 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 8b0f16409ed7..7da79fe0bb78 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc
>   ifeq (0,$(MAKELEVEL))
>   OUTPUT := $(shell pwd)
>   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
> @@ -31,43 +32,15 @@ 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))						\
> +		run_one "$$BASENAME_TEST" "$$test_num";	\
>   	done;
>   endef
>   
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:16       ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-16 23:16 UTC (permalink / raw)


On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah at kernel.org> wrote:
>
> Hi Kees,
>
> Thanks for the patch.
>
> On 4/9/19 5:55 PM, Kees Cook wrote:
> > 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 removes the unused "summary" Makefile variable
> > (and tests). However, future merging with the "emit_tests" target needs
> > to be able to redirect output, so a new "logfile" variable is introduced.
>
> Shouldn't the selftests/Makefile need update for "summary" removal??
>

I didn't see anything using "summary" except as a --summary argument
to the run_kselftests.sh script. Maybe I missed it?

> >
> > Signed-off-by: Kees Cook <keescook at chromium.org>
> > ---
> >   tools/testing/selftests/.gitignore          |  1 -
> >   tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
> >   tools/testing/selftests/lib.mk              | 33 ++-------------------
> >   3 files changed, 34 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/
>
> Please don't include this .gitignore change here. These are generated
> in tools/gpio and this .gitignore isn't the right place for them.

This change is only removing the "kselftest" entry, for which the
target is long gone. Since I was adding a directory by that name, I
needed to remove it from the .gitignore file. I have nothing to do
with the gpio stuff. :)

Thanks for looking this over!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:16       ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-16 23:16 UTC (permalink / raw)


On Tue, Apr 16, 2019@6:11 PM shuah <shuah@kernel.org> wrote:
>
> Hi Kees,
>
> Thanks for the patch.
>
> On 4/9/19 5:55 PM, Kees Cook wrote:
> > 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 removes the unused "summary" Makefile variable
> > (and tests). However, future merging with the "emit_tests" target needs
> > to be able to redirect output, so a new "logfile" variable is introduced.
>
> Shouldn't the selftests/Makefile need update for "summary" removal??
>

I didn't see anything using "summary" except as a --summary argument
to the run_kselftests.sh script. Maybe I missed it?

> >
> > Signed-off-by: Kees Cook <keescook at chromium.org>
> > ---
> >   tools/testing/selftests/.gitignore          |  1 -
> >   tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
> >   tools/testing/selftests/lib.mk              | 33 ++-------------------
> >   3 files changed, 34 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/
>
> Please don't include this .gitignore change here. These are generated
> in tools/gpio and this .gitignore isn't the right place for them.

This change is only removing the "kselftest" entry, for which the
target is long gone. Since I was adding a directory by that name, I
needed to remove it from the .gitignore file. I have nothing to do
with the gpio stuff. :)

Thanks for looking this over!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:21         ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-16 23:21 UTC (permalink / raw)


On 4/16/19 5:16 PM, Kees Cook wrote:
> On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah at kernel.org> wrote:
>>
>> Hi Kees,
>>
>> Thanks for the patch.
>>
>> On 4/9/19 5:55 PM, Kees Cook wrote:
>>> 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 removes the unused "summary" Makefile variable
>>> (and tests). However, future merging with the "emit_tests" target needs
>>> to be able to redirect output, so a new "logfile" variable is introduced.
>>
>> Shouldn't the selftests/Makefile need update for "summary" removal??
>>
> 
> I didn't see anything using "summary" except as a --summary argument
> to the run_kselftests.sh script. Maybe I missed it?
> 

It is in the selftests/Makefile install target.

>>>
>>> Signed-off-by: Kees Cook <keescook at chromium.org>
>>> ---
>>>    tools/testing/selftests/.gitignore          |  1 -
>>>    tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
>>>    tools/testing/selftests/lib.mk              | 33 ++-------------------
>>>    3 files changed, 34 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/
>>
>> Please don't include this .gitignore change here. These are generated
>> in tools/gpio and this .gitignore isn't the right place for them.
> 
> This change is only removing the "kselftest" entry, for which the
> target is long gone. Since I was adding a directory by that name, I
> needed to remove it from the .gitignore file. I have nothing to do
> with the gpio stuff. :)
> 

Yeah my bad! :)

> Thanks for looking this over!
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-16 23:21         ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-16 23:21 UTC (permalink / raw)


On 4/16/19 5:16 PM, Kees Cook wrote:
> On Tue, Apr 16, 2019@6:11 PM shuah <shuah@kernel.org> wrote:
>>
>> Hi Kees,
>>
>> Thanks for the patch.
>>
>> On 4/9/19 5:55 PM, Kees Cook wrote:
>>> 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 removes the unused "summary" Makefile variable
>>> (and tests). However, future merging with the "emit_tests" target needs
>>> to be able to redirect output, so a new "logfile" variable is introduced.
>>
>> Shouldn't the selftests/Makefile need update for "summary" removal??
>>
> 
> I didn't see anything using "summary" except as a --summary argument
> to the run_kselftests.sh script. Maybe I missed it?
> 

It is in the selftests/Makefile install target.

>>>
>>> Signed-off-by: Kees Cook <keescook at chromium.org>
>>> ---
>>>    tools/testing/selftests/.gitignore          |  1 -
>>>    tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++
>>>    tools/testing/selftests/lib.mk              | 33 ++-------------------
>>>    3 files changed, 34 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/
>>
>> Please don't include this .gitignore change here. These are generated
>> in tools/gpio and this .gitignore isn't the right place for them.
> 
> This change is only removing the "kselftest" entry, for which the
> target is long gone. Since I was adding a directory by that name, I
> needed to remove it from the .gitignore file. I have nothing to do
> with the gpio stuff. :)
> 

Yeah my bad! :)

> Thanks for looking this over!
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-23 22:31           ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-23 22:31 UTC (permalink / raw)


On Tue, Apr 16, 2019 at 4:21 PM shuah <shuah at kernel.org> wrote:
>
> On 4/16/19 5:16 PM, Kees Cook wrote:
> > On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah at kernel.org> wrote:
> >>
> >> Hi Kees,
> >>
> >> Thanks for the patch.
> >>
> >> On 4/9/19 5:55 PM, Kees Cook wrote:
> >>> 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 removes the unused "summary" Makefile variable
> >>> (and tests). However, future merging with the "emit_tests" target needs
> >>> to be able to redirect output, so a new "logfile" variable is introduced.
> >>
> >> Shouldn't the selftests/Makefile need update for "summary" removal??
> >>
> >
> > I didn't see anything using "summary" except as a --summary argument
> > to the run_kselftests.sh script. Maybe I missed it?
> >
>
> It is in the selftests/Makefile install target.

Right: it's used only by the run_kselftest.sh script:

ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh

install:
...
        echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT)


So, I think this entire series can land. Is there other feedback I
should incorporate? I'd like to see it get some -next testing...

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-23 22:31           ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-23 22:31 UTC (permalink / raw)


On Tue, Apr 16, 2019@4:21 PM shuah <shuah@kernel.org> wrote:
>
> On 4/16/19 5:16 PM, Kees Cook wrote:
> > On Tue, Apr 16, 2019@6:11 PM shuah <shuah@kernel.org> wrote:
> >>
> >> Hi Kees,
> >>
> >> Thanks for the patch.
> >>
> >> On 4/9/19 5:55 PM, Kees Cook wrote:
> >>> 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 removes the unused "summary" Makefile variable
> >>> (and tests). However, future merging with the "emit_tests" target needs
> >>> to be able to redirect output, so a new "logfile" variable is introduced.
> >>
> >> Shouldn't the selftests/Makefile need update for "summary" removal??
> >>
> >
> > I didn't see anything using "summary" except as a --summary argument
> > to the run_kselftests.sh script. Maybe I missed it?
> >
>
> It is in the selftests/Makefile install target.

Right: it's used only by the run_kselftest.sh script:

ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh

install:
...
        echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT)


So, I think this entire series can land. Is there other feedback I
should incorporate? I'd like to see it get some -next testing...

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-23 22:47             ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-23 22:47 UTC (permalink / raw)


On 4/23/19 4:31 PM, Kees Cook wrote:
> On Tue, Apr 16, 2019 at 4:21 PM shuah <shuah at kernel.org> wrote:
>>
>> On 4/16/19 5:16 PM, Kees Cook wrote:
>>> On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah at kernel.org> wrote:
>>>>
>>>> Hi Kees,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> On 4/9/19 5:55 PM, Kees Cook wrote:
>>>>> 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 removes the unused "summary" Makefile variable
>>>>> (and tests). However, future merging with the "emit_tests" target needs
>>>>> to be able to redirect output, so a new "logfile" variable is introduced.
>>>>
>>>> Shouldn't the selftests/Makefile need update for "summary" removal??
>>>>
>>>
>>> I didn't see anything using "summary" except as a --summary argument
>>> to the run_kselftests.sh script. Maybe I missed it?
>>>
>>
>> It is in the selftests/Makefile install target.
> 
> Right: it's used only by the run_kselftest.sh script:
> 
> ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh
> 
> install:
> ...
>          echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT)
> 
> 
> So, I think this entire series can land. Is there other feedback I
> should incorporate? I'd like to see it get some -next testing...
> 
> Thanks!
> 


Kees,

Yes I was planning to get this into next and did some testing. I found
that the first patch breaks the summary option.

You can see it by running

make summary=1 TARGETS="breakpoints" kselftest

with and without your first patch. Ca you fix the regression and send
me revised patches. Sorry, I didn't get back to you sooner. A few bugs
I found kept me busy.

While I was testing your patch series, I found test build failures due
to circular dependency that needed fixing. In addition, the same
headers_install dependency, also broke O= and KBUILD_OUTPUT builds
and fixed those as well. Long story short, these fixes might conflict
with your work.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-23 22:47             ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-23 22:47 UTC (permalink / raw)


On 4/23/19 4:31 PM, Kees Cook wrote:
> On Tue, Apr 16, 2019@4:21 PM shuah <shuah@kernel.org> wrote:
>>
>> On 4/16/19 5:16 PM, Kees Cook wrote:
>>> On Tue, Apr 16, 2019@6:11 PM shuah <shuah@kernel.org> wrote:
>>>>
>>>> Hi Kees,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> On 4/9/19 5:55 PM, Kees Cook wrote:
>>>>> 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 removes the unused "summary" Makefile variable
>>>>> (and tests). However, future merging with the "emit_tests" target needs
>>>>> to be able to redirect output, so a new "logfile" variable is introduced.
>>>>
>>>> Shouldn't the selftests/Makefile need update for "summary" removal??
>>>>
>>>
>>> I didn't see anything using "summary" except as a --summary argument
>>> to the run_kselftests.sh script. Maybe I missed it?
>>>
>>
>> It is in the selftests/Makefile install target.
> 
> Right: it's used only by the run_kselftest.sh script:
> 
> ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh
> 
> install:
> ...
>          echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT)
> 
> 
> So, I think this entire series can land. Is there other feedback I
> should incorporate? I'd like to see it get some -next testing...
> 
> Thanks!
> 


Kees,

Yes I was planning to get this into next and did some testing. I found
that the first patch breaks the summary option.

You can see it by running

make summary=1 TARGETS="breakpoints" kselftest

with and without your first patch. Ca you fix the regression and send
me revised patches. Sorry, I didn't get back to you sooner. A few bugs
I found kept me busy.

While I was testing your patch series, I found test build failures due
to circular dependency that needed fixing. In addition, the same
headers_install dependency, also broke O= and KBUILD_OUTPUT builds
and fixed those as well. Long story short, these fixes might conflict
with your work.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-24  2:43               ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: keescook @ 2019-04-24  2:43 UTC (permalink / raw)


On Tue, Apr 23, 2019 at 3:47 PM shuah <shuah at kernel.org> wrote:
> You can see it by running
>
> make summary=1 TARGETS="breakpoints" kselftest

Okay, thanks for this! I'll give it a spin.

> with and without your first patch. Ca you fix the regression and send
> me revised patches. Sorry, I didn't get back to you sooner. A few bugs
> I found kept me busy.
>
> While I was testing your patch series, I found test build failures due
> to circular dependency that needed fixing. In addition, the same
> headers_install dependency, also broke O= and KBUILD_OUTPUT builds
> and fixed those as well. Long story short, these fixes might conflict
> with your work.

I can rebase the series -- which tree should I look at? (Is it all in
-next already?)

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-24  2:43               ` Kees Cook
  0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2019-04-24  2:43 UTC (permalink / raw)


On Tue, Apr 23, 2019@3:47 PM shuah <shuah@kernel.org> wrote:
> You can see it by running
>
> make summary=1 TARGETS="breakpoints" kselftest

Okay, thanks for this! I'll give it a spin.

> with and without your first patch. Ca you fix the regression and send
> me revised patches. Sorry, I didn't get back to you sooner. A few bugs
> I found kept me busy.
>
> While I was testing your patch series, I found test build failures due
> to circular dependency that needed fixing. In addition, the same
> headers_install dependency, also broke O= and KBUILD_OUTPUT builds
> and fixed those as well. Long story short, these fixes might conflict
> with your work.

I can rebase the series -- which tree should I look at? (Is it all in
-next already?)

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-24  2:46                 ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-24  2:46 UTC (permalink / raw)


On 4/23/19 8:43 PM, Kees Cook wrote:
> On Tue, Apr 23, 2019 at 3:47 PM shuah <shuah at kernel.org> wrote:
>> You can see it by running
>>
>> make summary=1 TARGETS="breakpoints" kselftest
> 
> Okay, thanks for this! I'll give it a spin.
> 
>> with and without your first patch. Ca you fix the regression and send
>> me revised patches. Sorry, I didn't get back to you sooner. A few bugs
>> I found kept me busy.
>>
>> While I was testing your patch series, I found test build failures due
>> to circular dependency that needed fixing. In addition, the same
>> headers_install dependency, also broke O= and KBUILD_OUTPUT builds
>> and fixed those as well. Long story short, these fixes might conflict
>> with your work.
> 
> I can rebase the series -- which tree should I look at? (Is it all in
> -next already?)
> 

Yes - the two patches are in linux-kselftest next

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk
@ 2019-04-24  2:46                 ` shuah
  0 siblings, 0 replies; 28+ messages in thread
From: shuah @ 2019-04-24  2:46 UTC (permalink / raw)


On 4/23/19 8:43 PM, Kees Cook wrote:
> On Tue, Apr 23, 2019@3:47 PM shuah <shuah@kernel.org> wrote:
>> You can see it by running
>>
>> make summary=1 TARGETS="breakpoints" kselftest
> 
> Okay, thanks for this! I'll give it a spin.
> 
>> with and without your first patch. Ca you fix the regression and send
>> me revised patches. Sorry, I didn't get back to you sooner. A few bugs
>> I found kept me busy.
>>
>> While I was testing your patch series, I found test build failures due
>> to circular dependency that needed fixing. In addition, the same
>> headers_install dependency, also broke O= and KBUILD_OUTPUT builds
>> and fixed those as well. Long story short, these fixes might conflict
>> with your work.
> 
> I can rebase the series -- which tree should I look at? (Is it all in
> -next already?)
> 

Yes - the two patches are in linux-kselftest next

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 28+ messages in thread
end of thread, other threads:[~2019-04-24  2:46 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-09 23:55 [PATCH 0/6] selftests: Move test output to diagnostic lines keescook
2019-04-09 23:55 ` Kees Cook
2019-04-09 23:55 ` [PATCH 1/6] selftests: Extract single-test shell logic from lib.mk keescook
2019-04-09 23:55   ` Kees Cook
2019-04-16 23:11   ` shuah
2019-04-16 23:11     ` shuah
2019-04-16 23:16     ` keescook
2019-04-16 23:16       ` Kees Cook
2019-04-16 23:21       ` shuah
2019-04-16 23:21         ` shuah
2019-04-23 22:31         ` keescook
2019-04-23 22:31           ` Kees Cook
2019-04-23 22:47           ` shuah
2019-04-23 22:47             ` shuah
2019-04-24  2:43             ` keescook
2019-04-24  2:43               ` Kees Cook
2019-04-24  2:46               ` shuah
2019-04-24  2:46                 ` shuah
2019-04-09 23:55 ` [PATCH 2/6] selftests: Use runner.sh for emit targets keescook
2019-04-09 23:55   ` Kees Cook
2019-04-09 23:55 ` [PATCH 3/6] selftests: Extract logic for multiple test runs keescook
2019-04-09 23:55   ` Kees Cook
2019-04-09 23:55 ` [PATCH 4/6] selftests/runner: Add plan line and fix result line syntax keescook
2019-04-09 23:55   ` Kees Cook
2019-04-09 23:55 ` [PATCH 5/6] selftests/runner: Distinguish between missing and non-executable keescook
2019-04-09 23:55   ` Kees Cook
2019-04-09 23:55 ` [PATCH 6/6] selftests: Move test output to diagnostic lines keescook
2019-04-09 23:55   ` Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.