[PATCH nft 0/5] tests: shell: add and use feature probing

[PATCH nft 0/5] tests: shell: add and use feature probing

[Florian Westphal]

This series allows to run run-tests.sh on the centos-stream-9 kernel,
which is based on 5.14.y, with all tests either passing or getting
skipped as the feature tested isn't available.

Before:
I: results: [OK] 366 [FAILED] 7 [TOTAL] 373

After:
I: results: [OK] 370 [FAILED] 0 [SKIPPED] 3 [TOTAL] 373

First patch adds feature probe skeleton, second patch adds feature
probe for netdev chains without a device.

Third patch alters a few test cases to no longer depend on 'inner header
offset base'.

Patch 4 adds and uses feature probe test for treating maps like sets
and last patch does the same for the inner header base offset.

Florian Westphal (5):
  tests: add feature probing
  tests: shell: let netdev_chain_0 test indicate SKIP if kernel requires
    netdev device
  tests: shell: typeof_integer/raw: prefer @nh for payload matching
  tests: shell: add and use feature probe for map query like a set
  tests: shell skip inner matching tests if unsupported

 tests/shell/features/chain_binding.nft        |  7 +++
 tests/shell/features/inner_matching.nft       |  7 +++
 tests/shell/features/map_lookup.nft           | 11 ++++
 .../features/netdev_chain_without_device.nft  |  7 +++
 tests/shell/run-tests.sh                      | 35 ++++++++++++-
 tests/shell/testcases/chains/netdev_chain_0   |  2 +
 .../testcases/maps/dumps/typeof_integer_0.nft |  4 +-
 .../testcases/maps/dumps/typeof_raw_0.nft     |  4 +-
 tests/shell/testcases/maps/typeof_integer_0   |  4 +-
 .../testcases/maps/typeof_maps_add_delete     | 35 ++++++++++---
 tests/shell/testcases/maps/typeof_raw_0       |  4 +-
 .../testcases/sets/dumps/typeof_raw_0.nft     |  4 +-
 tests/shell/testcases/sets/inner_0            |  2 +
 tests/shell/testcases/sets/typeof_raw_0       |  4 +-
 tests/shell/testcases/transactions/30s-stress | 52 ++++++++++++++++---
 15 files changed, 154 insertions(+), 28 deletions(-)
 create mode 100644 tests/shell/features/chain_binding.nft
 create mode 100644 tests/shell/features/inner_matching.nft
 create mode 100644 tests/shell/features/map_lookup.nft
 create mode 100644 tests/shell/features/netdev_chain_without_device.nft

-- 
2.41.0

[PATCH nft 1/5] tests: add feature probing

[Florian Westphal]

Running selftests on older kernels makes some of them fail very early
because some tests use features that are not available on older
kernels, e.g. -stable releases.

Known examples:
- inner header matching
- anonymous chains
- elem delete from packet path

Also, some test cases might fail because a feature isn't
compiled in, such as netdev chains for example.

This adds a feature-probing to the shell tests.

Simply drop a 'nft -f' compatible file with a .nft suffix into
tests/shell/features.

run-tests.sh will load it via --check and will add

NFT_TESTS_HAVE_${filename}=$?

to the environment.

The test script can then either elide a part of the test
or replace it with a supported feature subset.

This adds the probing skeleton, a probe file for chain_binding
and alters 30s-stress to suppress anonon chains when its unuspported.

Note that 30s-stress is optionally be run standalone, so this adds
more code than needed, for tests that always run via run-tests.sh
its enough to do

[ $NFT_HAVE_chain_binding -eq 1 ] && test_chain_binding

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/features/chain_binding.nft        |  7 +++
 tests/shell/run-tests.sh                      | 24 +++++++++
 tests/shell/testcases/transactions/30s-stress | 52 ++++++++++++++++---
 3 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 tests/shell/features/chain_binding.nft

diff --git a/tests/shell/features/chain_binding.nft b/tests/shell/features/chain_binding.nft
new file mode 100644
index 000000000000..b381ec540fae
--- /dev/null
+++ b/tests/shell/features/chain_binding.nft
@@ -0,0 +1,7 @@
+# d0e2c7de92c7 ("netfilter: nf_tables: add NFT_CHAIN_BINDING")
+# v5.9-rc1~133^2~302^2~1
+table ip t {
+	chain c {
+		jump { counter; }
+	}
+}
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index b66ef4fa4d1f..3113404de2b9 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -163,6 +163,24 @@ ok=0
 failed=0
 taint=0
 
