[PATCH nft] tests: shell: don't rely on writable test directory

[PATCH nft] tests: shell: don't rely on writable test directory

[Florian Westphal]

Running shell tests from a virtme-ng instance with ro mapped test dir
hangs due to runaway 'awk' reading from stdin instead of the intended
$tmpfile (variable is empty).

Some tests want to check relative includes and try to create temporary
files in the current directory.

[ -w ! $foo ... doesn't catch the error due to missing "".
Add quotes and return the skip retval so those tests get flagged as
skipped.

It would be better to resolve this by creating all temporary
files in /tmp, but this is more intrusive change.

0013input_descriptors_included_files_0 and 0020include_chain_0 are
switched to normal tmpfiles, there is nothing in the test that needs
relative includes.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/shell/testcases/include/0002relative_0  | 13 ++++----
 .../0013input_descriptors_included_files_0    | 30 +++++++++----------
 .../testcases/include/0020include_chain_0     |  9 +++---
 3 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/tests/shell/testcases/include/0002relative_0 b/tests/shell/testcases/include/0002relative_0
index a91cd8f00047..30f4bbdbff79 100755
--- a/tests/shell/testcases/include/0002relative_0
+++ b/tests/shell/testcases/include/0002relative_0
@@ -1,21 +1,20 @@
 #!/bin/bash
 
-set -e
-
 tmpfile1=$(mktemp -p .)
-if [ ! -w $tmpfile1 ] ; then
+if [ ! -w "$tmpfile1" ] ; then
         echo "Failed to create tmp file" >&2
-        exit 0
+        exit 77
 fi
 
+trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
+set -e
+
 tmpfile2=$(mktemp -p .)
-if [ ! -w $tmpfile2 ] ; then
+if [ ! -w "$tmpfile2" ] ; then
         echo "Failed to create tmp file" >&2
         exit 0
 fi
 
-trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
-
 RULESET1="add table x"
 RULESET2="include \"$tmpfile1\""
 
diff --git a/tests/shell/testcases/include/0013input_descriptors_included_files_0 b/tests/shell/testcases/include/0013input_descriptors_included_files_0
index 03de50b3c499..9dc6615dd332 100755
--- a/tests/shell/testcases/include/0013input_descriptors_included_files_0
+++ b/tests/shell/testcases/include/0013input_descriptors_included_files_0
@@ -7,32 +7,32 @@
 # instead of return value of nft.
 
 
-tmpfile1=$(mktemp -p .)
-if [ ! -w $tmpfile1 ] ; then
+tmpfile1=$(mktemp)
+if [ ! -w "$tmpfile1" ] ; then
         echo "Failed to create tmp file" >&2
-        exit 0
+        exit 77
 fi
 
-tmpfile2=$(mktemp -p .)
-if [ ! -w $tmpfile2 ] ; then
+trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 $tmpfile4" EXIT # cleanup if aborted
+
+tmpfile2=$(mktemp)
+if [ ! -w "$tmpfile2" ] ; then
         echo "Failed to create tmp file" >&2
-        exit 0
+        exit 77
 fi
 
-tmpfile3=$(mktemp -p .)
-if [ ! -w $tmpfile3 ] ; then
+tmpfile3=$(mktemp)
+if [ ! -w "$tmpfile3" ] ; then
         echo "Failed to create tmp file" >&2
-        exit 0
+        exit 77
 fi
 
-tmpfile4=$(mktemp -p .)
-if [ ! -w $tmpfile4 ]; then
+tmpfile4=$(mktemp)
+if [ ! -w "$tmpfile4" ]; then
         echo "Failed to create tmp file" >&2
-        exit 0
+        exit 77
 fi
 
-trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 $tmpfile4" EXIT # cleanup if aborted
-
 RULESET1="include \"$tmpfile2\""
 RULESET2="include \"$tmpfile3\""
 RULESET3="add rule x y anything everything"			# wrong nft syntax
@@ -44,7 +44,7 @@ echo "$RULESET3" > $tmpfile2
 
 $NFT -f $tmpfile1 2> $tmpfile4
 
-var=$(awk -F: '$4==" Error"{print $1;exit;}' $tmpfile4)
+var=$(awk -F: '$4==" Error"{print $1;exit;}' "$tmpfile4")
 
 if [ $var == "$tmpfile3" ]; then
 	echo "E: Test failed" >&2
diff --git a/tests/shell/testcases/include/0020include_chain_0 b/tests/shell/testcases/include/0020include_chain_0
index 49b6f76c6a8d..a09511974e29 100755
--- a/tests/shell/testcases/include/0020include_chain_0
+++ b/tests/shell/testcases/include/0020include_chain_0
@@ -1,13 +1,12 @@
 #!/bin/bash
 
-set -e
-
-tmpfile1=$(mktemp -p .)
-if [ ! -w $tmpfile1 ] ; then
+tmpfile1=$(mktemp)
+if [ ! -w "$tmpfile1" ] ; then
 	echo "Failed to create tmp file" >&2
-	exit 0
+	exit 77
 fi
 
+set -e
 trap "rm -rf $tmpfile1" EXIT # cleanup if aborted
 
 RULESET="table inet filter { }
-- 
2.45.2

Re: [PATCH nft] tests: shell: don't rely on writable test directory

[Pablo Neira Ayuso]

On Tue, Oct 22, 2024 at 04:09:54PM +0200, Florian Westphal wrote:
> Running shell tests from a virtme-ng instance with ro mapped test dir
> hangs due to runaway 'awk' reading from stdin instead of the intended
> $tmpfile (variable is empty).
> 
> Some tests want to check relative includes and try to create temporary
> files in the current directory.
> 
> [ -w ! $foo ... doesn't catch the error due to missing "".
> Add quotes and return the skip retval so those tests get flagged as
> skipped.
> 
> It would be better to resolve this by creating all temporary
> files in /tmp, but this is more intrusive change.
>
> 0013input_descriptors_included_files_0 and 0020include_chain_0 are
> switched to normal tmpfiles, there is nothing in the test that needs
> relative includes.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  tests/shell/testcases/include/0002relative_0  | 13 ++++----
>  .../0013input_descriptors_included_files_0    | 30 +++++++++----------
>  .../testcases/include/0020include_chain_0     |  9 +++---
>  3 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/shell/testcases/include/0002relative_0 b/tests/shell/testcases/include/0002relative_0
> index a91cd8f00047..30f4bbdbff79 100755
> --- a/tests/shell/testcases/include/0002relative_0
> +++ b/tests/shell/testcases/include/0002relative_0
> @@ -1,21 +1,20 @@
>  #!/bin/bash
>  
> -set -e
> -
>  tmpfile1=$(mktemp -p .)
> -if [ ! -w $tmpfile1 ] ; then
> +if [ ! -w "$tmpfile1" ] ; then
>          echo "Failed to create tmp file" >&2
> -        exit 0
> +        exit 77
>  fi
>  
> +trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
> +set -e
> +
>  tmpfile2=$(mktemp -p .)
> -if [ ! -w $tmpfile2 ] ; then
> +if [ ! -w "$tmpfile2" ] ; then
>          echo "Failed to create tmp file" >&2
>          exit 0

this does not return 77, see below...

>  fi
>  
> -trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
> -
>  RULESET1="add table x"
>  RULESET2="include \"$tmpfile1\""
>  
> diff --git a/tests/shell/testcases/include/0013input_descriptors_included_files_0 b/tests/shell/testcases/include/0013input_descriptors_included_files_0
> index 03de50b3c499..9dc6615dd332 100755
> --- a/tests/shell/testcases/include/0013input_descriptors_included_files_0
> +++ b/tests/shell/testcases/include/0013input_descriptors_included_files_0
> @@ -7,32 +7,32 @@
>  # instead of return value of nft.
>  
>  
> -tmpfile1=$(mktemp -p .)
> -if [ ! -w $tmpfile1 ] ; then
> +tmpfile1=$(mktemp)
> +if [ ! -w "$tmpfile1" ] ; then
>          echo "Failed to create tmp file" >&2
> -        exit 0
> +        exit 77

if patch is lazy, maybe just check for the first one to fail.

>  fi
>  
> -tmpfile2=$(mktemp -p .)
> -if [ ! -w $tmpfile2 ] ; then
> +trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 $tmpfile4" EXIT # cleanup if aborted
> +
> +tmpfile2=$(mktemp)

this is good to remove -p . in mktemp.

I have destroyed 2 SSDs in 1.5 years running tests in a loop.

> +if [ ! -w "$tmpfile2" ] ; then
>          echo "Failed to create tmp file" >&2
> -        exit 0
> +        exit 77
>  fi
>  
> -tmpfile3=$(mktemp -p .)
> -if [ ! -w $tmpfile3 ] ; then
> +tmpfile3=$(mktemp)
> +if [ ! -w "$tmpfile3" ] ; then
>          echo "Failed to create tmp file" >&2
> -        exit 0
> +        exit 77
>  fi
>  
> -tmpfile4=$(mktemp -p .)
> -if [ ! -w $tmpfile4 ]; then
> +tmpfile4=$(mktemp)
> +if [ ! -w "$tmpfile4" ]; then
>          echo "Failed to create tmp file" >&2
> -        exit 0
> +        exit 77
>  fi
>  
> -trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 $tmpfile4" EXIT # cleanup if aborted
> -
>  RULESET1="include \"$tmpfile2\""
>  RULESET2="include \"$tmpfile3\""
>  RULESET3="add rule x y anything everything"			# wrong nft syntax
> @@ -44,7 +44,7 @@ echo "$RULESET3" > $tmpfile2
>  
>  $NFT -f $tmpfile1 2> $tmpfile4
>  
> -var=$(awk -F: '$4==" Error"{print $1;exit;}' $tmpfile4)
> +var=$(awk -F: '$4==" Error"{print $1;exit;}' "$tmpfile4")
>  
>  if [ $var == "$tmpfile3" ]; then
>  	echo "E: Test failed" >&2
> diff --git a/tests/shell/testcases/include/0020include_chain_0 b/tests/shell/testcases/include/0020include_chain_0
> index 49b6f76c6a8d..a09511974e29 100755
> --- a/tests/shell/testcases/include/0020include_chain_0
> +++ b/tests/shell/testcases/include/0020include_chain_0
> @@ -1,13 +1,12 @@
>  #!/bin/bash
>  
> -set -e
> -
> -tmpfile1=$(mktemp -p .)
> -if [ ! -w $tmpfile1 ] ; then
> +tmpfile1=$(mktemp)
> +if [ ! -w "$tmpfile1" ] ; then
>  	echo "Failed to create tmp file" >&2
> -	exit 0
> +	exit 77
>  fi
>  
> +set -e
>  trap "rm -rf $tmpfile1" EXIT # cleanup if aborted
>  
>  RULESET="table inet filter { }
> -- 
> 2.45.2
> 
> 

Re: [PATCH nft] tests: shell: don't rely on writable test directory

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Oct 22, 2024 at 04:09:54PM +0200, Florian Westphal wrote:
> >  tmpfile1=$(mktemp -p .)
> > -if [ ! -w $tmpfile1 ] ; then
> > +if [ ! -w "$tmpfile1" ] ; then
> >          echo "Failed to create tmp file" >&2
> > -        exit 0
> > +        exit 77
> >  fi
> >  
> > +trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
> > +set -e
> > +
> >  tmpfile2=$(mktemp -p .)
> > -if [ ! -w $tmpfile2 ] ; then
> > +if [ ! -w "$tmpfile2" ] ; then
> >          echo "Failed to create tmp file" >&2
> >          exit 0
> 
> this does not return 77, see below...

I only changed first invocation, if pwd is ro, that will
have failed already.

I can make that consistent if you prefer.

Re: [PATCH nft] tests: shell: don't rely on writable test directory

[Pablo Neira Ayuso]

On Tue, Oct 29, 2024 at 08:14:01AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Oct 22, 2024 at 04:09:54PM +0200, Florian Westphal wrote:
> > >  tmpfile1=$(mktemp -p .)
> > > -if [ ! -w $tmpfile1 ] ; then
> > > +if [ ! -w "$tmpfile1" ] ; then
> > >          echo "Failed to create tmp file" >&2
> > > -        exit 0
> > > +        exit 77
> > >  fi
> > >  
> > > +trap "rm -rf $tmpfile1 $tmpfile2" EXIT # cleanup if aborted
> > > +set -e
> > > +
> > >  tmpfile2=$(mktemp -p .)
> > > -if [ ! -w $tmpfile2 ] ; then
> > > +if [ ! -w "$tmpfile2" ] ; then
> > >          echo "Failed to create tmp file" >&2
> > >          exit 0
> > 
> > this does not return 77, see below...
> 
> I only changed first invocation, if pwd is ro, that will
> have failed already.

Right.

> I can make that consistent if you prefer.

Sure, go ahead and push this out if it helps you with virtme-ng

Thanks!