[PATCH nft] tests: shell: Drop redefinition of DIFF variable

[PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Stefano Brivio]

Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
introduced the definition of DIFF at the top of run-tests.sh, to make
it visually part of the configuration section. Commit 68310ba0f9c2
("tests: shell: Search diff tool once and for all") override this
definition.

Drop the unexpected redefinition of DIFF.

Fixes: 68310ba0f9c2 ("tests: shell: Search diff tool once and for all")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tests/shell/run-tests.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 94115066b1bf..fcc87a8957c8 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -45,7 +45,6 @@ if [ ! -x "$MODPROBE" ] ; then
 	msg_error "no modprobe binary found"
 fi
 
-DIFF="$(which diff)"
 if [ ! -x "$DIFF" ] ; then
 	DIFF=true
 fi
-- 
2.27.0

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Phil Sutter]

Hi Stefano,

On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:
> Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> introduced the definition of DIFF at the top of run-tests.sh, to make
> it visually part of the configuration section. Commit 68310ba0f9c2
> ("tests: shell: Search diff tool once and for all") override this
> definition.
> 
> Drop the unexpected redefinition of DIFF.

I would fix it the other way round, dropping the first definition. It's
likely a missing bit from commit 68310ba0f9c20, the second definition is
in line with FIND and MODPROBE definitions immediately preceding it.

Cheers, Phil

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Stefano Brivio]

Hi Phil,

On Mon, 15 Jun 2020 11:00:44 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Hi Stefano,
> 
> On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:
> > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > introduced the definition of DIFF at the top of run-tests.sh, to make
> > it visually part of the configuration section. Commit 68310ba0f9c2
> > ("tests: shell: Search diff tool once and for all") override this
> > definition.
> > 
> > Drop the unexpected redefinition of DIFF.  
> 
> I would fix it the other way round, dropping the first definition.

Then it's not visibly configurable anymore. It was in 2018, so it
looks like a regression to me.

> It's likely a missing bit from commit 68310ba0f9c20, the second
> definition is in line with FIND and MODPROBE definitions immediately
> preceding it.

I see a few issues with those blocks:

- that should be a single function called (once or multiple times) for
  nft, find, modprobe, diff, anything else we'll need in the future.
  It would avoid any oversight of this kind and keep the script
  cleaner. For example, what makes sort(1) special here?

- quotes are applied inconsistently. If you expect multiple words from
  which(1), then variables should also be quoted when used, otherwise
  the check might go through, and we fail later

- we should use 'command -v', which is the standard and standardised
  way of doing this rather than which(1): 'which' has many different
  and inconsistent implementations. Will it check aliases? Should you
  suppress stdout or stderr? How do you... 'which which'?

- we should extend the configurability for single commands to all of
  them. I need to export NFT, 'diff' I can edit on top of the file, the
  rest is not configurable at all. It's easy with a single function.

...so I started rewriting that, then realised I didn't have time at the
moment and just fixed the obvious issue I saw.

If the definition on the top is not actually useful, then I'd rather
keep things this way instead of just proposing a cosmetic change for
things that would actually need a small rework.

-- 
Stefano

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Phil Sutter]

Hi Stefano,

On Mon, Jun 15, 2020 at 12:18:11PM +0200, Stefano Brivio wrote:
> On Mon, 15 Jun 2020 11:00:44 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Hi Stefano,
> > 
> > On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:
> > > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > > introduced the definition of DIFF at the top of run-tests.sh, to make
> > > it visually part of the configuration section. Commit 68310ba0f9c2
> > > ("tests: shell: Search diff tool once and for all") override this
> > > definition.
> > > 
> > > Drop the unexpected redefinition of DIFF.  
> > 
> > I would fix it the other way round, dropping the first definition.
> 
> Then it's not visibly configurable anymore. It was in 2018, so it
> looks like a regression to me.

Hmm? Commit 68310ba0f9c2 is from January this year?!

> > It's likely a missing bit from commit 68310ba0f9c20, the second
> > definition is in line with FIND and MODPROBE definitions immediately
> > preceding it.
> 
> I see a few issues with those blocks:
> 
> - that should be a single function called (once or multiple times) for
>   nft, find, modprobe, diff, anything else we'll need in the future.
>   It would avoid any oversight of this kind and keep the script
>   cleaner. For example, what makes sort(1) special here?

It is simply grown and therefore a bit inconsistent.