+check_features()
+{
+	for ffilename in $TESTDIR/../features/*.nft; do
+		feature=$(basename $ffilename)
+
+		feature=${feature#*/}
+		feature=${feature%*.nft}
+
+		eval NFT_HAVE_${feature}=0
+		$NFT --check -f "$ffilename" 2>/dev/null
+		if [ $? -eq 0 ]; then
+			eval NFT_HAVE_${feature}=1
+		fi
+
+		export NFT_HAVE_${feature}
+	done
+}
+
 check_taint()
 {
 	read taint_now < /proc/sys/kernel/tainted
@@ -211,6 +229,7 @@ check_kmemleak()
 	fi
 }
 
+check_features
 check_taint
 
 for testfile in $(find_tests)
@@ -277,5 +296,10 @@ check_kmemleak_force
 
 msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
 
+if [ "$VERBOSE" == "y" ] ; then
+	echo "Probed Features:"
+	env | grep NFT_HAVE_
+fi
+
 kernel_cleanup
 [ "$failed" -eq 0 ]
diff --git a/tests/shell/testcases/transactions/30s-stress b/tests/shell/testcases/transactions/30s-stress
index 4d3317e22b0c..924e7e28f97e 100755
--- a/tests/shell/testcases/transactions/30s-stress
+++ b/tests/shell/testcases/transactions/30s-stress
@@ -16,6 +16,18 @@ if [ x = x"$NFT" ] ; then
 	NFT=nft
 fi
 
+if [ -z $NFT_HAVE_chain_binding ];then
+	NFT_HAVE_chain_binding=0
+	mydir=$(dirname $0)
+	$NFT --check -f $mydir/../../features/chain_binding.nft
+	if [ $? -eq 0 ];then
+		NFT_HAVE_chain_binding=1
+	else
+		echo "Assuming anonymous chains are not supported"
+	fi
+
+fi
+
 testns=testns-$(mktemp -u "XXXXXXXX")
 tmp=""
 
