[PATCH nft 1/3] tests: shell: README: copy edit

[PATCH nft 1/3] tests: shell: README: copy edit

[Štěpán Němec]

Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/tests/shell/README b/tests/shell/README
index e0279bbdc30c..35f6e3785f0e 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -1,16 +1,17 @@
-This test-suite is intended to perform tests of higher level than
-the other regression test-suite.
+This test suite is intended to perform tests on a higher level
+than the other regression test suites.
 
-It can run arbitrary executables which can perform any test apart of testing
-the nft syntax or netlink code (which is what the regression tests does).
+It can run arbitrary executables which can perform any test, not
+limited to testing the nft syntax or netlink code (which is what
+the regression tests do).
 
 To run the test suite (as root):
  $ cd tests/shell
  # ./run-tests.sh
 
-Test files are executables files with the pattern <<name_N>>, where N is the
-expected return code of the executable. Since they are located with `find',
-test-files can be spread in any sub-directories.
+Test files are executable files matching the pattern <<name_N>>,
+where N is the expected return code of the executable. Since they
+are located with `find', test files can be put in any subdirectory.
 
 You can turn on a verbose execution by calling:
  # ./run-tests.sh -v
@@ -18,11 +19,11 @@ You can turn on a verbose execution by calling:
 And generate missing dump files with:
  # ./run-tests.sh -g <TESTFILE>
 
-Before each call to the test-files, `nft flush ruleset' will be called.
-Also, test-files will receive the environment variable $NFT which contains the
-path to the nftables binary being tested.
+Before each test file invocation, `nft flush ruleset' will be called.
+Also, test file process environment will include the variable $NFT
+which contains the path to the nft binary being tested.
 
 You can pass an arbitrary $NFT value as well:
  # NFT=/usr/local/sbin/nft ./run-tests.sh
 
-By default the tests are run with the nft binary at '../../src/nft'
+By default, the tests are run with the nft binary at '../../src/nft'

base-commit: 2139913694a9850c9160920b2c638aac4828f9bb
-- 
2.33.1

[PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary

[Štěpán Němec]

Fixes: 7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command")
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/shell/README b/tests/shell/README
index 35f6e3785f0e..4dd595d99556 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -21,7 +21,7 @@ And generate missing dump files with:
 
 Before each test file invocation, `nft flush ruleset' will be called.
 Also, test file process environment will include the variable $NFT
-which contains the path to the nft binary being tested.
+which contains the nft command being tested.
 
 You can pass an arbitrary $NFT value as well:
  # NFT=/usr/local/sbin/nft ./run-tests.sh
-- 
2.33.1

[PATCH nft 3/3] tests: shell: README: clarify test file name convention

[Štěpán Němec]

Fixes: 4d26b6dd3c4c ("tests: shell: change all test scripts to return 0")
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/shell/README b/tests/shell/README
index 4dd595d99556..07d5cc2e3e7c 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -10,8 +10,12 @@ To run the test suite (as root):
  # ./run-tests.sh
 
 Test files are executable files matching the pattern <<name_N>>,
-where N is the expected return code of the executable. Since they
-are located with `find', test files can be put in any subdirectory.
+where N used to be the expected return code of the executable.
+(After commit 4d26b6dd3c4c, all tests should return 0 on success,
+no matter the test file name.)
+
+Since they are located with `find', test files can be put in any
+subdirectory.
 
 You can turn on a verbose execution by calling:
  # ./run-tests.sh -v
-- 
2.33.1

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Phil Sutter]

Hi,

What you you mean with 'copy edit'? Please make subject line a bit more
comprehensible. Also, adding at least a single line of description is
mandatory, even if the subject speaks for itself.

The actual changes look good to me, though!

Thanks, Phil

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Štěpán Němec]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Wed, 20 Oct 2021 17:04:02 +0200
Phil Sutter wrote:

> What you you mean with 'copy edit'?

https://en.wikipedia.org/wiki/Copy_editing

> Please make subject line a bit more comprehensible.

Would "fix language issues" work for you? Or, could we perhaps keep the
original subject, and add "fix language issues" in the body, to address
your other concern?

> Also, adding at least a single line of description is mandatory, even
> if the subject speaks for itself.

Commit messages consisting of only the subject and trailers (SOB et al.)
are quite common, both in this project and elsewhere. [1]

I suppose that as someone responsible for reviewing and applying
patches, you're free to install any hoops you see fit for contributors
to jump through, but if it is the project's and contributors' best
interest you have in mind, I think it would be better if you mentioned
the "description is always mandatory" rule explicitly in patch
submission guidelines [2] (providing a rationale and being consistent
about it in reality would be better still).

Thanks,
 
  Štěpán

[1] To get some picture, you can pipe output of the attached script to
'git log --stdin --no-walk', or further pipe that to 'git shortlog -ns'
for a summary.

[2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines


[-- Attachment #2: git_log_filter.perl --]
[-- Type: text/plain, Size: 334 bytes --]

#!/usr/bin/env perl

use strict;
use warnings;

my $log_cmd = "git log -z --pretty=format:'%H\n%b'";

local $/ = "\0";

open(my $log_fh, '-|', $log_cmd) or die "Can't start git log: $!";

while (<$log_fh>) {
  chomp;
  my ($hash, $body) = split(/\n/, $_, 2);
  print($hash, "\n") if ($body =~ /\A\s*(?:[^:\n]+: +[^\n]+\n?)*\s*\Z/);
}

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Phil Sutter]

Hi,

On Thu, Oct 21, 2021 at 10:30:25AM +0200, Štěpán Němec wrote:
> On Wed, 20 Oct 2021 17:04:02 +0200
> Phil Sutter wrote:
> 
> > What you you mean with 'copy edit'?
> 
> https://en.wikipedia.org/wiki/Copy_editing

Ah, thanks! I didn't know the term.

> > Please make subject line a bit more comprehensible.
> 
> Would "fix language issues" work for you? Or, could we perhaps keep the
> original subject, and add "fix language issues" in the body, to address
> your other concern?

Yes, I'm fine with keeping the subject if a description is added.

> > Also, adding at least a single line of description is mandatory, even
> > if the subject speaks for itself.
> 
> Commit messages consisting of only the subject and trailers (SOB et al.)
> are quite common, both in this project and elsewhere. [1]

Well yes, roughly 10% of all commits in nftables repo are. In iptables
repo it's even worse with close to 50%. Thanks a lot for providing the
script, BTW! :)

> I suppose that as someone responsible for reviewing and applying
> patches, you're free to install any hoops you see fit for contributors
> to jump through, but if it is the project's and contributors' best
> interest you have in mind, I think it would be better if you mentioned
> the "description is always mandatory" rule explicitly in patch
> submission guidelines [2] (providing a rationale and being consistent
> about it in reality would be better still).

Sorry for not checking the guideline but quoting advice I once received
from the top of my head instead. Maybe I was incorrect and in obvious
cases the description is optional. The relevant text in [2] at least
doesn't explicitly state it is, while being pretty verbose about it
regarding cover letters.

Cheers, Phil

> [2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Štěpán Němec]

On Thu, 21 Oct 2021 12:26:44 +0200
Phil Sutter wrote:

> Sorry for not checking the guideline but quoting advice I once received
> from the top of my head instead. Maybe I was incorrect and in obvious
> cases the description is optional. The relevant text in [2] at least
> doesn't explicitly state it is, while being pretty verbose about it
> regarding cover letters.

Does this mean that you retract your requirement? If not, could you
please make sure that you and the other maintainers are on the same page
about this, and document the requirement?

Regarding this patch series (if it is to be resent, in part or as a
whole): we've discussed the first patch so far; the other two patches
have no description, either. Do they need one? If so, could you provide
some suggestions? I can't think of anything sensible that isn't already
said in the subject, Fixes:, or the modified README text itself.

Thanks,

  Štěpán

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Pablo Neira Ayuso]

On Thu, Oct 21, 2021 at 01:03:23PM +0200, Štěpán Němec wrote:
> On Thu, 21 Oct 2021 12:26:44 +0200
> Phil Sutter wrote:
> 
> > Sorry for not checking the guideline but quoting advice I once received
> > from the top of my head instead. Maybe I was incorrect and in obvious
> > cases the description is optional. The relevant text in [2] at least
> > doesn't explicitly state it is, while being pretty verbose about it
> > regarding cover letters.
> 
> Does this mean that you retract your requirement? If not, could you
> please make sure that you and the other maintainers are on the same page
> about this, and document the requirement?
> 
> Regarding this patch series (if it is to be resent, in part or as a
> whole): we've discussed the first patch so far; the other two patches
> have no description, either. Do they need one? If so, could you provide
> some suggestions? I can't think of anything sensible that isn't already
> said in the subject, Fixes:, or the modified README text itself.

I also prefer if there is oneline description in the patch. My
suggestions:

* patch 1/3, not clear to me what "copy edit" means either.

* patch 2/3, what is the intention there? a path to the nft executable
  is most generic way to refer how you use $NFT, right?

* patch 3/3, I'd add a terse sentence so I do not need to scroll down
  and read the update to README to understand what this patch updates.
  I'd suggest: "Test files are located with find, so they can be placed
  in any location."

Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this
to the README file. The test infrastructure is only used for internal
development use, this is included in tarballs but distributors do not
package this.

So please address Phil's feedback.

Thanks.

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Štěpán Němec]

On Wed, 27 Oct 2021 11:06:00 +0200
Pablo Neira Ayuso wrote:

> I also prefer if there is oneline description in the patch. My
> suggestions:
>
> * patch 1/3, not clear to me what "copy edit" means either.

Description proposal:

  Grammar, wording, formatting fixes (no substantial change of meaning).

> * patch 2/3, what is the intention there? a path to the nft executable
>   is most generic way to refer how you use $NFT, right?

No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly
or at least also meant for humans, i.e., that the person reading the
commit message and wanting to better understand the change would look at
the referenced commit, but if that is a wrong assumption to make, I
propose to add the following description:

  Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g.
  'valgrind nft'.

> * patch 3/3, I'd add a terse sentence so I do not need to scroll down
>   and read the update to README to understand what this patch updates.
>   I'd suggest: "Test files are located with find, so they can be placed
>   in any location."

That text was just split to a separate paragraph, it has nothing to do
with the actual change.

> Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this
> to the README file. The test infrastructure is only used for internal
> development use, this is included in tarballs but distributors do not
> package this.

IMO this argument should speak _for_ including the commit reference
rather than omitting it, as the developer is more likely to be
interested in the commit than the consumer.

I thought about making the wording simply describe the current state
without any historical explanations, but saying "Test files are
executable files matching the pattern <<name_N>>, where N doesn't mean
anything." seemed weird.

Please advise.

-- 
Štěpán

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Pablo Neira Ayuso]

On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote:
> On Wed, 27 Oct 2021 11:06:00 +0200
> Pablo Neira Ayuso wrote:
> 
> > I also prefer if there is oneline description in the patch. My
> > suggestions:
> >
> > * patch 1/3, not clear to me what "copy edit" means either.
> 
> Description proposal:
> 
>   Grammar, wording, formatting fixes (no substantial change of meaning).
> 
> > * patch 2/3, what is the intention there? a path to the nft executable
> >   is most generic way to refer how you use $NFT, right?
> 
> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly
> or at least also meant for humans, i.e., that the person reading the
> commit message and wanting to better understand the change would look at
> the referenced commit, but if that is a wrong assumption to make, I
> propose to add the following description:

>   Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g.
>   'valgrind nft'.

OK, but why the reader need to know about former behaviour? The git
repository already provides the historical information if this is of
his interest. To me, the README file should contain the most up to
date information that is relevant to run the test infrastructure.

> > * patch 3/3, I'd add a terse sentence so I do not need to scroll down
> >   and read the update to README to understand what this patch updates.
> >   I'd suggest: "Test files are located with find, so they can be placed
> >   in any location."
> 
> That text was just split to a separate paragraph, it has nothing to do
> with the actual change.

OK, then please document every update in your patch.

> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this
> > to the README file. The test infrastructure is only used for internal
> > development use, this is included in tarballs but distributors do not
> > package this.
> 
> IMO this argument should speak _for_ including the commit reference
> rather than omitting it, as the developer is more likely to be
> interested in the commit than the consumer.
>
> I thought about making the wording simply describe the current state
> without any historical explanations, but saying "Test files are
> executable files matching the pattern <<name_N>>, where N doesn't mean
> anything." seemed weird.

For historical explanations, you can dig into git.

Thanks.

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Štěpán Němec]

On Wed, 27 Oct 2021 12:13:57 +0200
Pablo Neira Ayuso wrote:

> On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote:
>> > * patch 2/3, what is the intention there? a path to the nft executable
>> >   is most generic way to refer how you use $NFT, right?
>> 
>> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly
>> or at least also meant for humans, i.e., that the person reading the
>> commit message and wanting to better understand the change would look at
>> the referenced commit, but if that is a wrong assumption to make, I
>> propose to add the following description:
>
>>   Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g.
>>   'valgrind nft'.
>
> OK, but why the reader need to know about former behaviour? The git
> repository already provides the historical information if this is of
> his interest. To me, the README file should contain the most up to
> date information that is relevant to run the test infrastructure.

I agree, and that's what the patch does: 'a path to the nft executable'
is the former behaviour, arbitrary command is the most up to date
information. (The "Since..." text above is my proposal for the commit
description, not for the README file.)

>> > * patch 3/3, I'd add a terse sentence so I do not need to scroll down
>> >   and read the update to README to understand what this patch updates.
>> >   I'd suggest: "Test files are located with find, so they can be placed
>> >   in any location."
>> 
>> That text was just split to a separate paragraph, it has nothing to do
>> with the actual change.
>
> OK, then please document every update in your patch.

>> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this
>> > to the README file. The test infrastructure is only used for internal
>> > development use, this is included in tarballs but distributors do not
>> > package this.
>> 
>> IMO this argument should speak _for_ including the commit reference
>> rather than omitting it, as the developer is more likely to be
>> interested in the commit than the consumer.
>>
>> I thought about making the wording simply describe the current state
>> without any historical explanations, but saying "Test files are
>> executable files matching the pattern <<name_N>>, where N doesn't mean
>> anything." seemed weird.
>
> For historical explanations, you can dig into git.

Would the following README text work for you?

  Test files are executable files matching the pattern <<name_N>>,
  where N has no meaning. All tests should return 0 on success.

  Since they are located with `find', test files can be put in any
  subdirectory.

Maybe saying "where N should be 0 in all new tests" would be less
strange?

I think both are significantly less helpful than my original version,
but at least they aren't wrong like the current text.

Proposed commit description, based on your comments:

  Since commit 4d26b6dd3c4c, test file name suffix no longer reflects
  expected exit code in all cases.

  Move the sentence "Since they are located with `find', test files can
  be put in any subdirectory." to a separate paragraph.

-- 
Štěpán

Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Pablo Neira Ayuso]

