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@xxxxxxxxx>
> ---
> 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))