@@ -31,8 +43,8 @@ failslab_defaults() {
 	# allow all slabs to fail (if process is tagged).
 	find /sys/kernel/slab/ -wholename '*/kmalloc-[0-9]*/failslab' -type f -exec sh -c 'echo 1 > {}' \;
 
-	# no limit on the number of failures
-	echo -1 > /sys/kernel/debug/failslab/times
+	# no limit on the number of failures, or clause works around old kernels that reject negative integer.
+	echo -1 > /sys/kernel/debug/failslab/times 2>/dev/null || printf '%#x -1' > /sys/kernel/debug/failslab/times
 
 	# Set to 2 for full dmesg traces for each injected error
 	echo 0 > /sys/kernel/debug/failslab/verbose
@@ -91,6 +103,15 @@ nft_with_fault_inject()
 trap cleanup EXIT
 tmp=$(mktemp)
 
+jump_or_goto()
+{
+	if [ $((RANDOM & 1)) -eq 0 ] ;then
+		echo -n "jump"
+	else
+		echo -n "goto"
+	fi
+}
+
 random_verdict()
 {
 	max="$1"
@@ -102,7 +123,8 @@ random_verdict()
 	rnd=$((RANDOM%max))
 
 	if [ $rnd -gt 0 ];then
-		printf "jump chain%03u" "$((rnd+1))"
+		jump_or_goto
+		printf " chain%03u" "$((rnd+1))"
 		return
 	fi
 
@@ -411,6 +433,21 @@ stress_all()
 	randmonitor &
 }
 
+gen_anon_chain_jump()
+{
+	echo -n "insert rule inet $@ "
+	jump_or_goto
+
+	if [ $NFT_HAVE_chain_binding -ne 1 ];then
+		echo " defaultchain"
+		return
+	fi
+
+	echo -n " { "
+	jump_or_goto
+	echo " defaultchain; counter; }"
+}
+
 gen_ruleset() {
 echo > "$tmp"
 for table in $tables; do
@@ -452,12 +489,13 @@ for table in $tables; do
 	echo "insert rule inet $table $chain ip6 saddr { ::1, dead::beef } counter" comment hash >> "$tmp"
 	echo "insert rule inet $table $chain ip saddr { 1.2.3.4 - 5.6.7.8, 127.0.0.1 } comment rbtree" >> "$tmp"
 	# bitmap 1byte, with anon chain jump
-	echo "insert rule inet $table $chain ip protocol { 6, 17 } jump { jump defaultchain; counter; }" >> "$tmp"
+	gen_anon_chain_jump "$table $chain ip protocol { 6, 17 }" >> "$tmp"
+
 	# bitmap 2byte
 	echo "insert rule inet $table $chain tcp dport != { 22, 23, 80 } goto defaultchain" >> "$tmp"
 	echo "insert rule inet $table $chain tcp dport { 1-1024, 8000-8080 } jump defaultchain comment rbtree" >> "$tmp"
 	# pipapo (concat + set), with goto anonymous chain.
-	echo "insert rule inet $table $chain ip saddr . tcp dport { 1.2.3.4 . 1-1024, 1.2.3.6 - 1.2.3.10 . 8000-8080, 1.2.3.4 . 8080, 1.2.3.6 - 1.2.3.10 . 22 } goto { jump defaultchain; counter; }" >> "$tmp"
+	gen_anon_chain_jump "$table $chain ip saddr . tcp dport { 1.2.3.4 . 1-1024, 1.2.3.6 - 1.2.3.10 . 8000-8080, 1.2.3.4 . 8080, 1.2.3.6 - 1.2.3.10 . 22 }" >> "$tmp"
 
 	# add a few anonymous sets. rhashtable is convered by named sets below.
 	c=$((RANDOM%$count))
@@ -466,12 +504,12 @@ for table in $tables; do
 	echo "insert rule inet $table $chain ip6 saddr { ::1, dead::beef } counter" comment hash >> "$tmp"
 	echo "insert rule inet $table $chain ip saddr { 1.2.3.4 - 5.6.7.8, 127.0.0.1 } comment rbtree" >> "$tmp"
 	# bitmap 1byte, with anon chain jump
-	echo "insert rule inet $table $chain ip protocol { 6, 17 } jump { jump defaultchain; counter; }" >> "$tmp"
+	gen_anon_chain_jump "$table $chain ip protocol { 6, 17 }" >> "$tmp"
 	# bitmap 2byte
 	echo "insert rule inet $table $chain tcp dport != { 22, 23, 80 } goto defaultchain" >> "$tmp"
 	echo "insert rule inet $table $chain tcp dport { 1-1024, 8000-8080 } jump defaultchain comment rbtree" >> "$tmp"
 	# pipapo (concat + set), with goto anonymous chain.
-	echo "insert rule inet $table $chain ip saddr . tcp dport { 1.2.3.4 . 1-1024, 1.2.3.6 - 1.2.3.10 . 8000-8080, 1.2.3.4 . 8080, 1.2.3.6 - 1.2.3.10 . 22 } goto { jump defaultchain; counter; }" >> "$tmp"
+	gen_anon_chain_jump "$table $chain ip saddr . tcp dport { 1.2.3.4 . 1-1024, 1.2.3.6 - 1.2.3.10 . 8000-8080, 1.2.3.4 . 8080, 1.2.3.6 - 1.2.3.10 . 22 }" >> "$tmp"
 
 	# add constant/immutable sets
 	size=$((RANDOM%5120000))
-- 
2.41.0

[PATCH nft 2/5] tests: shell: let netdev_chain_0 test indicate SKIP if kernel requires netdev device

[Florian Westphal]

This test case only works on kernel 6.4+.
Add feature probe for this and then exit early.

We don't want to indicate a test failure, as this test doesn't apply
on older kernels.

But we should not indicate sucess either, else we might be fooled
in case something went wrong during feature probe.

Add a special return value, 123, and let run-tests.sh count this
as 'SKIPPED'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/features/netdev_chain_without_device.nft |  7 +++++++
 tests/shell/run-tests.sh                             | 11 ++++++++++-
 tests/shell/testcases/chains/netdev_chain_0          |  2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/features/netdev_chain_without_device.nft

diff --git a/tests/shell/features/netdev_chain_without_device.nft b/tests/shell/features/netdev_chain_without_device.nft
new file mode 100644
index 000000000000..25eb200ffe31
--- /dev/null
+++ b/tests/shell/features/netdev_chain_without_device.nft
@@ -0,0 +1,7 @@
+# 207296f1a03b ("netfilter: nf_tables: allow to create netdev chain without device")
+# v6.4-rc1~132^2~14^2
+table netdev t {
+	chain c {
+		type filter hook ingress priority 0; policy accept;
+        }
+}
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 3113404de2b9..17cab3f11c9b 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -161,6 +161,7 @@ fi
 echo ""
 ok=0
 failed=0
+skipped=0
 taint=0
 
 check_features()
@@ -270,6 +271,9 @@ do
 				msg_warn "[DUMP FAIL]	$testfile"
 			fi
 		fi
+	elif [ "$rc_got" -eq 123 ]; then
+		((skipped++))
+		msg_info "[SKIPPED]	$testfile"
 	else
 		((failed++))
 		if [ "$VERBOSE" == "y" ] ; then
@@ -294,7 +298,12 @@ echo ""
 kmemleak_found=0
 check_kmemleak_force
 
-msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
+msg_info "results: [OK] $ok [FAILED] $failed [SKIPPED] $skipped [TOTAL] $((ok+failed+skipped))"
+
+if [ $ok -eq 0 -a  $failed -eq 0 ]; then
+	# no test cases were run, indicate a failure
+	failed=1
+fi
 
 if [ "$VERBOSE" == "y" ] ; then
 	echo "Probed Features:"
diff --git a/tests/shell/testcases/chains/netdev_chain_0 b/tests/shell/testcases/chains/netdev_chain_0
index 67cd715fc59f..2e2a0c177fcb 100755
--- a/tests/shell/testcases/chains/netdev_chain_0
+++ b/tests/shell/testcases/chains/netdev_chain_0
@@ -1,5 +1,7 @@
 #!/bin/bash
 
+[ "$NFT_HAVE_netdev_chain_without_device" -eq 0 ] && exit 123
+
 ip link add d0 type dummy || {
         echo "Skipping, no dummy interface available"
         exit 0
-- 
2.41.0

[PATCH nft 3/5] tests: shell: typeof_integer/raw: prefer @nh for payload matching

[Florian Westphal]

@ih fails on kernels where payload expression doesn't support the 'inner'
base offset.

This test isn't about inner headers, so just use @nh which is
universally available.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/testcases/maps/dumps/typeof_integer_0.nft | 4 ++--
 tests/shell/testcases/maps/dumps/typeof_raw_0.nft     | 4 ++--
 tests/shell/testcases/maps/typeof_integer_0           | 4 ++--
 tests/shell/testcases/maps/typeof_raw_0               | 4 ++--
 tests/shell/testcases/sets/dumps/typeof_raw_0.nft     | 4 ++--
 tests/shell/testcases/sets/typeof_raw_0               | 4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/shell/testcases/maps/dumps/typeof_integer_0.nft b/tests/shell/testcases/maps/dumps/typeof_integer_0.nft
index 330415574c95..19c24febffcc 100644
--- a/tests/shell/testcases/maps/dumps/typeof_integer_0.nft
+++ b/tests/shell/testcases/maps/dumps/typeof_integer_0.nft
@@ -13,8 +13,8 @@ table inet t {
 	}
 
 	chain c {
-		udp length . @ih,32,32 vmap @m1
-		udp length . @ih,32,32 vmap @m2
+		udp length . @nh,32,32 vmap @m1
+		udp length . @nh,32,32 vmap @m2
 		udp length . @th,160,128 vmap { 47-63 . 0xe373135363130333131303735353203 : accept }
 	}
 }
diff --git a/tests/shell/testcases/maps/dumps/typeof_raw_0.nft b/tests/shell/testcases/maps/dumps/typeof_raw_0.nft
index e876425b2bc6..476169f2943b 100644
--- a/tests/shell/testcases/maps/dumps/typeof_raw_0.nft
+++ b/tests/shell/testcases/maps/dumps/typeof_raw_0.nft
@@ -7,7 +7,7 @@ table ip x {
 	}
 
 	chain y {
-		ip saddr . @ih,32,32 vmap @y
-		ip saddr . @ih,32,32 vmap { 4.4.4.4 . 0x34 : accept, 5.5.5.5 . 0x45 : drop }
+		ip saddr . @nh,32,32 vmap @y
+		ip saddr . @nh,32,32 vmap { 4.4.4.4 . 0x34 : accept, 5.5.5.5 . 0x45 : drop }
 	}
 }
diff --git a/tests/shell/testcases/maps/typeof_integer_0 b/tests/shell/testcases/maps/typeof_integer_0
index d51510af9073..0deff5eef67b 100755
--- a/tests/shell/testcases/maps/typeof_integer_0
+++ b/tests/shell/testcases/maps/typeof_integer_0
@@ -13,8 +13,8 @@ EXPECTED="table inet t {
 	}
 
 	chain c {
-		udp length . @ih,32,32 vmap @m1
-		udp length . @ih,32,32 vmap @m2
+		udp length . @nh,32,32 vmap @m1
+		udp length . @nh,32,32 vmap @m2
 		udp length . @th,160,128 vmap { 47-63 . 0xe373135363130333131303735353203 : accept }
 	}
 }"