On Wed, Oct 27, 2021 at 01:04:38PM +0200, Štěpán Němec wrote:
> On Wed, 27 Oct 2021 12:13:57 +0200
> Pablo Neira Ayuso wrote:
> 
> > On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote:
> >> > * patch 2/3, what is the intention there? a path to the nft executable
> >> >   is most generic way to refer how you use $NFT, right?
> >> 
> >> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly
> >> or at least also meant for humans, i.e., that the person reading the
> >> commit message and wanting to better understand the change would look at
> >> the referenced commit, but if that is a wrong assumption to make, I
> >> propose to add the following description:
> >
> >>   Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g.
> >>   'valgrind nft'.
> >
> > OK, but why the reader need to know about former behaviour? The git
> > repository already provides the historical information if this is of
> > his interest. To me, the README file should contain the most up to
> > date information that is relevant to run the test infrastructure.
> 
> I agree, and that's what the patch does: 'a path to the nft executable'
> is the former behaviour, arbitrary command is the most up to date
> information. (The "Since..." text above is my proposal for the commit
> description, not for the README file.)

OK, this slightly more verbose description in the paragraph above LGTM.

[...]
> >> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this
> >> > to the README file. The test infrastructure is only used for internal
> >> > development use, this is included in tarballs but distributors do not
> >> > package this.
> >> 
> >> IMO this argument should speak _for_ including the commit reference
> >> rather than omitting it, as the developer is more likely to be
> >> interested in the commit than the consumer.
> >>
> >> I thought about making the wording simply describe the current state
> >> without any historical explanations, but saying "Test files are
> >> executable files matching the pattern <<name_N>>, where N doesn't mean
> >> anything." seemed weird.
> >
> > For historical explanations, you can dig into git.
> 
> Would the following README text work for you?
> 
>   Test files are executable files matching the pattern <<name_N>>,
>   where N has no meaning. All tests should return 0 on success.
> 
>   Since they are located with `find', test files can be put in any
>   subdirectory.

LGTM.

> Maybe saying "where N should be 0 in all new tests" would be less
> strange?

This is also OK, we can probably remove the trailing 0 at some point
given it has no meaning anymore.

> I think both are significantly less helpful than my original version,
> but at least they aren't wrong like the current text.

Makes sense.

> Proposed commit description, based on your comments:
> 
>   Since commit 4d26b6dd3c4c, test file name suffix no longer reflects
>   expected exit code in all cases.
> 
>   Move the sentence "Since they are located with `find', test files can
>   be put in any subdirectory." to a separate paragraph.

