[PATCH RFC] tests: add feature probing

[PATCH RFC] 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>
---
 Not yet fully tested, so RFC tag.

 tests/shell/features/chain_binding.nft        |  5 ++
 tests/shell/run-tests.sh                      | 23 ++++++++
 tests/shell/testcases/transactions/30s-stress | 52 ++++++++++++++++---
 3 files changed, 73 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..eac8b941ab5c
--- /dev/null
+++ b/tests/shell/features/chain_binding.nft
@@ -0,0 +1,5 @@
+table ip t {
+	chain c {
+		jump { counter; }
+	}
+}
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index b66ef4fa4d1f..3855bd9f4768 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -163,6 +163,23 @@ ok=0
 failed=0
 taint=0
 
+check_features()
+{
+	for ffilename in features/*.nft; do
+		feature=${ffilename#*/}
+		feature=${feature%*.nft}
+		eval NFT_HAVE_${feature}=0
+		$NFT -f "$ffilename" 2>/dev/null
+		if [ $? -eq 0 ]; then
+			eval NFT_HAVE_${feature}=1
+		else
+			echo "WARNING: Disabling feature $feature" 1>&2
+		fi
+
+		export NFT_HAVE_${feature}
+	done
+}
+
 check_taint()
 {
 	read taint_now < /proc/sys/kernel/tainted
@@ -211,6 +228,7 @@ check_kmemleak()
 	fi
 }
 
+check_features
 check_taint
 
 for testfile in $(find_tests)
@@ -277,5 +295,10 @@ check_kmemleak_force
 
 msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
 
+if [ "$VERBOSE" == "y" ] ; then
+	echo "Used 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

Re: [PATCH RFC] tests: add feature probing

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> +check_features()
> +{
> +	for ffilename in features/*.nft; do
> +		feature=${ffilename#*/}
> +		feature=${feature%*.nft}
> +		eval NFT_HAVE_${feature}=0
> +		$NFT -f "$ffilename" 2>/dev/null

This is broken if run-tests.sh is not invoked inside
tests/shell, I've fixed this up.

Meanwhile I added probe for netdev-chains-without-device and SKIPPED
support:

I: [OK]         ././testcases/chains/0044chain_destroy_0
I: [SKIPPED]    ././testcases/chains/netdev_chain_0
I: [OK]         ././testcases/comments/comments_0

My plan is to keep adding more probe files locally until ./run-tests.sh
passes on 5.14-patched-kernel and then send the batch.

I've also changed the probe files to contain the upstream patch
and its upstream kernel version as a comment.

Re: [PATCH RFC] tests: add feature probing

[Thomas Haller]

On Thu, 2023-08-31 at 15:51 +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}=$?
> 
> 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>
> ---
>  Not yet fully tested, so RFC tag.
> 
>  tests/shell/features/chain_binding.nft        |  5 ++
>  tests/shell/run-tests.sh                      | 23 ++++++++
>  tests/shell/testcases/transactions/30s-stress | 52 ++++++++++++++++-
> --
>  3 files changed, 73 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..eac8b941ab5c
> --- /dev/null
> +++ b/tests/shell/features/chain_binding.nft
> @@ -0,0 +1,5 @@
> +table ip t {
> +       chain c {
> +               jump { counter; }
> +       }
> +}
> diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
> index b66ef4fa4d1f..3855bd9f4768 100755
> --- a/tests/shell/run-tests.sh
> +++ b/tests/shell/run-tests.sh
> @@ -163,6 +163,23 @@ ok=0
>  failed=0
>  taint=0
>  
> +check_features()
> +{
> +       for ffilename in features/*.nft; do
> +               feature=${ffilename#*/}
> +               feature=${feature%*.nft}
> +               eval NFT_HAVE_${feature}=0
> +               $NFT -f "$ffilename" 2>/dev/null

is the "--check" option here missing? At least, the commit message says

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


> +               if [ $? -eq 0 ]; then
> +                       eval NFT_HAVE_${feature}=1
> +               else
> +                       echo "WARNING: Disabling feature $feature"
> 1>&2
> +               fi
> +
> +               export NFT_HAVE_${feature}

I think it should run in a separate netns too.
With "[PATCH nft v2 0/3] tests/shell: allow running tests as non-root"
patch, that would be `"${UNSHARE[@]}" $NFT ...`



> +       done
> +}
> +
>  check_taint()
>  {
>         read taint_now < /proc/sys/kernel/tainted
> @@ -211,6 +228,7 @@ check_kmemleak()
>         fi
>  }
>  
> +check_features
>  check_taint
>  
>  for testfile in $(find_tests)
> @@ -277,5 +295,10 @@ check_kmemleak_force
>  
>  msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
>  
> +if [ "$VERBOSE" == "y" ] ; then
> +       echo "Used Features:"
> +       env | grep NFT_HAVE_
> +fi


OK, that's nice, to see in the output.

But why this "nft -f" specific detection? Why not just executable
scripts?