diff --git a/tests/shell/testcases/maps/typeof_raw_0 b/tests/shell/testcases/maps/typeof_raw_0
index e3da7825cb7b..bcd2c6d8c502 100755
--- a/tests/shell/testcases/maps/typeof_raw_0
+++ b/tests/shell/testcases/maps/typeof_raw_0
@@ -7,8 +7,8 @@ EXPECTED="table ip x {
 	}
 
 	chain y {
-		ip saddr . @ih,32,32 vmap @y
-		ip saddr . @ih,32,32 vmap { 4.4.4.4 . 0x34 : accept, 5.5.5.5 . 0x45 : drop}
+		ip saddr . @nh,32,32 vmap @y
+		ip saddr . @nh,32,32 vmap { 4.4.4.4 . 0x34 : accept, 5.5.5.5 . 0x45 : drop}
 	}
 }"
 
diff --git a/tests/shell/testcases/sets/dumps/typeof_raw_0.nft b/tests/shell/testcases/sets/dumps/typeof_raw_0.nft
index 499ff167f51d..4d6abaaa151b 100644
--- a/tests/shell/testcases/sets/dumps/typeof_raw_0.nft
+++ b/tests/shell/testcases/sets/dumps/typeof_raw_0.nft
@@ -6,7 +6,7 @@ table inet t {
 	}
 
 	chain y {
-		ip saddr . @ih,32,32 { 1.1.1.1 . 0x14, 2.2.2.2 . 0x1e }
-		ip daddr . @ih,32,32 @y
+		ip saddr . @nh,32,32 { 1.1.1.1 . 0x14, 2.2.2.2 . 0x1e }
+		ip daddr . @nh,32,32 @y
 	}
 }
