| Message ID | 20241022140956.8160-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [nft] tests: shell: don't rely on writable test directory | expand |
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 > >
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.
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!
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 { }
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(-)