On Mon, 15 Jun 2020 13:54:24 +0200
Phil Sutter <phil@xxxxxx> 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@xxxxxx> 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