Also, why should "run-tests.sh" pre-evaluate those NFT_HAVE_* features?
Just let each tests:

     if ! "$BASEDIR/features/chain-binding" ; then
         echo " defaultchain"
         return
     fi

then the checks are more flexible (arbitrary executables).

Downside, if the check is time consuming (which it shouldn't), then
tests might call it over and over. Workaround for that: have "run-
tests.sh" prepare a temporary directory and export it as
$NFT_TEST_TMPDIR". Then "features/chain-binding" can cache the result
there. "run-tests.sh" would delete the directory afterwards.

If you want to print those features, you can still let run-tests.sh
iterate over "$BASEDIR"/features/* and print the result.



> +
>  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)

I think run-tests.sh should export the base directory, like "$BASEDIR",
or "$NFT_TEST_BASEDIR". Tests should use it (and rely to have it).



> +       $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

ah, you'd have each tests re-implement the check? So that they can run
without the "run-tests.sh" wrapper?

I think that use-case should be dropped. The "run-tests.sh" wrapper can
provide very useful features, and every test reimplementing that is
wrong. Just accept that test scripts should not be run directly, then
drop this [ -z $NFT_HAVE_chain_binding ] check.

Well, above I argue that "run-tests.sh" should not prepare "$NFT_HAVE_"
variables, instead have features-detections plain shell scripts, that
each test runs as needed.

The point here is, that if the "run-tests.sh" wrapper can provide
something useful, then tests should be able to rely on it (and don't
implement a fallback path).


> +
>  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))

Re: [PATCH RFC] tests: add feature probing

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> > +check_features()
> > +{
> > +       for ffilename in features/*.nft; do
> > +               feature=${ffilename#*/}
> > +               feature=${feature%*.nft}
> > +               eval NFT_HAVE_${feature}=0
> > +               $NFT -f "$ffilename" 2>/dev/null
> 
> is the "--check" option here missing? At least, the commit message says

Yes, added.

> I think it should run in a separate netns too.

Should not make a difference due to --check, nothing
is altered.

> OK, that's nice, to see in the output.
> 
> But why this "nft -f" specific detection? Why not just executable
> scripts?

Because I want it to be simple, it will be enough to drop a ruleset
with the feature into features/ durectory and run-tests.sh would do the
reset.

While its possible to launch nft as a script via

 #!/usr/bin/env nft

This would use the system nft, not the newly built one.  And fudging
with $PATH seems ugly...

> Also, why should "run-tests.sh" pre-evaluate those NFT_HAVE_* features?
> Just let each tests:
> 
>      if ! "$BASEDIR/features/chain-binding" ; then
>          echo " defaultchain"
>          return
>      fi
> 
> then the checks are more flexible (arbitrary executables).

I could do that, but I don't see the need for arbitrary scripts so far.