> - quotes are applied inconsistently. If you expect multiple words from
>   which(1), then variables should also be quoted when used, otherwise
>   the check might go through, and we fail later

There are needless quotes around $(...) but within single brackets we
need them if we expect empty or multi-word strings. Typical output would
be 'diff not found' and using '[ ! -x "$DIFF" ]' we check if which
returned diff's path or said error message. We don't expect diff's path
to contain spaces, hence no quoting afterwards.

Cheers, Phil

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Stefano Brivio]

On Mon, 15 Jun 2020 13:54:24 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Hi Stefano,
> 
> On Mon, Jun 15, 2020 at 12:18:11PM +0200, Stefano Brivio wrote:
> > On Mon, 15 Jun 2020 11:00:44 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > Hi Stefano,
> > > 
> > > On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:  
> > > > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > > > introduced the definition of DIFF at the top of run-tests.sh, to make
> > > > it visually part of the configuration section. Commit 68310ba0f9c2
> > > > ("tests: shell: Search diff tool once and for all") override this
> > > > definition.
> > > > 
> > > > Drop the unexpected redefinition of DIFF.    
> > > 
> > > I would fix it the other way round, dropping the first definition.  
> > 
> > Then it's not visibly configurable anymore. It was in 2018, so it
> > looks like a regression to me.  
> 
> Hmm? Commit 68310ba0f9c2 is from January this year?!

Commit 7d93e2c2fbc7 (which makes it "configurable") is from March 2018.

> > > It's likely a missing bit from commit 68310ba0f9c20, the second
> > > definition is in line with FIND and MODPROBE definitions immediately
> > > preceding it.  
> > 
> > I see a few issues with those blocks:
> > 
> > - that should be a single function called (once or multiple times) for
> >   nft, find, modprobe, diff, anything else we'll need in the future.
> >   It would avoid any oversight of this kind and keep the script
> >   cleaner. For example, what makes sort(1) special here?  
> 
> It is simply grown and therefore a bit inconsistent.

Yes, hence worth a minor rework perhaps? I can take care of this at
some point.

> > - quotes are applied inconsistently. If you expect multiple words from
> >   which(1), then variables should also be quoted when used, otherwise
> >   the check might go through, and we fail later  
> 
> There are needless quotes around $(...) but within single brackets we
> need them if we expect empty or multi-word strings. Typical output would
> be 'diff not found' and using '[ ! -x "$DIFF" ]' we check if which
> returned diff's path or said error message. We don't expect diff's path
> to contain spaces, hence no quoting afterwards.

Probably stupid but:

[ break nft ]

# mkdir "my secret binaries"
# cp /usr/bin/diff "my secret binaries/"
# export PATH="my secret binaries:$PATH"
# nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
I: using nft command: nftables/tests/shell/../../src/nft

W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 127
nftables/tests/shell/testcases/listing/0003table_0: line 15: my: command not found

I: results: [OK] 0 [FAILED] 1 [TOTAL] 1

or, perhaps more reasonable:

# grep DIFF=\" nftables/tests/shell/run-tests.sh
DIFF="diff -y"
# nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
I: using nft command: nftables/tests/shell/../../src/nft

W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 1

I: results: [OK] 0 [FAILED] 1 [TOTAL] 1

Personally, this bothers me more than some cheap, extra quotes. If the
general agreement is that it's not worth to add quotes to fix this,
fine, I would skip this the day I have time to propose the single
checking function rework I mentioned. Just let me know.

-- 
Stefano

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Phil Sutter]

Hi,

On Mon, Jun 15, 2020 at 02:40:55PM +0200, Stefano Brivio wrote:
> On Mon, 15 Jun 2020 13:54:24 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Hi Stefano,
> > 
> > On Mon, Jun 15, 2020 at 12:18:11PM +0200, Stefano Brivio wrote:
> > > On Mon, 15 Jun 2020 11:00:44 +0200
> > > Phil Sutter <phil@nwl.cc> wrote:
> > >   
> > > > Hi Stefano,
> > > > 
> > > > On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:  
> > > > > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > > > > introduced the definition of DIFF at the top of run-tests.sh, to make
> > > > > it visually part of the configuration section. Commit 68310ba0f9c2
> > > > > ("tests: shell: Search diff tool once and for all") override this
> > > > > definition.
> > > > > 
> > > > > Drop the unexpected redefinition of DIFF.    
> > > > 
> > > > I would fix it the other way round, dropping the first definition.  
> > > 
> > > Then it's not visibly configurable anymore. It was in 2018, so it
> > > looks like a regression to me.  
> > 
> > Hmm? Commit 68310ba0f9c2 is from January this year?!
> 
> Commit 7d93e2c2fbc7 (which makes it "configurable") is from March 2018.