diff --git a/tests/shell/testcases/sets/typeof_raw_0 b/tests/shell/testcases/sets/typeof_raw_0
index 36396b5c2e1d..66042eb4085a 100755
--- a/tests/shell/testcases/sets/typeof_raw_0
+++ b/tests/shell/testcases/sets/typeof_raw_0
@@ -7,8 +7,8 @@ EXPECTED="table inet t {
 	}
 
 	chain y {
-		ip saddr . @ih,32,32 { 1.1.1.1 . 0x14, 2.2.2.2 . 0x1e }
-		ip daddr . @ih,32,32 @y
+		ip saddr . @nh,32,32 { 1.1.1.1 . 0x14, 2.2.2.2 . 0x1e }
+		ip daddr . @nh,32,32 @y
 	}
 }"
 
-- 
2.41.0

[PATCH nft 4/5] tests: shell: add and use feature probe for map query like a set

[Florian Westphal]

On recent kernels one can perform a lookup in a map without a destination
register (i.e., treat the map like a set -- pure existence check).

Add a feature probe and work around the missing feature in
typeof_maps_add_delete: do the test with a simplified ruleset,

Indicate skipped even though a reduced test was run (earlier errors
cause a failure) to not trigger dump validation error.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/features/map_lookup.nft           | 11 ++++++
 .../testcases/maps/typeof_maps_add_delete     | 35 ++++++++++++++-----
 2 files changed, 38 insertions(+), 8 deletions(-)
 create mode 100644 tests/shell/features/map_lookup.nft

diff --git a/tests/shell/features/map_lookup.nft b/tests/shell/features/map_lookup.nft
new file mode 100644
index 000000000000..06c4c9d9c82d
--- /dev/null
+++ b/tests/shell/features/map_lookup.nft
@@ -0,0 +1,11 @@
+# a4878eeae390 ("netfilter: nf_tables: relax set/map validation checks")
+# v6.5-rc1~163^2~256^2~8
+table ip t {
+        map m {
+                typeof ip daddr : meta mark
+        }
+
+        chain c {
+                ip saddr @m
+        }
+}
diff --git a/tests/shell/testcases/maps/typeof_maps_add_delete b/tests/shell/testcases/maps/typeof_maps_add_delete
index 341de538e90e..579194b03372 100755
--- a/tests/shell/testcases/maps/typeof_maps_add_delete
+++ b/tests/shell/testcases/maps/typeof_maps_add_delete
@@ -1,6 +1,15 @@
 #!/bin/bash
 
-EXPECTED='table ip dynset {
+CONDMATCH="ip saddr @dynmark"
+NCONDMATCH="ip saddr != @dynmark"
+
+# use reduced feature set
+if [ $NFT_HAVE_map_lookup -eq 0 ] ;then
+	CONDMATCH=""
+	NCONDMATCH=""
+fi
+
+EXPECTED="table ip dynset {
 	map dynmark {
 		typeof ip daddr : meta mark
 		counter
@@ -9,20 +18,20 @@ EXPECTED='table ip dynset {
 	}
 
 	chain test_ping {
-		ip saddr @dynmark counter comment "should not increment"
-		ip saddr != @dynmark add @dynmark { ip saddr : 0x1 } counter
-		ip saddr @dynmark counter comment "should increment"
-		ip saddr @dynmark delete @dynmark { ip saddr : 0x1 }
-		ip saddr @dynmark counter comment "delete should be instant but might fail under memory pressure"
+		$CONDMATCH counter comment \"should not increment\"
+		$NCONDMATCH add @dynmark { ip saddr : 0x1 } counter
+		$CONDMATCH counter comment \"should increment\"
+		$CONDMATCH delete @dynmark { ip saddr : 0x1 }
+		$CONDMATCH counter comment \"delete should be instant but might fail under memory pressure\"
 	}
 
 	chain input {
 		type filter hook input priority 0; policy accept;
 
-		add @dynmark { 10.2.3.4 timeout 1s : 0x2 } comment "also check timeout-gc"
+		add @dynmark { 10.2.3.4 timeout 1s : 0x2 } comment \"also check timeout-gc\"
 		meta l4proto icmp ip daddr 127.0.0.42 jump test_ping
 	}
-}'
+}"
 
 set -e
 $NFT -f - <<< $EXPECTED
