[PATCH nft v3 00/11] tests/shell: allow running tests as

[PATCH nft v3 00/11] tests/shell: allow running tests as

[Thomas Haller]

Changes to v3:

- large rework of all patches.
- we still try to unshare as much as we can, but gracefully fallback to
  only unshare the netns. What we don't do anymore, is accept failure to unshare
  altogether and proceed silently. If you want that, use NFT_TEST_NO_UNSHARE=y or
  NFT_TEST_UNSHARE_CMD=cmd.
- compared to v2, fix `nft flush` to be called inside the target netns.
  It's now done by "test-wrapper.sh"
- add mode to run jobs in parallel.
- move test-specific functionality from "run-tests.sh to "test-wrapper.sh".
- collect test results in a temporary directory for later inspection.

Changes to v2:

- new patch: rework the parsing of command line options
- new patch: add a "--list-tests" option to show the found tests
- call "unshare" for each test individually.
- drop NFT_TEST_ROOTLESS environment variable. You no longer have to
  opt-in to run rootless. However, if any tests fail and we ran
  rootless, then an info is printed at the end.
- the environment variables NFT_TEST_HAVE_REALROOT and
  NFT_TEST_NO_UNSHARE can still be set to configure the script.
  Those are now also configurable via command line options.
  Usually you would not have to set them.

Thomas Haller (11):
  tests/shell: rework command line parsing in "run-tests.sh"
  tests/shell: rework finding tests and add "--list-tests" option
  tests/shell: check test names before start and support directories
  tests/shell: export NFT_TEST_BASEDIR and NFT_TEST_TMPDIR for tests
  tests/shell: run each test in separate namespace and allow rootless
  tests/shell: interpret an exit code of 77 from scripts as "skipped"
  tests/shell: support --keep-logs option (NFT_TEST_KEEP_LOGS=y) to
    preserve test output
  tests/shell: move the dump diff handling inside "test-wrapper.sh"
  tests/shell: rework printing of test results
  tests/shell: move taint check to "test-wrapper.sh"
  tests/shell: support running tests in parallel

 tests/shell/helpers/test-wrapper.sh |  77 +++++
 tests/shell/run-tests.sh            | 467 ++++++++++++++++++++--------
 2 files changed, 422 insertions(+), 122 deletions(-)
 create mode 100755 tests/shell/helpers/test-wrapper.sh

-- 
2.41.0

[PATCH nft v3 01/11] tests/shell: rework command line parsing in "run-tests.sh"

[Thomas Haller]

Parse the arguments in a loop, so that their order does not matter.
Also, soon more command line arguments will be added, and this way of
parsing seems more maintainable and flexible.

Currently this is still after the is-root check and after unshare. That
will be addressed later.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 95 +++++++++++++++++++++++++++-------------
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index b66ef4fa4d1f..ae8c6d934dcf 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -1,10 +1,5 @@
 #!/bin/bash
 
-# Configuration
-TESTDIR="./$(dirname $0)/testcases"
-SRC_NFT="$(dirname $0)/../../src/nft"
-DIFF=$(which diff)
-
 msg_error() {
 	echo "E: $1 ..." >&2
 	exit 1
@@ -18,6 +13,29 @@ msg_info() {
 	echo "I: $1"
 }
 
+usage() {
+	echo " $0 [OPTIONS]"
+	echo
+	echo "OPTIONS:"
+	echo " -h|--help : print usage"
+	echo " -v        : sets VERBOSE=y"
+	echo " -g        : sets DUMPGEN=y"
+	echo " -V        : sets VALGRIND=y"
+	echo " -K        : sets KMEMLEAK=y"
+	echo
+	echo "ENVIRONMENT VARIABLES:"
+	echo " NFT=<PATH>   : Path to nft executable"
+	echo " VERBOSE=*|y  : Enable verbose output"
+	echo " DUMPGEN=*|y  : Regenerate dump files"
+	echo " VALGRIND=*|y : Run \$NFT in valgrind"
+	echo " KMEMLEAK=*|y : Check for kernel memleaks"
+}
+
+# Configuration
+TESTDIR="./$(dirname $0)/testcases"
+SRC_NFT="$(dirname $0)/../../src/nft"
+DIFF=$(which diff)
+
 if [ "$(id -u)" != "0" ] ; then
 	msg_error "this requires root!"
 fi
@@ -31,6 +49,48 @@ if [ "${1}" != "run" ]; then
 fi
 shift
 
+VERBOSE="$VERBOSE"
+DUMPGEN="$DUMPGEN"
+VALGRIND="$VALGRIND"
+KMEMLEAK="$KMEMLEAK"
+
+TESTS=()
+
+while [ $# -gt 0 ] ; do
+	A="$1"
+	shift
+	case "$A" in
+		-v)
+			VERBOSE=y
+			;;
+		-g)
+			DUMPGEN=y
+			;;
+		-V)
+			VALGRIND=y
+			;;
+		-K)
+			KMEMLEAK=y
+			;;
+		-h|--help)
+			usage
+			exit 0
+			;;
+		--)
+			TESTS+=( "$@" )
+			shift $#
+			;;
+		*)
+			# Any unrecognized option is treated as a test name, and also
+			# enable verbose tests.
+			TESTS+=( "$A" )
+			VERBOSE=y
+			;;
+	esac
+done
+
+SINGLE="${TESTS[*]}"
+
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
 ret=$?
@@ -59,31 +119,6 @@ if [ ! -x "$DIFF" ] ; then
 	DIFF=true
 fi
 
