[PATCH nft 1/3] tests/shell: fix preserving ruleset diff after test

[PATCH nft 1/3] tests/shell: fix preserving ruleset diff after test

[Thomas Haller]

We want to delete the file in the case when there was no diff (and we
expect the file to be empty). The condition was wrong.

Fixes: 55fe071cd193 ('tests/shell: cleanup result handling in "test-wrapper.sh"')

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index cd8f480504ad..ad6a71031506 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -125,6 +125,7 @@ if [ "$rc_test" -ne 77 -a -f "$DUMPFILE" ] ; then
 	if [ "$dump_written" != y ] ; then
 		if ! $DIFF -u "$DUMPFILE" "$NFT_TEST_TESTTMPDIR/ruleset-after" &> "$NFT_TEST_TESTTMPDIR/ruleset-diff" ; then
 			rc_dump=124
+		else
 			rm -f "$NFT_TEST_TESTTMPDIR/ruleset-diff"
 		fi
 	fi
-- 
2.41.0

[PATCH nft 2/3] tests/shell: simplify collecting error result in "test-wrapper.sh"

[Thomas Haller]

The previous pattern was unnecessarily confusing.

The "$rc_{dump,valgrind,tainted}" variable should only remember whether
that particular check failed, not the overall exit code of the test
wrapper.

Otherwise, if you want to know in which case the wrapper exits with code
122, you have to oddly follow the rc_valgrind variable.

This change will make more sense, when we add another such variable, but
which will be assigned the non-zero value at multiple places. Assigning
there the exit code of the wrapper, duplicates the places where the
condition maps to the exit code.

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

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index ad6a71031506..165a944da2b1 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -124,7 +124,7 @@ rc_dump=0
 if [ "$rc_test" -ne 77 -a -f "$DUMPFILE" ] ; then
 	if [ "$dump_written" != y ] ; then
 		if ! $DIFF -u "$DUMPFILE" "$NFT_TEST_TESTTMPDIR/ruleset-after" &> "$NFT_TEST_TESTTMPDIR/ruleset-diff" ; then
-			rc_dump=124
+			rc_dump=1
 		else
 			rm -f "$NFT_TEST_TESTTMPDIR/ruleset-diff"
 		fi
@@ -135,27 +135,27 @@ if [ "$rc_dump" -ne 0 ] ; then
 fi
 
 rc_valgrind=0
-[ -f "$NFT_TEST_TESTTMPDIR/rc-failed-valgrind" ] && rc_valgrind=122
+[ -f "$NFT_TEST_TESTTMPDIR/rc-failed-valgrind" ] && rc_valgrind=1
 
 rc_tainted=0
 if [ "$tainted_before" != "$tainted_after" ] ; then
 	echo "$tainted_after" > "$NFT_TEST_TESTTMPDIR/rc-failed-tainted"
-	rc_tainted=123
+	rc_tainted=1
 fi
 
 if [ "$rc_valgrind" -ne 0 ] ; then
-	rc_exit="$rc_valgrind"
+	rc_exit=122
 elif [ "$rc_tainted" -ne 0 ] ; then
-	rc_exit="$rc_tainted"
+	rc_exit=123
 elif [ "$rc_test" -ge 118 -a "$rc_test" -le 124 ] ; then
 	# Special exit codes are reserved. Coerce them.
-	rc_exit="125"
+	rc_exit=125
 elif [ "$rc_test" -ne 0 ] ; then
 	rc_exit="$rc_test"
 elif [ "$rc_dump" -ne 0 ] ; then
-	rc_exit="$rc_dump"
+	rc_exit=124
 else
-	rc_exit="0"
+	rc_exit=0
 fi
 
 
-- 
2.41.0

[PATCH nft 3/3] tests/shell: run `nft --check` on persisted dump files

[Thomas Haller]

"nft --check" will trigger a rollback in kernel. The existing dump files
might hit new code paths. Take the opportunity to call the command on
the existing files.