@@ -31,5 +40,15 @@ $NFT list ruleset
 ip link set lo up
 ping -c 1 127.0.0.42
 
+$NFT get element ip dynset dynmark { 10.2.3.4 }
+
 # wait so that 10.2.3.4 times out.
 sleep 2
+
+set +e
+$NFT get element ip dynset dynmark { 10.2.3.4 } && exit 1
+
+# success, but indicate skip for reduced test to avoid dump validation error
+if [ $NFT_HAVE_map_lookup -eq 0 ];then
+	exit 123
+fi
-- 
2.41.0

[PATCH nft 5/5] tests: shell skip inner matching tests if unsupported

[Florian Westphal]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/features/inner_matching.nft | 7 +++++++
 tests/shell/testcases/sets/inner_0      | 2 ++
 2 files changed, 9 insertions(+)
 create mode 100644 tests/shell/features/inner_matching.nft

diff --git a/tests/shell/features/inner_matching.nft b/tests/shell/features/inner_matching.nft
new file mode 100644
index 000000000000..6c86fd3558ac
--- /dev/null
+++ b/tests/shell/features/inner_matching.nft
@@ -0,0 +1,7 @@
+# 3a07327d10a0 ("netfilter: nft_inner: support for inner tunnel header matching")
+# v6.2-rc1~99^2~350^2~4
+table ip t {
+        chain c {
+                udp dport 4789 vxlan ip saddr 1.2.3.4
+        }
+}
diff --git a/tests/shell/testcases/sets/inner_0 b/tests/shell/testcases/sets/inner_0
index 0eb172a8cf06..ceb35115cc7d 100755
--- a/tests/shell/testcases/sets/inner_0
+++ b/tests/shell/testcases/sets/inner_0
@@ -1,5 +1,7 @@
 #!/bin/bash
 
+[ $NFT_HAVE_inner_matching -eq 0 ] && exit 123
+
 set -e
 
 RULESET="table netdev x {
-- 
2.41.0

Re: [PATCH nft 1/5] tests: add feature probing

[Phil Sutter]

On Mon, Sep 04, 2023 at 11:06:30AM +0200, Florian Westphal wrote:
> Running selftests on older kernels makes some of them fail very early
> because some tests use features that are not available on older
> kernels, e.g. -stable releases.
> 
> Known examples:
> - inner header matching
> - anonymous chains
> - elem delete from packet path
> 
> Also, some test cases might fail because a feature isn't
> compiled in, such as netdev chains for example.
> 
> This adds a feature-probing to the shell tests.
> 
> Simply drop a 'nft -f' compatible file with a .nft suffix into
> tests/shell/features.
> 
> run-tests.sh will load it via --check and will add
> 
> NFT_TESTS_HAVE_${filename}=$?

Maybe make this:

| truefalse=(true false)
| NFT_TESTS_HAVE_${filename}=${truefalse[$?]}

[...]

> [ $NFT_HAVE_chain_binding -eq 1 ] && test_chain_binding

So this becomes:

| $NFT_HAVE_chain_binding && test_chain_binding

Use of true/false appears to work in dash, so might be POSIX sh
compatible?

Cheers, Phil

Re: [PATCH nft 2/5] tests: shell: let netdev_chain_0 test indicate SKIP if kernel requires netdev device

[Phil Sutter]

On Mon, Sep 04, 2023 at 11:06:31AM +0200, Florian Westphal wrote:
> This test case only works on kernel 6.4+.
> Add feature probe for this and then exit early.
> 
> We don't want to indicate a test failure, as this test doesn't apply
> on older kernels.
> 
> But we should not indicate sucess either, else we might be fooled
> in case something went wrong during feature probe.
> 
> Add a special return value, 123, and let run-tests.sh count this
> as 'SKIPPED'.

I suggest we adhere to Gnu automake convention:

"[...] an exit status of 0
from a test script will denote a success, an exit status of 77 a skipped
test, an exit status of 99 a hard error, and any other exit status will
denote a failure."[1]

Cheers, Phil

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

Re: [PATCH nft 1/5] tests: add feature probing

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Sep 04, 2023 at 11:06:30AM +0200, Florian Westphal wrote:
> > Running selftests on older kernels makes some of them fail very early
> > because some tests use features that are not available on older
> > kernels, e.g. -stable releases.
> > 
> > Known examples:
> > - inner header matching
> > - anonymous chains
> > - elem delete from packet path
> > 
> > Also, some test cases might fail because a feature isn't
> > compiled in, such as netdev chains for example.
> > 
> > This adds a feature-probing to the shell tests.
> > 
> > Simply drop a 'nft -f' compatible file with a .nft suffix into
> > tests/shell/features.
> > 
> > run-tests.sh will load it via --check and will add
> > 
> > NFT_TESTS_HAVE_${filename}=$?
> 
> Maybe make this:
> 
> | truefalse=(true false)
> | NFT_TESTS_HAVE_${filename}=${truefalse[$?]}
> 
> [...]
> 
> > [ $NFT_HAVE_chain_binding -eq 1 ] && test_chain_binding
> 
> So this becomes:
> 
> | $NFT_HAVE_chain_binding && test_chain_binding
> 
> Use of true/false appears to work in dash, so might be POSIX sh
> compatible?