> Downside, if the check is time consuming (which it shouldn't),

Agree, it should not.

> then
> tests might call it over and over. Workaround for that: have "run-
> tests.sh" prepare a temporary directory and export it as
> $NFT_TEST_TMPDIR". Then "features/chain-binding" can cache the result
> there. "run-tests.sh" would delete the directory afterwards.
> 
> If you want to print those features, you can still let run-tests.sh
> iterate over "$BASEDIR"/features/* and print the result.

Yes, thats a working alternative but I don't see the need for this.

> > 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)
> 
> I think run-tests.sh should export the base directory, like "$BASEDIR",
> or "$NFT_TEST_BASEDIR". Tests should use it (and rely to have it).

Can do that.

> ah, you'd have each tests re-implement the check? So that they can run
> without the "run-tests.sh" wrapper?

No, just 30s-stress.  This test is special because its random by nature
and it makes sense to run it for multiple hours once in a while.

> The point here is, that if the "run-tests.sh" wrapper can provide
> something useful, then tests should be able to rely on it (and don't
> implement a fallback path).

Yes, no other script will do this.
I'll send an updated batch soon to see how this looks like with more
feature tests in place.

Re: [PATCH RFC] tests: add feature probing

[Thomas Haller]

On Mon, 2023-09-04 at 10:53 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > 
> > 
> > But why this "nft -f" specific detection? Why not just executable
> > scripts?
> 
> Because I want it to be simple,

It does not seem "simple[r]" to me. The approach requires extra
infrastructure in run-test.sh, while being less flexible.


> I could do that, but I don't see the need for arbitrary scripts so
> far.

When building without JSON support, various tests fail, but should be
skipped.

Could we detect JSON support via .nft files? Would we drop then a JSON
.nft file and change the check call to `nft --check -j`?).

Or maybe detection of JSON support needs to be a shell script (doing
`ldd "$NFT_REAL" | greq libjansson`)? In that case, we would have
features-as-shell-scripts very soon.
   

Thomas

Re: [PATCH RFC] tests: add feature probing

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> On Mon, 2023-09-04 at 10:53 +0200, Florian Westphal wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > > 
> > > 
> > > But why this "nft -f" specific detection? Why not just executable
> > > scripts?
> > 
> > Because I want it to be simple,
> 
> It does not seem "simple[r]" to me. The approach requires extra
> infrastructure in run-test.sh, while being less flexible.

I can add bla.nft and use nft --check -f bla.nft.

Or, I can add bla.sh, which does

exec $NFT -f - <<EOF
table ...
EOF

I see zero reason why we can't add scripts later on if there
are cases where flat-files don't work.

At this point, its just more boilerplate to add a script wrapper
around the .nft file.

> > I could do that, but I don't see the need for arbitrary scripts so
> > far.
> 
> When building without JSON support, various tests fail, but should be
> skipped.
> 
> Could we detect JSON support via .nft files? Would we drop then a JSON
> .nft file and change the check call to `nft --check -j`?).

No, but the test that should be skipped can do

$NFT -j list ruleset || exit 77

as first line of the script, no need to load any files, nft will fail
with error in case its not built with json support.

> Or maybe detection of JSON support needs to be a shell script (doing
> `ldd "$NFT_REAL" | greq libjansson`)? In that case, we would have
> features-as-shell-scripts very soon.

Sure, I see no reason why to not have both.  The flat files have the
'*nft' suffix for a reason...

I'll no longer work on this for the remainder of the month due to
time constraints.

Re: [PATCH RFC] tests: add feature probing

[Pablo Neira Ayuso]

On Wed, Sep 06, 2023 at 12:04:40PM +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > On Mon, 2023-09-04 at 10:53 +0200, Florian Westphal wrote:
> > > Thomas Haller <thaller@redhat.com> wrote:
> > > > 
> > > > 
> > > > But why this "nft -f" specific detection? Why not just executable
> > > > scripts?
> > > 
> > > Because I want it to be simple,
> > 
> > It does not seem "simple[r]" to me. The approach requires extra
> > infrastructure in run-test.sh, while being less flexible.
> 
> I can add bla.nft and use nft --check -f bla.nft.
> 
> Or, I can add bla.sh, which does
> 
> exec $NFT -f - <<EOF
> table ...
> EOF
> 
> I see zero reason why we can't add scripts later on if there
> are cases where flat-files don't work.

Agreed, we need this flexibility.

> At this point, its just more boilerplate to add a script wrapper
> around the .nft file.
> 
> > > I could do that, but I don't see the need for arbitrary scripts so
> > > far.
> > 
> > When building without JSON support, various tests fail, but should be
> > skipped.
> > 
> > Could we detect JSON support via .nft files? Would we drop then a JSON
> > .nft file and change the check call to `nft --check -j`?).
> 
> No, but the test that should be skipped can do
> 
> $NFT -j list ruleset || exit 77
> 
> as first line of the script, no need to load any files, nft will fail
> with error in case its not built with json support.

This is fine to start with.

> > Or maybe detection of JSON support needs to be a shell script (doing
> > `ldd "$NFT_REAL" | greq libjansson`)? In that case, we would have
> > features-as-shell-scripts very soon.
> 
> Sure, I see no reason why to not have both.  The flat files have the
> '*nft' suffix for a reason...

I think this feature approach you propose is good enough and it is
rather incremental and small.

Re: [PATCH RFC] tests: add feature probing

[Thomas Haller]

On Wed, 2023-09-06 at 12:04 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > On Mon, 2023-09-04 at 10:53 +0200, Florian Westphal wrote:
> > > Thomas Haller <thaller@redhat.com> wrote:
> > > > 
> > > > 
> > > > But why this "nft -f" specific detection? Why not just
> > > > executable
> > > > scripts?
> > > 
> > > Because I want it to be simple,
> > 
> > It does not seem "simple[r]" to me. The approach requires extra
> > infrastructure in run-test.sh, while being less flexible.
> 
> I can add bla.nft and use nft --check -f bla.nft.
> 
> Or, I can add bla.sh, which does
> 
> exec $NFT -f - <<EOF
> table ...
> EOF
> 
> I see zero reason why we can't add scripts later on if there
> are cases where flat-files don't work.
> 
> At this point, its just more boilerplate to add a script wrapper
> around the .nft file.
> 
> > > I could do that, but I don't see the need for arbitrary scripts
> > > so
> > > far.
> > 
> > When building without JSON support, various tests fail, but should
> > be
> > skipped.
> > 
> > Could we detect JSON support via .nft files? Would we drop then a
> > JSON
> > .nft file and change the check call to `nft --check -j`?).
> 
> No, but the test that should be skipped can do
> 
> $NFT -j list ruleset || exit 77
> 
> as first line of the script, no need to load any files, nft will fail
> with error in case its not built with json support.
> 
> > Or maybe detection of JSON support needs to be a shell script
> > (doing
> > `ldd "$NFT_REAL" | greq libjansson`)? In that case, we would have
> > features-as-shell-scripts very soon.
> 
> Sure, I see no reason why to not have both.  The flat files have the
> '*nft' suffix for a reason...
> 
> I'll no longer work on this for the remainder of the month due to
> time constraints.
> 


Sounds all good! Thanks.


I go ahead and implement an early version of the "NFT_HAVE_json"
feature. It can later be reconciled with your feature probing patch.


Thomas