And alternative would be to write a separate tests, that iterates over
all files. However, then we can only run all the commands sequentially
(unless we do something smart). That might be slower than the
opportunity to run the checks in parallel. More importantly, it would be
nice if the check for the dump file is clearly tied to the file's test.
So run it right after the test, from the test wrapper.

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

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index 165a944da2b1..e10360c9b266 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -134,6 +134,35 @@ if [ "$rc_dump" -ne 0 ] ; then
 	echo "$DUMPFILE" > "$NFT_TEST_TESTTMPDIR/rc-failed-dump"
 fi
 
+rc_chkdump=0
+# check that a flush after the test succeeds. We anyway need a clean ruleset
+# for the `nft --check` next.
+$NFT flush ruleset &> "$NFT_TEST_TESTTMPDIR/rc-failed-chkdump" || rc_chkdump=1
+if [ -f "$DUMPFILE" ] ; then
+	# We have a dumpfile. Call `nft --check` to possibly cover new code
+	# paths.
+	if [ "$rc_test" -eq 77 ] ; then
+		# The test was skipped. Possibly we don't have the required
+		# features to process this file. Ignore any output and exit
+		# code, but still call the program (for valgrind or sanitizer
+		# issue we hope to find).
+		$NFT --check -f "$DUMPFILE" &>/dev/null || :
+	else
+		$NFT --check -f "$DUMPFILE" &>> "$NFT_TEST_TESTTMPDIR/rc-failed-chkdump" || rc_chkdump=1
+	fi
+fi
+if [ -s "$NFT_TEST_TESTTMPDIR/rc-failed-chkdump" ] ; then
+	# Non-empty output? That is wrong.
+	rc_chkdump=1
+elif [ "$rc_chkdump" -eq 0 ] ; then
+	rm -rf "$NFT_TEST_TESTTMPDIR/rc-failed-chkdump"
+fi
+if [ "$rc_chkdump" -ne 0 ] ; then
+	# Ensure we don't have empty output files. Always write something, so
+	# that `grep ^ -R` lists the file.
+	echo -e "<<<<<\n\nCalling \`nft --check\` (or \`nft flush ruleset\`) failed for \"$DUMPFILE\"" >> "$NFT_TEST_TESTTMPDIR/rc-failed-chkdump"
+fi
+
 rc_valgrind=0
 [ -f "$NFT_TEST_TESTTMPDIR/rc-failed-valgrind" ] && rc_valgrind=1
 
@@ -154,6 +183,8 @@ elif [ "$rc_test" -ne 0 ] ; then
 	rc_exit="$rc_test"
 elif [ "$rc_dump" -ne 0 ] ; then
 	rc_exit=124
+elif [ "$rc_chkdump" -ne 0 ] ; then
+	rc_exit=121
 else
 	rc_exit=0
 fi
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 528646f57eca..85aa498ca8ee 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -731,7 +731,9 @@ print_test_result() {
 	else
 		((failed++))
 		result_msg_level="W"
-		if [ "$rc_got" -eq 122 ] ; then
+		if [ "$rc_got" -eq 121 ] ; then
+			result_msg_status="CHK DUMP"
+		elif [ "$rc_got" -eq 122 ] ; then
 			result_msg_status="VALGRIND"
 		elif [ "$rc_got" -eq 123 ] ; then
 			result_msg_status="TAINTED"
-- 
2.41.0

Re: [PATCH nft 3/3] tests/shell: run `nft --check` on persisted dump files

[Pablo Neira Ayuso]

On Mon, Sep 18, 2023 at 09:59:24PM +0200, Thomas Haller wrote:
> "nft --check" will trigger a rollback in kernel. The existing dump files
> might hit new code paths. Take the opportunity to call the command on
> the existing files.
> 
> And alternative would be to write a separate tests, that iterates over
> all files. However, then we can only run all the commands sequentially
> (unless we do something smart). That might be slower than the
> opportunity to run the checks in parallel. More importantly, it would be
> nice if the check for the dump file is clearly tied to the file's test.
> So run it right after the test, from the test wrapper.

I am already using this patch in my tests.

Applied, thanks.