Can do that, but if [ false ] evaluates to true...

Re: [PATCH nft 1/5] tests: add feature probing

[Phil Sutter]

On Tue, Sep 05, 2023 at 03:44:06PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Mon, Sep 04, 2023 at 11:06:30AM +0200, Florian Westphal wrote:
> > > Running selftests on older kernels makes some of them fail very early
> > > because some tests use features that are not available on older
> > > kernels, e.g. -stable releases.
> > > 
> > > Known examples:
> > > - inner header matching
> > > - anonymous chains
> > > - elem delete from packet path
> > > 
> > > Also, some test cases might fail because a feature isn't
> > > compiled in, such as netdev chains for example.
> > > 
> > > This adds a feature-probing to the shell tests.
> > > 
> > > Simply drop a 'nft -f' compatible file with a .nft suffix into
> > > tests/shell/features.
> > > 
> > > run-tests.sh will load it via --check and will add
> > > 
> > > NFT_TESTS_HAVE_${filename}=$?
> > 
> > Maybe make this:
> > 
> > | truefalse=(true false)
> > | NFT_TESTS_HAVE_${filename}=${truefalse[$?]}
> > 
> > [...]
> > 
> > > [ $NFT_HAVE_chain_binding -eq 1 ] && test_chain_binding
> > 
> > So this becomes:
> > 
> > | $NFT_HAVE_chain_binding && test_chain_binding
> > 
> > Use of true/false appears to work in dash, so might be POSIX sh
> > compatible?
> 
> Can do that, but if [ false ] evaluates to true...

Sure, because that's a short-cut for '[ -n false ]'. In what context is
that problematic?

Re: [PATCH nft 1/5] tests: add feature probing

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Sure, because that's a short-cut for '[ -n false ]'. In what context is
> that problematic?

if [ $HAVE_NFT_foo ] ; then ...

Re: [PATCH nft 1/5] tests: add feature probing

[Phil Sutter]

On Tue, Sep 05, 2023 at 04:09:20PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Sure, because that's a short-cut for '[ -n false ]'. In what context is
> > that problematic?
> 
> if [ $HAVE_NFT_foo ] ; then ...

Same as if $HAVE_NFT_foo was either 0 or 1?! Obviously, with variables
holding the string "true" or "false", one has to test them either via:

| if $var; then ...

or

| if [ $var == true ]; then ...

I just find code more straightforward which does "if $have_foo; then
..." instead of "if [ $have_foo -ne 1 ]; then ...". The latter makes me
question whether that 1 is positive (as with C) or negative (as with
shell) and whether there are more possible values than two and any but 1
are OK.

Cheers, Phil

Re: [PATCH nft 1/5] tests: add feature probing

[Thomas Haller]

On Tue, 2023-09-05 at 15:00 +0200, Phil Sutter wrote:
> On Mon, Sep 04, 2023 at 11:06:30AM +0200, Florian Westphal wrote:
> > Running selftests on older kernels makes some of them fail very
> > early
> > because some tests use features that are not available on older
> > kernels, e.g. -stable releases.
> > 
> > Known examples:
> > - inner header matching
> > - anonymous chains
> > - elem delete from packet path
> > 
> > Also, some test cases might fail because a feature isn't
> > compiled in, such as netdev chains for example.
> > 
> > This adds a feature-probing to the shell tests.
> > 
> > Simply drop a 'nft -f' compatible file with a .nft suffix into
> > tests/shell/features.
> > 
> > run-tests.sh will load it via --check and will add
> > 
> > NFT_TESTS_HAVE_${filename}=$?
> 
> Maybe make this:
> 
> > truefalse=(true false)
> > NFT_TESTS_HAVE_${filename}=${truefalse[$?]}
> 
> [...]
> 
> > [ $NFT_HAVE_chain_binding -eq 1 ] && test_chain_binding
> 
> So this becomes:
> 
> > $NFT_HAVE_chain_binding && test_chain_binding
> 
> Use of true/false appears to work in dash, so might be POSIX sh
> compatible?
> 
> Cheers, Phil
> 


That's possible. But I would not do such non-obvious magic.

The existing variables like VERBOSE,VALGRIND,DUMPGEN use "y" for true
and everything else is false. I'd stick with that form.

The form would be (exactly):

if [ "$NFT_TESTS_HAVE_chain_binding" = y ] ; then


Btw, as a matter of principle, I think that all variables in the patch
set should be quoted.


Thomas