I think you're misinterpreting that commit regarding an attempt at
making diff binary configurable.

> > > > It's likely a missing bit from commit 68310ba0f9c20, the second
> > > > definition is in line with FIND and MODPROBE definitions immediately
> > > > preceding it.  
> > > 
> > > I see a few issues with those blocks:
> > > 
> > > - that should be a single function called (once or multiple times) for
> > >   nft, find, modprobe, diff, anything else we'll need in the future.
> > >   It would avoid any oversight of this kind and keep the script
> > >   cleaner. For example, what makes sort(1) special here?  
> > 
> > It is simply grown and therefore a bit inconsistent.
> 
> Yes, hence worth a minor rework perhaps? I can take care of this at
> some point.

Sure, why not. For now it works just fine, apart from the duplicate
line which is merely a cosmetic issue.

> > > - quotes are applied inconsistently. If you expect multiple words from
> > >   which(1), then variables should also be quoted when used, otherwise
> > >   the check might go through, and we fail later  
> > 
> > There are needless quotes around $(...) but within single brackets we
> > need them if we expect empty or multi-word strings. Typical output would
> > be 'diff not found' and using '[ ! -x "$DIFF" ]' we check if which
> > returned diff's path or said error message. We don't expect diff's path
> > to contain spaces, hence no quoting afterwards.
> 
> Probably stupid but:
> 
> [ break nft ]
> 
> # mkdir "my secret binaries"
> # cp /usr/bin/diff "my secret binaries/"
> # export PATH="my secret binaries:$PATH"
> # nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
> I: using nft command: nftables/tests/shell/../../src/nft

Yes, I mentioned we don't expect $PATH contents to contain spaces.
Having such a thing would probably break more things than just
nftables testsuite. :)

> W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 127
> nftables/tests/shell/testcases/listing/0003table_0: line 15: my: command not found
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> or, perhaps more reasonable:
> 
> # grep DIFF=\" nftables/tests/shell/run-tests.sh
> DIFF="diff -y"

This is no guaranteed functionality. There's no comment or anything
stating you could change the DIFF definition atop the script to
customize diff behaviour. So strictly speaking you're on your own if you
do that and a broken testsuite is not unexpected.

> # nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
> I: using nft command: nftables/tests/shell/../../src/nft
> 
> W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 1
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> Personally, this bothers me more than some cheap, extra quotes. If the
> general agreement is that it's not worth to add quotes to fix this,
> fine, I would skip this the day I have time to propose the single
> checking function rework I mentioned. Just let me know.

As said, the quotes are there to cover the expected 'which' output, no
more and no less. Supporting user-defined diff-command (or custom
options) is new functionality IMO. I'm totally fine with that and merely
want to point out we're not talking about fixing a bug here.

Cheers, Phil

Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Stefano Brivio]

On Mon, 15 Jun 2020 15:21:34 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Mon, Jun 15, 2020 at 02:40:55PM +0200, Stefano Brivio wrote:
> > 
> > [...]
> > 
> > Commit 7d93e2c2fbc7 (which makes it "configurable") is from March 2018.  
> 
> I think you're misinterpreting that commit regarding an attempt at
> making diff binary configurable.
> 
> [...]
>
> > [...]
> >
> > # grep DIFF=\" nftables/tests/shell/run-tests.sh
> > DIFF="diff -y"  
> 
> This is no guaranteed functionality. There's no comment or anything
> stating you could change the DIFF definition atop the script to
> customize diff behaviour.

But...

 # Configuration
 TESTDIR="./$(dirname $0)/"
 RETURNCODE_SEPARATOR="_"
 SRC_NFT="$(dirname $0)/../../src/nft"
+POSITIVE_RET=0
+DIFF=$(which diff)

:) well, now that you tell me, I can guess that "# Configuration" only
applied to the original parts, and the rest was added there simply
because there were no other "sections".

> [...]
> 
> As said, the quotes are there to cover the expected 'which' output, no
> more and no less. Supporting user-defined diff-command (or custom
> options) is new functionality IMO. I'm totally fine with that and merely
> want to point out we're not talking about fixing a bug here.

Okay, I see. I'll try to get back to that soon.

Pablo, I think you can drop this patch.

-- 
Stefano