-if [ "$1" == "-v" ] ; then
-	VERBOSE=y
-	shift
-fi
-
-if [ "$1" == "-g" ] ; then
-	DUMPGEN=y
-	shift
-fi
-
-if [ "$1" == "-V" ] ; then
-	VALGRIND=y
-	shift
-fi
-
-if [ "$1" == "-K" ]; then
-	KMEMLEAK=y
-	shift
-fi
-
-for arg in "$@"; do
-	SINGLE+=" $arg"
-	VERBOSE=y
-done
-
 kernel_cleanup() {
 	$NFT flush ruleset
 	$MODPROBE -raq \
-- 
2.41.0

[PATCH nft v3 02/11] tests/shell: rework finding tests and add "--list-tests" option

[Thomas Haller]

Cleanup finding the test files. Also add a "--list-tests" option to see
which tests are found and would run.

Also get rid of the FIND="$(which find)" detection. Which system doesn't
have a working find? Also, we can just fail when we try to use find, and
don't need a check first.

This is still  after "unshare", which will be addressed next.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 53 ++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index ae8c6d934dcf..184dd3f38be5 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -17,11 +17,12 @@ usage() {
 	echo " $0 [OPTIONS]"
 	echo
 	echo "OPTIONS:"
-	echo " -h|--help : print usage"
-	echo " -v        : sets VERBOSE=y"
-	echo " -g        : sets DUMPGEN=y"
-	echo " -V        : sets VALGRIND=y"
-	echo " -K        : sets KMEMLEAK=y"
+	echo " -h|--help       : print usage"
+	echo " -L|--list-tests : list test names and quit"
+	echo " -v              : sets VERBOSE=y"
+	echo " -g              : sets DUMPGEN=y"
+	echo " -V              : sets VALGRIND=y"
+	echo " -K              : sets KMEMLEAK=y"
 	echo
 	echo "ENVIRONMENT VARIABLES:"
 	echo " NFT=<PATH>   : Path to nft executable"
@@ -32,8 +33,8 @@ usage() {
 }
 
 # Configuration
-TESTDIR="./$(dirname $0)/testcases"
-SRC_NFT="$(dirname $0)/../../src/nft"
+BASEDIR="$(dirname "$0")"
+SRC_NFT="$BASEDIR/../../src/nft"
 DIFF=$(which diff)
 
 if [ "$(id -u)" != "0" ] ; then
@@ -53,6 +54,7 @@ VERBOSE="$VERBOSE"
 DUMPGEN="$DUMPGEN"
 VALGRIND="$VALGRIND"
 KMEMLEAK="$KMEMLEAK"
+DO_LIST_TESTS=
 
 TESTS=()
 
@@ -76,6 +78,9 @@ while [ $# -gt 0 ] ; do
 			usage
 			exit 0
 			;;
+		-L|--list-tests)
+			DO_LIST_TESTS=y
+			;;
 		--)
 			TESTS+=( "$@" )
 			shift $#
@@ -89,7 +94,19 @@ while [ $# -gt 0 ] ; do
 	esac
 done
 
-SINGLE="${TESTS[*]}"
+find_tests() {
+	find "$1" -type f -executable | sort
+}
+
+if [ "${#TESTS[@]}" -eq 0 ] ; then
+	TESTS=( $(find_tests "$BASEDIR/testcases/") )
+	test "${#TESTS[@]}" -gt 0 || msg_error "Could not find tests"
+fi
+
+if [ "$DO_LIST_TESTS" = y ] ; then
+	printf '%s\n' "${TESTS[@]}"
+	exit 0
+fi
 
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
@@ -100,15 +117,6 @@ else
 	msg_info "using nft command: ${NFT}"
 fi
 
-if [ ! -d "$TESTDIR" ] ; then
-	msg_error "missing testdir $TESTDIR"
-fi
-
-FIND="$(which find)"
-if [ ! -x "$FIND" ] ; then
-	msg_error "no find binary found"
-fi
-
 MODPROBE="$(which modprobe)"
 if [ ! -x "$MODPROBE" ] ; then
 	msg_error "no modprobe binary found"
@@ -143,14 +151,6 @@ kernel_cleanup() {
 	nft_xfrm
 }
 
-find_tests() {
-	if [ ! -z "$SINGLE" ] ; then
-		echo $SINGLE
-		return
-	fi
-	${FIND} ${TESTDIR} -type f -executable | sort
-}
-
 printscript() { # (cmd, tmpd)
 	cat <<EOF
 #!/bin/bash
@@ -248,8 +248,7 @@ check_kmemleak()
 
 check_taint
 
-for testfile in $(find_tests)
-do
+for testfile in "${TESTS[@]}" ; do
 	read taint < /proc/sys/kernel/tainted
 	kernel_cleanup
 
-- 
2.41.0

[PATCH nft v3 03/11] tests/shell: check test names before start and support directories

[Thomas Haller]

Check for valid test names early. That's useful because we treat any
unrecognized options as test names. We should detect a mistake early.

While at it, also support specifying directory names instead of
executable files.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 184dd3f38be5..0a2598f10bed 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -103,6 +103,18 @@ if [ "${#TESTS[@]}" -eq 0 ] ; then
 	test "${#TESTS[@]}" -gt 0 || msg_error "Could not find tests"
 fi
 
+TESTSOLD=( "${TESTS[@]}" )
+TESTS=()
+for t in "${TESTSOLD[@]}" ; do
+	if [ -f "$t" -a -x "$t" ] ; then
+		TESTS+=( "$t" )
+	elif [ -d "$t" ] ; then
+		TESTS+=( $(find_tests "$t") )
+	else
+		msg_error "Unknown test \"$t\""
+	fi
+done
+
 if [ "$DO_LIST_TESTS" = y ] ; then
 	printf '%s\n' "${TESTS[@]}"
 	exit 0
-- 
2.41.0

[PATCH nft v3 04/11] tests/shell: export NFT_TEST_BASEDIR and NFT_TEST_TMPDIR for tests

[Thomas Haller]

Let the test wrapper prepare and export two environment variables for
the test:

- "$NFT_TEST_BASEDIR" is just the top directory where the test scripts
  lie.

- "$NFT_TEST_TMPDIR" is a `mktemp` directory created by "run-tests.sh"
  and removed at the end. Tests may use that to leave data there.
  This directory will be used for various things, like the "nft" wrapper
  in valgrind mode, the results of the tests and possibly as cache for
  feature detection.

The "$NFT_TEST_TMPDIR" was already used before with the "VALGRIND=y"
mode. It's only renamed and got an extended purpose.

Also drop the unnecessary first detection of "$DIFF" and the "$SRC_NFT"
variable.

Also, note that the mktemp creates the temporary directory under /tmp.
Which is commonly a tempfs. The user can override that by exporting
TMPDIR.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 43 +++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 0a2598f10bed..22a34f1f7898 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -32,10 +32,10 @@ usage() {
 	echo " KMEMLEAK=*|y : Check for kernel memleaks"
 }
 
-# Configuration
-BASEDIR="$(dirname "$0")"
-SRC_NFT="$BASEDIR/../../src/nft"
-DIFF=$(which diff)
+NFT_TEST_BASEDIR="$(dirname "$0")"
+
+# Export the base directory. It may be used by tests.
+export NFT_TEST_BASEDIR
 
 if [ "$(id -u)" != "0" ] ; then
 	msg_error "this requires root!"
@@ -99,7 +99,7 @@ find_tests() {
 }
 
 if [ "${#TESTS[@]}" -eq 0 ] ; then
-	TESTS=( $(find_tests "$BASEDIR/testcases/") )
+	TESTS=( $(find_tests "$NFT_TEST_BASEDIR/testcases/") )
 	test "${#TESTS[@]}" -gt 0 || msg_error "Could not find tests"
 fi
 
@@ -120,7 +120,7 @@ if [ "$DO_LIST_TESTS" = y ] ; then
 	exit 0
 fi
 
-[ -z "$NFT" ] && NFT=$SRC_NFT
+[ -z "$NFT" ] && NFT="$NFT_TEST_BASEDIR/../../src/nft"
 ${NFT} > /dev/null 2>&1
 ret=$?
 if [ ${ret} -eq 126 ] || [ ${ret} -eq 127 ]; then
@@ -139,6 +139,23 @@ if [ ! -x "$DIFF" ] ; then
 	DIFF=true
 fi
 
+cleanup_on_exit() {
+	test -z "$NFT_TEST_TMPDIR" || rm -rf "$NFT_TEST_TMPDIR"
+}
+trap cleanup_on_exit EXIT
+
+_TMPDIR="${TMPDIR:-/tmp}"
+
+NFT_TEST_TMPDIR="$(mktemp --tmpdir="$_TMPDIR" -d "nft-test.$(date '+%Y%m%d-%H%M%S.%3N').XXXXXX")"
+chmod 755 "$NFT_TEST_TMPDIR"
+
+ln -snf "$NFT_TEST_TMPDIR" "$_TMPDIR/nft-test.latest"
+
+# export the tmp directory for tests. They may use it, but create
+# distinct files! It will be deleted on EXIT.
+export NFT_TEST_TMPDIR
+
+
 kernel_cleanup() {
 	$NFT flush ruleset
 	$MODPROBE -raq \
@@ -193,16 +210,10 @@ EOF
 }
 
 if [ "$VALGRIND" == "y" ]; then
-	tmpd=$(mktemp -d)
-	chmod 755 $tmpd
-
-	msg_info "writing valgrind logs to $tmpd"
-
-	printscript "$NFT" "$tmpd" >${tmpd}/nft
-	trap "rm ${tmpd}/nft" EXIT
-	chmod a+x ${tmpd}/nft
-
-	NFT="${tmpd}/nft"
+	msg_info "writing valgrind logs to $NFT_TEST_TMPDIR"
+	printscript "$NFT" "$NFT_TEST_TMPDIR" > "$NFT_TEST_TMPDIR/nft"
+	chmod a+x "$NFT_TEST_TMPDIR/nft"
+	NFT="$NFT_TEST_TMPDIR/nft"
 fi
 
 echo ""
-- 
2.41.0

[PATCH nft v3 05/11] tests/shell: run each test in separate namespace and allow rootless

[Thomas Haller]

Don't unshare the entire shell script. Instead, call unshare each test
separately. That means, all tests use now a different sandbox and will
also allow (with further changes) to run them in parallel.

Also, allow to run rootless/unprivileged.

The script first tries to run a separate PID+USER+NET namespace. If that
fails, it downgrades to USER+NET. If that fails, it downgrades to a
separate NET namespace. If unshare still fails, the script fails
entirely. That differs from before, where the script would proceed
without sandboxing. The script will now always require that unsharing
works, unless the user opts-out.

If the user cannot unshare, they have two options. Either the
NFT_TEST_NO_UNSHARE=y variable (-U/--no-unshare), which disables unshare
done by the script. This allows the user to run the script as before.
Alternatively, the user can set NFT_TEST_UNSHARE_CMD for the command to
unshare, including setting it to empty for no unshare.

If we are able to create a separate USER namespace, then this mode
allows to run the test as rootless/unprivileged. We no longer require
[ `id -u` = 0 ]. Some tests may not work as rootless. For example, the
socket buffers is limited by /proc/sys/net/core/{wmem_max,rmem_max}
which real-root can override, but rootless tests cannot. Such tests
should check for [ "$NFT_TEST_HAVE_REALROOT" != y ] and skip gracefully.

Usually, the user doesn't need to tell the script whether they have
real-root. The script will autodetect it via [ `id -u` = 0 ]. But that
won't work when run inside a rootless container already. In that case,
the user would want to tell the script that there is no real-root. They
can do so via the -R/--without-root option or NFT_TEST_HAVE_REALROOT=n.

If tests wish, the can know whether they run inside "unshare"
environment by checking for [ "$NFT_TEST_IS_UNSHARED" = y ].

When setting NFT_TEST_UNSHARE_CMD to override the unshare command, users
may want to also set NFT_TEST_IS_UNSHARED= and NFT_TEST_HAVE_REALROOT=
correctly.

As we run each test in a separate unshare environment, we need a wrapper
"tests/shell/helpers/test-wrapper.sh" around the test, which executes
inside the tested environment. Also, each test gets its own temp
directory prepared in NFT_TEST_TESTTMPDIR. This is also the place, where
test artifacts and results will be collected.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh |  21 ++++++
 tests/shell/run-tests.sh            | 109 ++++++++++++++++++++++------
 2 files changed, 107 insertions(+), 23 deletions(-)
 create mode 100755 tests/shell/helpers/test-wrapper.sh

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
new file mode 100755
index 000000000000..99546f060e26
--- /dev/null
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -0,0 +1,21 @@
+#!/bin/bash -e
+
+# This wrapper wraps the invocation of the test. It is called by run-tests.sh,
+# and already in the unshared namespace.
+#
+# For some printf debugging, you can also patch this file.
+
+TEST="$1"
+
+rc_test=0
+"$TEST" |& tee "$NFT_TEST_TESTTMPDIR/testout.log" || rc_test=$?
+
+if [ "$rc_test" -eq 0 ] ; then
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-ok"
+else
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-failed"
+fi
+
+$NFT list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after"
+
+exit "$rc_test"
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 22a34f1f7898..3d0ef6fa8499 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -17,12 +17,14 @@ usage() {
 	echo " $0 [OPTIONS]"
 	echo
 	echo "OPTIONS:"
-	echo " -h|--help       : print usage"
-	echo " -L|--list-tests : list test names and quit"
-	echo " -v              : sets VERBOSE=y"
-	echo " -g              : sets DUMPGEN=y"
-	echo " -V              : sets VALGRIND=y"
-	echo " -K              : sets KMEMLEAK=y"
+	echo " -h|--help        : print usage"
+	echo " -L|--list-tests  : list test names and quit"
+	echo " -v               : sets VERBOSE=y"
+	echo " -g               : sets DUMPGEN=y"
+	echo " -V               : sets VALGRIND=y"
+	echo " -K               : sets KMEMLEAK=y"
+	echo " -R|--without-realroot : sets NFT_TEST_HAVE_REALROOT=n"
+	echo " -U|--no-unshare  : sets NFT_TEST_NO_UNSHARE=y"
 	echo
 	echo "ENVIRONMENT VARIABLES:"
 	echo " NFT=<PATH>   : Path to nft executable"
@@ -30,6 +32,24 @@ usage() {
 	echo " DUMPGEN=*|y  : Regenerate dump files"
 	echo " VALGRIND=*|y : Run \$NFT in valgrind"
 	echo " KMEMLEAK=*|y : Check for kernel memleaks"
+	echo " NFT_TEST_HAVE_REALROOT=*|y : To indicate whether the test has real root permissions."
+	echo "                Usually, you don't need this and it gets autodetected."
+	echo "                You might want to set it, if you know better than the"
+	echo "                \`id -u\` check, whether the user is root in the main namespace."
+	echo "                Note that without real root, certain tests may not work,"
+	echo "                e.g. due to limited /proc/sys/net/core/{wmem_max,rmem_max}."
+	echo "                Checks that cannot pass in such environment should check for"
+	echo "                [ \"\$NFT_TEST_HAVE_REALROOT\" != y ] and skip gracefully."
+	echo " NFT_TEST_NO_UNSHARE=*|y : Usually, each test will run in a separate user namespace."
+	echo "                That allows to run rootless."
+	echo "                If creating a user namespace fails, fallback to only unshare the"
+	echo "                network namespace. This requires root. If that still fails, error out."
+	echo "                Setting NFT_TEST_NO_UNSHARE=y disables any unshare and the test runs"
+	echo "                in the callers namespace. This is ignored when NFT_TEST_UNSHARE_CMD is set."
+	echo " NFT_TEST_UNSHARE_CMD=cmd : when set, NFT_TEST_NO_UNSHARE is ignored and the script"
+	echo "                will not try to unshare. Instead, it uses this command to unshare."
+	echo "                Set to empty to not unshare. You may want to export NFT_TEST_IS_UNSHARED="
+	echo "                and NFT_TEST_HAVE_REALROOT= accordingly."
 }
 
 NFT_TEST_BASEDIR="$(dirname "$0")"
@@ -37,23 +57,12 @@ NFT_TEST_BASEDIR="$(dirname "$0")"
 # Export the base directory. It may be used by tests.
 export NFT_TEST_BASEDIR
 
-if [ "$(id -u)" != "0" ] ; then
-	msg_error "this requires root!"
-fi
-
-if [ "${1}" != "run" ]; then
-	if unshare -f -n true; then
-		unshare -n "${0}" run $@
-		exit $?
-	fi
-	msg_warn "cannot run in own namespace, connectivity might break"
-fi
-shift
-
 VERBOSE="$VERBOSE"
 DUMPGEN="$DUMPGEN"
 VALGRIND="$VALGRIND"
 KMEMLEAK="$KMEMLEAK"
+NFT_TEST_HAVE_REALROOT="$NFT_TEST_HAVE_REALROOT"
+NFT_TEST_NO_UNSHARE="$NFT_TEST_NO_UNSHARE"
 DO_LIST_TESTS=
 
 TESTS=()
@@ -81,6 +90,12 @@ while [ $# -gt 0 ] ; do
 		-L|--list-tests)
 			DO_LIST_TESTS=y
 			;;
+		-R|--without-realroot)
+			NFT_TEST_HAVE_REALROOT=n
+			;;
+		-U|--no-unshare)
+			NFT_TEST_NO_UNSHARE=y
+			;;
 		--)
 			TESTS+=( "$@" )
 			shift $#
@@ -120,6 +135,42 @@ if [ "$DO_LIST_TESTS" = y ] ; then
 	exit 0
 fi
 
+if [ "$NFT_TEST_HAVE_REALROOT" = "" ] ; then
+	# The caller didn't set NFT_TEST_HAVE_REALROOT and didn't specify
+	# -R/--without-root option. Autodetect it based on `id -u`.
+	export NFT_TEST_HAVE_REALROOT="$(test "$(id -u)" = "0" && echo y || echo n)"
+fi
+
+if [ -n "${NFT_TEST_UNSHARE_CMD+x}" ] ; then
+	# User overrides the unshare command. We ignore NFT_TEST_NO_UNSHARE.
+	# It may be set to empty, to not unshare. The user should export NFT_TEST_IS_UNSHARED=
+	# accordingly.
+	NFT_TEST_UNSHARE_CMD="${NFT_TEST_UNSHARE_CMD}"
+	NFT_TEST_IS_UNSHARED="${NFT_TEST_IS_UNSHARED}"
+else
+	NFT_TEST_UNSHARE_CMD=""
+	NFT_TEST_IS_UNSHARED="n"
+	if [ "$NFT_TEST_NO_UNSHARE" != y ] ; then
+		# We unshare both if we NFT_TEST_HAVE_REALROOT and the rootless/unpriv
+		# case. Without real root, some tests may fail. Tests that don't work
+		# without real root should check for [ "$NFT_TEST_HAVE_REALROOT" != y ]
+		# and skip gracefully.
+		NFT_TEST_UNSHARE_CMD="unshare -f -p --mount-proc -U --map-root-user -n"
+		if ! $NFT_TEST_UNSHARE_CMD true &>/dev/null ; then
+			NFT_TEST_UNSHARE_CMD="unshare -f -U --map-root-user -n"
+			if ! $NFT_TEST_UNSHARE_CMD true &>/dev/null ; then
+				NFT_TEST_UNSHARE_CMD="unshare -f -n"
+				if ! $NFT_TEST_UNSHARE_CMD true &>/dev/null ; then
+					msg_error "Unshare does not work. Run as root with -U/--no-unshare/NFT_TEST_NO_UNSHARE=y or set NFT_TEST_UNSHARE_CMD"
+				fi
+			fi
+		fi
+		NFT_TEST_IS_UNSHARED=y
+	fi
+fi
+# If tests wish, they can know whether they are unshared via this variable.
+export NFT_TEST_IS_UNSHARED
+
 [ -z "$NFT" ] && NFT="$NFT_TEST_BASEDIR/../../src/nft"
 ${NFT} > /dev/null 2>&1
 ret=$?
@@ -157,7 +208,9 @@ export NFT_TEST_TMPDIR
 
 
 kernel_cleanup() {
-	$NFT flush ruleset
+	if [ "$NFT_TEST_IS_UNSHARED" != y ] ; then
+		$NFT flush ruleset
+	fi
 	$MODPROBE -raq \
 	nft_reject_ipv4 nft_reject_bridge nft_reject_ipv6 nft_reject \
 	nft_redir_ipv4 nft_redir_ipv6 nft_redir \
@@ -275,18 +328,23 @@ for testfile in "${TESTS[@]}" ; do
 	read taint < /proc/sys/kernel/tainted
 	kernel_cleanup
 
+	# We also create and export a test-specific temporary directory.
+	NFT_TEST_TESTTMPDIR="$(mktemp -p "$NFT_TEST_TMPDIR" -d "test-${testfile//\//-}.XXXXXX")"
+	export NFT_TEST_TESTTMPDIR
+
 	msg_info "[EXECUTING]	$testfile"
-	test_output=$(NFT="$NFT" DIFF=$DIFF ${testfile} 2>&1)
+	test_output="$(NFT="$NFT" DIFF=$DIFF $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile" 2>&1)"
 	rc_got=$?
 	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
 
 	if [ "$rc_got" -eq 0 ] ; then
+		# FIXME: this should move inside test-wrapper.sh.
 		# check nft dump only for positive tests
 		dumppath="$(dirname ${testfile})/dumps"
 		dumpfile="${dumppath}/$(basename ${testfile}).nft"
 		rc_spec=0
 		if [ "$rc_got" -eq 0 ] && [ -f ${dumpfile} ]; then
-			test_output=$(${DIFF} -u ${dumpfile} <($NFT list ruleset) 2>&1)
+			test_output=$(${DIFF} -u ${dumpfile} <(cat "$NFT_TEST_TESTTMPDIR/ruleset-after") 2>&1)
 			rc_spec=$?
 		fi
 
@@ -297,7 +355,7 @@ for testfile in "${TESTS[@]}" ; do
 
 			if [ "$DUMPGEN" == "y" ] && [ "$rc_got" == 0 ] && [ ! -f "${dumpfile}" ]; then
 				mkdir -p "${dumppath}"
-				$NFT list ruleset > "${dumpfile}"
+				cat "$NFT_TEST_TESTTMPDIR/ruleset-after" > "${dumpfile}"
 			fi
 		else
 			((failed++))
@@ -335,4 +393,9 @@ check_kmemleak_force
 msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
 
 kernel_cleanup
+
+if [ "$failed" -gt 0 -a "$NFT_TEST_HAVE_REALROOT" != y ] ; then
+	msg_info "test was not running as real root"
+fi
+
 [ "$failed" -eq 0 ]
-- 
2.41.0

[PATCH nft v3 06/11] tests/shell: interpret an exit code of 77 from scripts as "skipped"

[Thomas Haller]

Allow scripts to indicate that a test could not run by exiting 77.

"77" is chosen as exit code from automake's testsuites ([1]). Compare to
git-bisect which chooses 125 to indicate skipped.

[1] https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh |  2 ++
 tests/shell/run-tests.sh            | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index 99546f060e26..8f0dc685e1fe 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -12,6 +12,8 @@ rc_test=0
 
 if [ "$rc_test" -eq 0 ] ; then
 	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-ok"
+elif [ "$rc_test" -eq 77 ] ; then
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-skipped"
 else
 	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-failed"
 fi
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 3d0ef6fa8499..f3b58e1e200b 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -271,6 +271,7 @@ fi
 
 echo ""
 ok=0
+skipped=0
 failed=0
 taint=0
 
@@ -366,6 +367,14 @@ for testfile in "${TESTS[@]}" ; do
 				msg_warn "[DUMP FAIL]	$testfile"
 			fi
 		fi
+	elif [ "$rc_got" -eq 77 ] ; then
+		((skipped++))
+		if [ "$VERBOSE" == "y" ] ; then
+			msg_warn "[SKIPPED]	$testfile"
+			[ ! -z "$test_output" ] && echo "$test_output"
+		else
+			msg_warn "[SKIPPED]	$testfile"
+		fi
 	else
 		((failed++))
 		if [ "$VERBOSE" == "y" ] ; then
@@ -390,7 +399,7 @@ echo ""
 kmemleak_found=0
 check_kmemleak_force
 
-msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
+msg_info "results: [OK] $ok [SKIPPED] $skipped [FAILED] $failed [TOTAL] $((ok+skipped+failed))"
 
 kernel_cleanup
 
-- 
2.41.0

[PATCH nft v3 07/11] tests/shell: support --keep-logs option (NFT_TEST_KEEP_LOGS=y) to preserve test output

[Thomas Haller]

The test output is now all collected in the temporary directory. On
success, that directory is deleted. Add an option to always preserve
that directory.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index f3b58e1e200b..8da0cc3d5702 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -25,6 +25,7 @@ usage() {
 	echo " -K               : sets KMEMLEAK=y"
 	echo " -R|--without-realroot : sets NFT_TEST_HAVE_REALROOT=n"
 	echo " -U|--no-unshare  : sets NFT_TEST_NO_UNSHARE=y"
+	echo " -k|--keep-logs   : sets NFT_TEST_KEEP_LOGS=y"
 	echo
 	echo "ENVIRONMENT VARIABLES:"
 	echo " NFT=<PATH>   : Path to nft executable"
@@ -50,6 +51,7 @@ usage() {
 	echo "                will not try to unshare. Instead, it uses this command to unshare."
 	echo "                Set to empty to not unshare. You may want to export NFT_TEST_IS_UNSHARED="
 	echo "                and NFT_TEST_HAVE_REALROOT= accordingly."
+	echo " NFT_TEST_KEEP_LOGS=*|y: Keep the temp directory. On success, it will be deleted by default."
 }
 
 NFT_TEST_BASEDIR="$(dirname "$0")"
@@ -61,6 +63,7 @@ VERBOSE="$VERBOSE"
 DUMPGEN="$DUMPGEN"
 VALGRIND="$VALGRIND"
 KMEMLEAK="$KMEMLEAK"
+NFT_TEST_KEEP_LOGS="$NFT_TEST_KEEP_LOGS"
 NFT_TEST_HAVE_REALROOT="$NFT_TEST_HAVE_REALROOT"
 NFT_TEST_NO_UNSHARE="$NFT_TEST_NO_UNSHARE"
 DO_LIST_TESTS=
@@ -87,6 +90,9 @@ while [ $# -gt 0 ] ; do
 			usage
 			exit 0
 			;;
+		-k|--keep-logs)
+			NFT_TEST_KEEP_LOGS=y
+			;;
 		-L|--list-tests)
 			DO_LIST_TESTS=y
 			;;
@@ -202,8 +208,8 @@ chmod 755 "$NFT_TEST_TMPDIR"
 
 ln -snf "$NFT_TEST_TMPDIR" "$_TMPDIR/nft-test.latest"
 
-# export the tmp directory for tests. They may use it, but create
-# distinct files! It will be deleted on EXIT.
+# export the tmp directory for tests. They may use it, but create distinct
+# files! On success, it will be deleted on EXIT. See also "--keep-logs"
 export NFT_TEST_TMPDIR
 
 
@@ -407,4 +413,12 @@ if [ "$failed" -gt 0 -a "$NFT_TEST_HAVE_REALROOT" != y ] ; then
 	msg_info "test was not running as real root"
 fi
 
+if [ "$failed" -gt 0 -o "$NFT_TEST_KEEP_LOGS" = y ] ; then
+	ln -snf "$NFT_TEST_TMPDIR" "$_TMPDIR/nftables-test.latest"
+	msg_info "check the temp directory \"$NFT_TEST_TMPDIR\" (\"$_TMPDIR/nftables-test.latest\")"
+	msg_info "   ls -lad /tmp/nftables-test.latest/*/*"
+	msg_info "   grep -R ^ /tmp/nftables-test.latest/"
+	NFT_TEST_TMPDIR=
+fi
+
 [ "$failed" -eq 0 ]
-- 
2.41.0

[PATCH nft v3 08/11] tests/shell: move the dump diff handling inside "test-wrapper.sh"

[Thomas Haller]

This fits there better. At this point, we are  still inside the unshared
namespace and right after the test. The test-wrapper.sh should compare
(and generate) the dumps.

Also change behavior for DUMPGEN=y.

- Previously it would only rewrite the dump if the dumpfile didn't
  exist yet. Now instead, always rewrite the file with DUMPGEN=y.
  The mode of operation is anyway, that the developer afterwards
  checks `git diff|status` to pick up the changes. There should be
  no changes to existing files (as existing tests are supposed to
  pass). So a diff there either means something went wrong (and we
  should see it) or it just means the dumps correctly should be
  regenerated.

- also, only generate the file if the "dumps/" directory exists. This
  allows to write tests that don't have a dump file and don't get it
  automatically generated.

The test wrapper will return a special error code 124 to indicate that
the test passed, but the dumps file differed.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh | 56 +++++++++++++++++++++++++----
 tests/shell/run-tests.sh            | 45 +++++++++--------------
 2 files changed, 66 insertions(+), 35 deletions(-)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index 8f0dc685e1fe..f9d759d0bb03 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -6,18 +6,60 @@
 # For some printf debugging, you can also patch this file.
 
 TEST="$1"
+TESTBASE="$(basename "$TEST")"
+TESTDIR="$(dirname "$TEST")"
 
 rc_test=0
 "$TEST" |& tee "$NFT_TEST_TESTTMPDIR/testout.log" || rc_test=$?
 
-if [ "$rc_test" -eq 0 ] ; then
-	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-ok"
+$NFT list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after"
+
+DUMPPATH="$TESTDIR/dumps"
+DUMPFILE="$DUMPPATH/$TESTBASE.nft"
+
+dump_written=
+rc_dump=
+
+# The caller can request a re-geneating of the dumps, by setting
+# DUMPGEN=y.
+#
+# This only will happen if the command completed with success.
+#
+# It also will only happen for tests, that have a "$DUMPPATH" directory. There
+# might be tests, that don't want to have dumps created. The existence of the
+# directory controls that.
+if [ "$rc_test" -eq 0 -a "$DUMPGEN" = y -a -d "$DUMPPATH" ] ; then
+	dump_written=y
+	cat "$NFT_TEST_TESTTMPDIR/ruleset-after" > "$DUMPFILE"
+fi
+
+if [ "$rc_test" -ne 77 -a -f "$DUMPFILE" ] ; then
+	rc_dump=0
+	if [ "$dump_written" != y ] ; then
+		$DIFF -u "$DUMPFILE" "$NFT_TEST_TESTTMPDIR/ruleset-after" &> "$NFT_TEST_TESTTMPDIR/ruleset-diff" || rc_dump=$?
+		if [ "$rc_dump" -eq 0 ] ; then
+			rm -f "$NFT_TEST_TESTTMPDIR/ruleset-diff"
+		fi
+	fi
+fi
+
+rc_exit="$rc_test"
+if [ -n "$rc_dump" ] && [ "$rc_dump" -ne 0 ] ; then
+	echo "$DUMPFILE" > "$NFT_TEST_TESTTMPDIR/rc-failed-dump"
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-failed"
+	if [ "$rc_exit" -eq 0 ] ; then
+		# Special exit code to indicate dump diff.
+		rc_exit=124
+	fi
+elif [ "$rc_test" -eq 0 ] ; then
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-ok"
 elif [ "$rc_test" -eq 77 ] ; then
-	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-skipped"
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-skipped"
 else
-	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc_test-failed"
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-failed"
+	if [ "$rc_test" -eq 124 ] ; then
+		rc_exit=125
+	fi
 fi
 
-$NFT list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after"
-
-exit "$rc_test"
+exit "$rc_exit"
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 8da0cc3d5702..a35337750ab7 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -30,7 +30,9 @@ usage() {
 	echo "ENVIRONMENT VARIABLES:"
 	echo " NFT=<PATH>   : Path to nft executable"
 	echo " VERBOSE=*|y  : Enable verbose output"
-	echo " DUMPGEN=*|y  : Regenerate dump files"
+	echo " DUMPGEN=*|y  : Regenerate dump files. Dump files are only recreated if the"
+	echo "                test completes successfully and the \"dumps\" directory for the"
+	echo "                test exits."
 	echo " VALGRIND=*|y : Run \$NFT in valgrind"
 	echo " KMEMLEAK=*|y : Check for kernel memleaks"
 	echo " NFT_TEST_HAVE_REALROOT=*|y : To indicate whether the test has real root permissions."
@@ -340,38 +342,25 @@ for testfile in "${TESTS[@]}" ; do
 	export NFT_TEST_TESTTMPDIR
 
 	msg_info "[EXECUTING]	$testfile"
-	test_output="$(NFT="$NFT" DIFF=$DIFF $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile" 2>&1)"
+	test_output="$(NFT="$NFT" DIFF=$DIFF DUMPGEN="$DUMPGEN" $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile" 2>&1)"
 	rc_got=$?
 	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
 
-	if [ "$rc_got" -eq 0 ] ; then
-		# FIXME: this should move inside test-wrapper.sh.
-		# check nft dump only for positive tests
-		dumppath="$(dirname ${testfile})/dumps"
-		dumpfile="${dumppath}/$(basename ${testfile}).nft"
-		rc_spec=0
-		if [ "$rc_got" -eq 0 ] && [ -f ${dumpfile} ]; then
-			test_output=$(${DIFF} -u ${dumpfile} <(cat "$NFT_TEST_TESTTMPDIR/ruleset-after") 2>&1)
-			rc_spec=$?
-		fi
-
-		if [ "$rc_spec" -eq 0 ]; then
-			msg_info "[OK]		$testfile"
-			[ "$VERBOSE" == "y" ] && [ ! -z "$test_output" ] && echo "$test_output"
-			((ok++))
+	if [ -s "$NFT_TEST_TESTTMPDIR/ruleset-diff" ] ; then
+		test_output="$test_output$(cat "$NFT_TEST_TESTTMPDIR/ruleset-diff")"
+	fi
 
-			if [ "$DUMPGEN" == "y" ] && [ "$rc_got" == 0 ] && [ ! -f "${dumpfile}" ]; then
-				mkdir -p "${dumppath}"
-				cat "$NFT_TEST_TESTTMPDIR/ruleset-after" > "${dumpfile}"
-			fi
+	if [ "$rc_got" -eq 0 ] ; then
+		((ok++))
+		msg_info "[OK]		$testfile"
+		[ "$VERBOSE" == "y" ] && [ ! -z "$test_output" ] && echo "$test_output"
+	elif [ "$rc_got" -eq 124 ] ; then
+		((failed++))
+		if [ "$VERBOSE" == "y" ] ; then
+			msg_warn "[DUMP FAIL]	$testfile: dump diff detected"
+			[ ! -z "$test_output" ] && echo "$test_output"
 		else
-			((failed++))
-			if [ "$VERBOSE" == "y" ] ; then
-				msg_warn "[DUMP FAIL]	$testfile: dump diff detected"
-				[ ! -z "$test_output" ] && echo "$test_output"
-			else
-				msg_warn "[DUMP FAIL]	$testfile"
-			fi
+			msg_warn "[DUMP FAIL]	$testfile"
 		fi
 	elif [ "$rc_got" -eq 77 ] ; then
 		((skipped++))
-- 
2.41.0

[PATCH nft v3 09/11] tests/shell: rework printing of test results

[Thomas Haller]

- "test-wrapper.sh" no longer will print the test output to its stdout.
  Instead, it only writes the testout.log file.

- rework the loop "run-tests.sh" for printing the test results. It no
  longer captures the output of the test, as the wrapper is expected to
  be silent. Instead, they get the output from the result directory.
  The benefit is, that there is no duplication in what we print and the
  captured data in the result directory. The verbose mode is only for
  convenience, to safe looking at the test data. It's not essential
  otherwise.

- also move the evaluation of the test result (and printing of the
  information) to a separate function. Later we want to run tests in
  parallel, so the steps need to be clearly separated.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh |   2 +-
 tests/shell/run-tests.sh            | 116 +++++++++++++++++++---------
 2 files changed, 81 insertions(+), 37 deletions(-)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index f9d759d0bb03..89ac9ff4ee15 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -10,7 +10,7 @@ TESTBASE="$(basename "$TEST")"
 TESTDIR="$(dirname "$TEST")"
 
 rc_test=0
-"$TEST" |& tee "$NFT_TEST_TESTTMPDIR/testout.log" || rc_test=$?
+"$TEST" &> "$NFT_TEST_TESTTMPDIR/testout.log" || rc_test=$?
 
 $NFT list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after"
 
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index a35337750ab7..4f162b4514cc 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -1,16 +1,31 @@
 #!/bin/bash
 
+_msg() {
+	local level="$1"
+	shift
+	local msg
+
+	msg="$level: $*"
+	if [ "$level" = E -o "$level" = W ] ; then
+		printf '%s\n' "$msg" >&2
+	else
+		printf '%s\n' "$msg"
+	fi
+	if [ "$level" = E ] ; then
+		exit 1
+	fi
+}
+
 msg_error() {
-	echo "E: $1 ..." >&2
-	exit 1
+	_msg E "$@"
 }
 
 msg_warn() {
-	echo "W: $1" >&2
+	_msg W "$@"
 }
 
 msg_info() {
-	echo "I: $1"
+	_msg I "$@"
 }
 
 usage() {
@@ -333,52 +348,81 @@ check_kmemleak()
 
 check_taint
 
-for testfile in "${TESTS[@]}" ; do
-	read taint < /proc/sys/kernel/tainted
-	kernel_cleanup
+print_test_header() {
+	local msglevel="$1"
+	local testfile="$2"
+	local status="$3"
+	local suffix="$4"
+	local text
 
-	# We also create and export a test-specific temporary directory.
-	NFT_TEST_TESTTMPDIR="$(mktemp -p "$NFT_TEST_TMPDIR" -d "test-${testfile//\//-}.XXXXXX")"
-	export NFT_TEST_TESTTMPDIR
+	if [ -n "$suffix" ] ; then
+		suffix=": $suffix"
+	fi
+	text="[$status]"
+	text="$(printf '%-12s' "$text")"
+	_msg "$msglevel" "$text $testfile$suffix"
+}
 
-	msg_info "[EXECUTING]	$testfile"
-	test_output="$(NFT="$NFT" DIFF=$DIFF DUMPGEN="$DUMPGEN" $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile" 2>&1)"
-	rc_got=$?
-	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
+print_test_result() {
+	local NFT_TEST_TESTTMPDIR="$1"
+	local testfile="$2"
+	local rc_got="$3"
+	shift 3
 
-	if [ -s "$NFT_TEST_TESTTMPDIR/ruleset-diff" ] ; then
-		test_output="$test_output$(cat "$NFT_TEST_TESTTMPDIR/ruleset-diff")"
-	fi
+	local result_msg_level="I"
+	local result_msg_status="OK"
+	local result_msg_suffix=""
+	local result_msg_files=( "$NFT_TEST_TESTTMPDIR/testout.log" "$NFT_TEST_TESTTMPDIR/ruleset-diff" )
 
 	if [ "$rc_got" -eq 0 ] ; then
 		((ok++))
-		msg_info "[OK]		$testfile"
-		[ "$VERBOSE" == "y" ] && [ ! -z "$test_output" ] && echo "$test_output"
 	elif [ "$rc_got" -eq 124 ] ; then
 		((failed++))
-		if [ "$VERBOSE" == "y" ] ; then
-			msg_warn "[DUMP FAIL]	$testfile: dump diff detected"
-			[ ! -z "$test_output" ] && echo "$test_output"
-		else
-			msg_warn "[DUMP FAIL]	$testfile"
-		fi
+		result_msg_level="W"
+		result_msg_status="DUMP FAIL"
 	elif [ "$rc_got" -eq 77 ] ; then
 		((skipped++))
-		if [ "$VERBOSE" == "y" ] ; then
-			msg_warn "[SKIPPED]	$testfile"
-			[ ! -z "$test_output" ] && echo "$test_output"
-		else
-			msg_warn "[SKIPPED]	$testfile"
-		fi
+		result_msg_level="I"
+		result_msg_status="SKIPPED"
 	else
 		((failed++))
-		if [ "$VERBOSE" == "y" ] ; then
-			msg_warn "[FAILED]	$testfile: got $rc_got"
-			[ ! -z "$test_output" ] && echo "$test_output"
-		else
-			msg_warn "[FAILED]	$testfile"
+		result_msg_level="W"
+		result_msg_status="FAILED"
+		result_msg_suffix="got $rc_got"
+		result_msg_files=( "$NFT_TEST_TESTTMPDIR/testout.log" )
+	fi
+
+	print_test_header "$result_msg_level" "$testfile" "$result_msg_status" "$result_msg_suffix"
+
+	if [ "$VERBOSE" = "y" ] ; then
+		local f
+
+		for f in "${result_msg_files[@]}"; do
+			if [ -s "$f" ] ; then
+				cat "$f"
+			fi
+		done
+
+		if [ "$rc_got" -ne 0 ] ; then
+			msg_info "check \"$NFT_TEST_TESTTMPDIR\""
 		fi
 	fi
+}
+
+for testfile in "${TESTS[@]}" ; do
+	read taint < /proc/sys/kernel/tainted
+	kernel_cleanup
+
+	# We also create and export a test-specific temporary directory.
+	NFT_TEST_TESTTMPDIR="$(mktemp -p "$NFT_TEST_TMPDIR" -d "test-${testfile//\//-}.XXXXXX")"
+	export NFT_TEST_TESTTMPDIR
+
+	print_test_header I "$testfile" "EXECUTING" ""
+	NFT="$NFT" DIFF="$DIFF" DUMPGEN="$DUMPGEN" $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile"
+	rc_got=$?
+	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
+
+	print_test_result "$NFT_TEST_TESTTMPDIR" "$testfile" "$rc_got"
 
 	check_taint
 	check_kmemleak
-- 
2.41.0

[PATCH nft v3 10/11] tests/shell: move taint check to "test-wrapper.sh"

[Thomas Haller]

We will run tests in parallel. That means, we have multiple tests data and results
in fly. That becomes simpler, if we move more result data to the
test-wrapper and out of "run-tests.sh".

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh | 18 +++++++++++++++---
 tests/shell/run-tests.sh            | 16 ++++------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index 89ac9ff4ee15..34d849e6e17e 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -9,11 +9,15 @@ TEST="$1"
 TESTBASE="$(basename "$TEST")"
 TESTDIR="$(dirname "$TEST")"
 
+read tainted_before < /proc/sys/kernel/tainted
+
 rc_test=0
 "$TEST" &> "$NFT_TEST_TESTTMPDIR/testout.log" || rc_test=$?
 
 $NFT list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after"
 
+read tainted_after < /proc/sys/kernel/tainted
+
 DUMPPATH="$TESTDIR/dumps"
 DUMPFILE="$DUMPPATH/$TESTBASE.nft"
 
@@ -43,6 +47,10 @@ if [ "$rc_test" -ne 77 -a -f "$DUMPFILE" ] ; then
 	fi
 fi
 
+if [ "$tainted_before" != "$tainted_after" ]; then
+	echo "$tainted_after" > "$NFT_TEST_TESTTMPDIR/rc-failed-tainted"
+fi
+
 rc_exit="$rc_test"
 if [ -n "$rc_dump" ] && [ "$rc_dump" -ne 0 ] ; then
 	echo "$DUMPFILE" > "$NFT_TEST_TESTTMPDIR/rc-failed-dump"
@@ -51,13 +59,17 @@ if [ -n "$rc_dump" ] && [ "$rc_dump" -ne 0 ] ; then
 		# Special exit code to indicate dump diff.
 		rc_exit=124
 	fi
-elif [ "$rc_test" -eq 0 ] ; then
-	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-ok"
 elif [ "$rc_test" -eq 77 ] ; then
 	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-skipped"
+elif [ "$rc_test" -eq 0 -a "$tainted_before" = "$tainted_after" ] ; then
+	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-ok"
 else
 	echo "$rc_test" > "$NFT_TEST_TESTTMPDIR/rc-failed"
-	if [ "$rc_test" -eq 124 ] ; then
+	if [ "$rc_test" -eq 0 -a "$tainted_before" != "$tainted_after" ] ; then
+		# Special exit code to indicate tainted.
+		rc_exit=123
+	elif [ "$rc_test" -eq 124 -o "$rc_test" -eq 123 ] ; then
+		# These exit codes are reserved
 		rc_exit=125
 	fi
 fi
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 4f162b4514cc..1451d913bd3a 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -296,15 +296,6 @@ echo ""
 ok=0
 skipped=0
 failed=0
-taint=0
-
-check_taint()
-{
-	read taint_now < /proc/sys/kernel/tainted
-	if [ $taint -ne $taint_now ] ; then
-		msg_warn "[FAILED]	kernel is tainted: $taint  -> $taint_now"
-	fi
-}
 
 kmem_runs=0
 kmemleak_found=0
@@ -346,7 +337,10 @@ check_kmemleak()
 	fi
 }
 
-check_taint
+read kernel_tainted < /proc/sys/kernel/tainted
+if [ "$kernel_tainted" -ne 0 ] ; then
+	msg_warn "[FAILED]	kernel is tainted"
+fi
 
 print_test_header() {
 	local msglevel="$1"
@@ -410,7 +404,6 @@ print_test_result() {
 }
 
 for testfile in "${TESTS[@]}" ; do
-	read taint < /proc/sys/kernel/tainted
 	kernel_cleanup
 
 	# We also create and export a test-specific temporary directory.
@@ -424,7 +417,6 @@ for testfile in "${TESTS[@]}" ; do
 
 	print_test_result "$NFT_TEST_TESTTMPDIR" "$testfile" "$rc_got"
 
-	check_taint
 	check_kmemleak
 done
 
-- 
2.41.0

[PATCH nft v3 11/11] tests/shell: support running tests in parallel

[Thomas Haller]

Add option to enable running jobs in parallel. The purpose is to speed
up the run time of the tests.

The global cleanup (removal of kernel modules) interferes with parallel
jobs (or even with, unrelated jobs on the system). By setting
NFT_TEST_JOBS= to a positive number, that cleanup is skipped.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 83 +++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 14 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 1451d913bd3a..160524e889ed 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -41,6 +41,7 @@ usage() {
 	echo " -R|--without-realroot : sets NFT_TEST_HAVE_REALROOT=n"
 	echo " -U|--no-unshare  : sets NFT_TEST_NO_UNSHARE=y"
 	echo " -k|--keep-logs   : sets NFT_TEST_KEEP_LOGS=y"
+	echo " -j|--jobs        : sets NFT_TEST_JOBS=12"
 	echo
 	echo "ENVIRONMENT VARIABLES:"
 	echo " NFT=<PATH>   : Path to nft executable"
@@ -69,6 +70,10 @@ usage() {
 	echo "                Set to empty to not unshare. You may want to export NFT_TEST_IS_UNSHARED="
 	echo "                and NFT_TEST_HAVE_REALROOT= accordingly."
 	echo " NFT_TEST_KEEP_LOGS=*|y: Keep the temp directory. On success, it will be deleted by default."
+	echo " NFT_TEST_JOBS=<NUM}>: by default, run test sequentially. Set to an integer > 1 to"
+	echo "                run jobs in parallel. Leaving this unset or at zero means to run jobs sequentially"
+	echo "                and perform global cleanups between tests (remove kernel modules). Setting this"
+	echo "                to a positive number (including \"1\") means to disable such global cleanups."
 }
 
 NFT_TEST_BASEDIR="$(dirname "$0")"
@@ -83,6 +88,7 @@ KMEMLEAK="$KMEMLEAK"
 NFT_TEST_KEEP_LOGS="$NFT_TEST_KEEP_LOGS"
 NFT_TEST_HAVE_REALROOT="$NFT_TEST_HAVE_REALROOT"
 NFT_TEST_NO_UNSHARE="$NFT_TEST_NO_UNSHARE"
+NFT_TEST_JOBS="$NFT_TEST_JOBS"
 DO_LIST_TESTS=
 
 TESTS=()
@@ -119,6 +125,9 @@ while [ $# -gt 0 ] ; do
 		-U|--no-unshare)
 			NFT_TEST_NO_UNSHARE=y
 			;;
+		-j|--jobs)
+			NFT_TEST_JOBS=12
+			;;
 		--)
 			TESTS+=( "$@" )
 			shift $#
@@ -132,6 +141,11 @@ while [ $# -gt 0 ] ; do
 	esac
 done
 
+# normalize the jobs number to be an integer.
+case "$NFT_TEST_JOBS" in
+	''|*[!0-9]*) NFT_TEST_JOBS=0 ;;
+esac
+
 find_tests() {
 	find "$1" -type f -executable | sort
 }
@@ -199,13 +213,15 @@ ${NFT} > /dev/null 2>&1
 ret=$?
 if [ ${ret} -eq 126 ] || [ ${ret} -eq 127 ]; then
 	msg_error "cannot execute nft command: ${NFT}"
-else
-	msg_info "using nft command: ${NFT}"
 fi
+msg_info "using nft command: ${NFT}"
+msg_info "parallel job mode: NFT_TEST_JOBS=$NFT_TEST_JOBS"
 
-MODPROBE="$(which modprobe)"
-if [ ! -x "$MODPROBE" ] ; then
-	msg_error "no modprobe binary found"
+if [ "$NFT_TEST_JOBS" -eq 0 ] ; then
+	MODPROBE="$(which modprobe)"
+	if [ ! -x "$MODPROBE" ] ; then
+		msg_error "no modprobe binary found"
+	fi
 fi
 
 DIFF="$(which diff)"
@@ -231,6 +247,11 @@ export NFT_TEST_TMPDIR
 
 
 kernel_cleanup() {
+	if [ "$NFT_TEST_JOBS" -ne 0 ] ; then
+		# When we run jobs in parallel (even with only one "parallel"
+		# job via `NFT_TEST_JOBS=1`), we skip such global cleanups.
+		return
+	fi
 	if [ "$NFT_TEST_IS_UNSHARED" != y ] ; then
 		$NFT flush ruleset
 	fi
@@ -403,23 +424,57 @@ print_test_result() {
 	fi
 }
 
-for testfile in "${TESTS[@]}" ; do
-	kernel_cleanup
+declare -A JOBS_TEMPDIR
+declare -A JOBS_PIDLIST
 
-	# We also create and export a test-specific temporary directory.
-	NFT_TEST_TESTTMPDIR="$(mktemp -p "$NFT_TEST_TMPDIR" -d "test-${testfile//\//-}.XXXXXX")"
-	export NFT_TEST_TESTTMPDIR
+job_start() {
+	local testfile="$1"
 
-	print_test_header I "$testfile" "EXECUTING" ""
+	if [ "$NFT_TEST_JOBS" -le 1 ] ; then
+		print_test_header I "$testfile" "EXECUTING" ""
+	fi
+
+	NFT_TEST_TESTTMPDIR="${JOBS_TEMPDIR["$testfile"]}" \
 	NFT="$NFT" DIFF="$DIFF" DUMPGEN="$DUMPGEN" $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile"
 	rc_got=$?
-	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
 
-	print_test_result "$NFT_TEST_TESTTMPDIR" "$testfile" "$rc_got"
+	if [ "$NFT_TEST_JOBS" -le 1 ] ; then
+		echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
+	fi
+
+	return "$rc_got"
+}
+
+job_wait()
+{
+	local num_jobs="$1"
+
+	while [ "$JOBS_N_RUNNING" -gt 0 -a "$JOBS_N_RUNNING" -ge "$num_jobs" ] ; do
+		wait -n -p JOBCOMPLETED
+		rc_got="$?"
+		testfile2="${JOBS_PIDLIST[$JOBCOMPLETED]}"
+		print_test_result "${JOBS_TEMPDIR["$testfile2"]}" "$testfile2" "$rc_got"
+		((JOBS_N_RUNNING--))
+		check_kmemleak
+	done
+}
+
+JOBS_N_RUNNING=0
+for testfile in "${TESTS[@]}" ; do
+	kernel_cleanup
+
+	job_wait "$NFT_TEST_JOBS"
 
-	check_kmemleak
+	NFT_TEST_TESTTMPDIR="$(mktemp -p "$NFT_TEST_TMPDIR" -d "test-${testfile//\//-}.XXXXXX")"
+	JOBS_TEMPDIR["$testfile"]="$NFT_TEST_TESTTMPDIR"
+
+	job_start "$testfile" &
+	JOBS_PIDLIST[$!]="$testfile"
+	((JOBS_N_RUNNING++))
 done
 
+job_wait 0
+
 echo ""
 
 # kmemleak may report suspected leaks
-- 
2.41.0

Re: [PATCH nft v3 00/11] tests/shell: allow running tests as

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> Changes to v3:

I was about to apply this but 10 tests now fail for me because they
no longer execute as real root and hit the socket buffer limits.

Please fix this, the default needs to be 'all tests pass',
i.e. use plain 'unshare -n' by default.

I'll leave it up to you if you want to automatically go with
unpriv netns if the script is invoked as non-root user or via
env/cmdline switch.

At least one failure isn't your fault, the blame is
with a shortcut check in sets/0043concatenated_ranges_0, so the test
never execeuted fully in the past. I will try
to figure out when this got broken :/

Re: [PATCH nft v3 00/11] tests/shell: allow running tests as

[Thomas Haller]

On Tue, 2023-09-05 at 13:09 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > Ch;anges to v3:
> 
> I was about to apply this but 10 tests now fail for me because they
> no longer execute as real root and hit the socket buffer limits.
> 
> Please fix this, the default needs to be 'all tests pass',
> i.e. use plain 'unshare -n' by default.
> 
> I'll leave it up to you if you want to automatically go with
> unpriv netns if the script is invoked as non-root user or via
> env/cmdline switch.
> 
> At least one failure isn't your fault, the blame is
> with a shortcut check in sets/0043concatenated_ranges_0, so the test
> never execeuted fully in the past. I will try
> to figure out when this got broken :/
> 

hi,

yes, I noticed. Sorry about that.

Please see v4. That works well for me.


A major benefit IMO (which doesn't become clear from the commit
messages) is the result data in "/tmp/nft-test.latest". Have a look
there. And try the new "-j" option. And of course, try without root.

Thanks
Thomas

Re: [PATCH nft v3 00/11] tests/shell: allow running tests as

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> On Tue, 2023-09-05 at 13:09 +0200, Florian Westphal wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > > Ch;anges to v3:
> > 
> > I was about to apply this but 10 tests now fail for me because they
> > no longer execute as real root and hit the socket buffer limits.
> > 
> > Please fix this, the default needs to be 'all tests pass',
> > i.e. use plain 'unshare -n' by default.
> > 
> > I'll leave it up to you if you want to automatically go with
> > unpriv netns if the script is invoked as non-root user or via
> > env/cmdline switch.
> > 
> > At least one failure isn't your fault, the blame is
> > with a shortcut check in sets/0043concatenated_ranges_0, so the test
> > never execeuted fully in the past. I will try
> > to figure out when this got broken :/

Seems its always broken.  Minimal reproducer:

nft -f - <<EOF
table ip filter {
        set test {
                type ipv4_addr . ether_addr . mark
                flags interval
                elements = { 198.51.100.0/25 . 00:0b:0c:ca:cc:10-c1:a0:c1:cc:10:00 . 0x0000006f, }
        }
}
EOF
nft get element ip filter test { 198.51.100.1 . 00:0b:0c:ca:cc:10 . 0x6f }
Error: Could not process rule: No such file or directory
get element ip filter test { 198.51.100.1 . 00:0b:0c:ca:cc:10 . 0x6f }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
nft get element ip filter test { 198.51.100.1 . 00:0b:0c:ca:cc:10 . 0x6f-0x6f }
table ip ...

Seems like this doesn't emit the needed end keys because the 'INTERVAL' flag
isn't toggled without using at least one phony range in the query.