LGTM.

Thanks.

[PATCH nft v2 1/4] tests: shell: README: copy edit

[Štěpán Němec]

Grammar, wording, formatting fixes (no substantial change of meaning).

Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/tests/shell/README b/tests/shell/README
index e0279bbdc30c..35f6e3785f0e 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -1,16 +1,17 @@
-This test-suite is intended to perform tests of higher level than
-the other regression test-suite.
+This test suite is intended to perform tests on a higher level
+than the other regression test suites.
 
-It can run arbitrary executables which can perform any test apart of testing
-the nft syntax or netlink code (which is what the regression tests does).
+It can run arbitrary executables which can perform any test, not
+limited to testing the nft syntax or netlink code (which is what
+the regression tests do).
 
 To run the test suite (as root):
  $ cd tests/shell
  # ./run-tests.sh
 
-Test files are executables files with the pattern <<name_N>>, where N is the
-expected return code of the executable. Since they are located with `find',
-test-files can be spread in any sub-directories.
+Test files are executable files matching the pattern <<name_N>>,
+where N is the expected return code of the executable. Since they
+are located with `find', test files can be put in any subdirectory.
 
 You can turn on a verbose execution by calling:
  # ./run-tests.sh -v
@@ -18,11 +19,11 @@ You can turn on a verbose execution by calling:
 And generate missing dump files with:
  # ./run-tests.sh -g <TESTFILE>
 
