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

[Stefano Brivio <sbrivio@redhat.com>]

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