Re: [PATCH nft 2/5] tests: shell: let netdev_chain_0 test indicate SKIP if kernel requires netdev device

[Thomas Haller]

On Mon, 2023-09-04 at 11:06 +0200, Florian Westphal wrote:
> This test case only works on kernel 6.4+.
> Add feature probe for this and then exit early.
> 
> We don't want to indicate a test failure, as this test doesn't apply
> on older kernels.
> 
> But we should not indicate sucess either, else we might be fooled
> in case something went wrong during feature probe.
> 
> Add a special return value, 123, and let run-tests.sh count this
> as 'SKIPPED'.
> 

[...]

>  failed=0
> +skipped=0
>  taint=0
>  
>  check_features()
> @@ -270,6 +271,9 @@ do
>                                 msg_warn "[DUMP FAIL]   $testfile"
>                         fi
>                 fi
> +       elif [ "$rc_got" -eq 123 ]; then
> +               ((skipped++))
> +               msg_info "[SKIPPED]     $testfile"

I agree with Phil, I think this should return 77.

Btw, I did a similar patch on 

  [PATCH nft v5 08/19] tests/shell: interpret an exit code of 77 from scripts as "skipped"

Granted, you send your first version with this patch/idea a few hours
before mine. I just point out the overlap...


>         else
>                 ((failed++))
>                 if [ "$VERBOSE" == "y" ] ; then
> @@ -294,7 +298,12 @@ echo ""
>  kmemleak_found=0
>  check_kmemleak_force
>  
> -msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
> +msg_info "results: [OK] $ok [FAILED] $failed [SKIPPED] $skipped
> [TOTAL] $((ok+failed+skipped))"
> +
> +if [ $ok -eq 0 -a  $failed -eq 0 ]; then
> +       # no test cases were run, indicate a failure
> +       failed=1
> +fi

I think this should be dropped.

When I run

  ./tests/shell/run-tests.sh tests/shell/testcases/maps/typeof_maps_add_delete

on my Fedora38, the test gets skipped. But I think the command should
just give me a success. What would I do about the "failure" anyway?


Thomas

Re: [PATCH nft 1/5] tests: add feature probing

[Thomas Haller]

On Mon, 2023-09-04 at 11:06 +0200, Florian Westphal wrote:
> Running selftests on older kernels makes some of them fail very early
> because some tests use features that are not available on older
> kernels, e.g. -stable releases.
> 
> Known examples:
> - inner header matching
> - anonymous chains
> - elem delete from packet path
> 
> Also, some test cases might fail because a feature isn't
> compiled in, such as netdev chains for example.
> 
> This adds a feature-probing to the shell tests.

On my Fedora 38, I have currently 7 tests failing.
With this patchset, tests/shell/testcases/maps/typeof_maps_add_delete
is now skipped (6 failing left).

Nice!


> +check_features()
> +{
> +       for ffilename in $TESTDIR/../features/*.nft; do
> +               feature=$(basename $ffilename)
> +
> +               feature=${feature#*/}
> +               feature=${feature%*.nft}
> +
> +               eval NFT_HAVE_${feature}=0
> +               $NFT --check -f "$ffilename" 2>/dev/null
> +               if [ $? -eq 0 ]; then
> +                       eval NFT_HAVE_${feature}=1

the existing variables like VERBOSE,VALGRIND use "y" for true and
everything else is false.

I think 0|1 looks better. But it should be consistent. If 0|1 is used,
the other variables should be adjusted.

Note that on the other branch I added normalization functions
bool_y()/bool_n() to accept values like true/1/y/yes from the user, and
normalize to y|n. This could be changed internally to 1|0 without
breaking user setups.


>  for testfile in $(find_tests)
> @@ -277,5 +296,10 @@ check_kmemleak_force
>  
>  msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
>  
> +if [ "$VERBOSE" == "y" ] ; then
> +       echo "Probed Features:"
> +       env | grep NFT_HAVE_
> +fi


  xxx=$'\nNFT_HAVE_XXXXX=bogus' ./tests/shell/run-tests.sh /bin/true -v

gives the wrong output. Could be instead:

  for v in $(compgen -v | grep '^NFT_HAVE_') ; do
      echo "$v=${!v}";
  done

Re: [PATCH nft 4/5] tests: shell: add and use feature probe for map query like a set

[Thomas Haller]

On Mon, 2023-09-04 at 11:06 +0200, Florian Westphal wrote:
> 
> +set +e
> +$NFT get element ip dynset dynmark { 10.2.3.4 } && exit 1
> +
> +# success, but indicate skip for reduced test to avoid dump
> validation error
> +if [ $NFT_HAVE_map_lookup -eq 0 ];then
> +       exit 123
> +fi

Instead of adding a comment, print a message with the reason why it was
skipped. That's useful information to see in the test output.
The `echo` line also makes the comment redundant.

Thomas