-Before each call to the test-files, `nft flush ruleset' will be called.
-Also, test-files will receive the environment variable $NFT which contains the
-path to the nftables binary being tested.
+Before each test file invocation, `nft flush ruleset' will be called.
+Also, test file process environment will include the variable $NFT
+which contains the path to the nft binary being tested.
 
 You can pass an arbitrary $NFT value as well:
  # NFT=/usr/local/sbin/nft ./run-tests.sh
 
-By default the tests are run with the nft binary at '../../src/nft'
+By default, the tests are run with the nft binary at '../../src/nft'

base-commit: 6ad2058da66affc105d325e45ff82fd5b5cac41e
-- 
2.33.1

[PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary

[Štěpán Němec]

Since commit 7c8a44b25c22, $NFT can contain an arbitrary command,
e.g. 'valgrind nft'.

Fixes: 7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command")
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/shell/README b/tests/shell/README
index 35f6e3785f0e..4dd595d99556 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -21,7 +21,7 @@ And generate missing dump files with:
 
 Before each test file invocation, `nft flush ruleset' will be called.
 Also, test file process environment will include the variable $NFT
-which contains the path to the nft binary being tested.
+which contains the nft command being tested.
 
 You can pass an arbitrary $NFT value as well:
  # NFT=/usr/local/sbin/nft ./run-tests.sh
-- 
2.33.1

[PATCH nft v2 3/4] tests: shell: README: clarify test file name convention

[Štěpán Němec]

Since commit 4d26b6dd3c4c, test file name suffix no longer reflects
expected exit code in all cases.

Move the sentence "Since they are located with `find', test files can
be put in any subdirectory." to a separate paragraph.

Fixes: 4d26b6dd3c4c ("tests: shell: change all test scripts to return 0")
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/shell/README b/tests/shell/README
index 4dd595d99556..ea2b0b98f95f 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -10,8 +10,11 @@ To run the test suite (as root):
  # ./run-tests.sh
 
 Test files are executable files matching the pattern <<name_N>>,
-where N is the expected return code of the executable. Since they
-are located with `find', test files can be put in any subdirectory.
+where N should be 0 in all new tests. All tests should return 0 on
+success.
+
+Since they are located with `find', test files can be put in any
+subdirectory.
 
 You can turn on a verbose execution by calling:
  # ./run-tests.sh -v
-- 
2.33.1

[PATCH nft v2 4/4] tests: shell: $NFT needs to be invoked unquoted

[Štěpán Němec]

The variable has to undergo word splitting, otherwise the shell tries
to find the variable value as an executable, which breaks in cases that
7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command")
intends to support.

Mention this in the shell tests README.

Fixes: d8ccad2a2b73 ("tests: cover baecd1cf2685 ("segtree: Fix segfault when restoring a huge interval set")")
Signed-off-by: Štěpán Němec <snemec@redhat.com>
---
 tests/shell/README                                       | 3 +++
 tests/shell/testcases/sets/0068interval_stack_overflow_0 | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/shell/README b/tests/shell/README
index ea2b0b98f95f..3af17a9e72ca 100644
--- a/tests/shell/README
+++ b/tests/shell/README
@@ -29,4 +29,7 @@ which contains the nft command being tested.
 You can pass an arbitrary $NFT value as well:
  # NFT=/usr/local/sbin/nft ./run-tests.sh
 
+Note that, to support usage such as NFT='valgrind nft', tests must
+invoke $NFT unquoted.
+
 By default, the tests are run with the nft binary at '../../src/nft'
diff --git a/tests/shell/testcases/sets/0068interval_stack_overflow_0 b/tests/shell/testcases/sets/0068interval_stack_overflow_0
index 134282de2826..6620572449c3 100755
--- a/tests/shell/testcases/sets/0068interval_stack_overflow_0
+++ b/tests/shell/testcases/sets/0068interval_stack_overflow_0
@@ -26,4 +26,4 @@ table inet test68_table {
 }
 EOF
 
-( ulimit -s 128 && "$NFT" -f "$ruleset_file" )
+( ulimit -s 128 && $NFT -f "$ruleset_file" )
-- 
2.33.1

Re: [PATCH nft v2 1/4] tests: shell: README: copy edit

[Phil Sutter]

On Fri, Nov 05, 2021 at 12:39:08PM +0100, Štěpán Němec wrote:
> Grammar, wording, formatting fixes (no substantial change of meaning).

Series applied, thanks for following-up!