[PATCH nft] tests: monitor: fix up test case breakage

[PATCH nft] tests: monitor: fix up test case breakage

[Florian Westphal]

Monitor test fails:

echo: running tests from file set-simple.t
echo output differs!
--- /tmp/tmp.FGtiyL99bB/tmp.2QxLSjzQqk  2024-10-29 20:54:41.308293201 +0100
+++ /tmp/tmp.FGtiyL99bB/tmp.A5rp0Z0dBJ  2024-10-29 20:54:41.317293201 +0100
@@ -1,2 +1,3 @@
-add element ip t portrange { 1024-65535 }
 add element ip t portrange { 100-200 }
+add element ip t portrange { 1024-65535 }
+# new generation 510 by process 129009 (nft)

I also noticed -j mode did not work correctly, add missing json annotations
in set-concat-interval.t while at it.

Fixes: 20f1c60ac8c8 ("src: collapse set element commands from parser")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/monitor/testcases/set-concat-interval.t | 3 +++
 tests/monitor/testcases/set-simple.t          | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/monitor/testcases/set-concat-interval.t b/tests/monitor/testcases/set-concat-interval.t
index 763dc319f0d1..75f38280bf82 100644
--- a/tests/monitor/testcases/set-concat-interval.t
+++ b/tests/monitor/testcases/set-concat-interval.t
@@ -10,3 +10,6 @@ I add map ip t s { typeof udp length . @ih,32,32 : verdict; flags interval; elem
 O add map ip t s { typeof udp length . @ih,32,32 : verdict; flags interval; }
 O add element ip t s { 20-80 . 0x14 : accept }
 O add element ip t s { 1-10 . 0xa : drop }
+J {"add": {"map": {"family": "ip", "name": "s", "table": "t", "type": ["integer", "integer"], "handle": 0, "map": "verdict", "flags": ["interval"]}}}
+J {"add": {"element": {"family": "ip", "table": "t", "name": "s", "elem": {"set": [[{"concat": [{"range": [20, 80]}, 20]}, {"accept": null}]]}}}}
+J {"add": {"element": {"family": "ip", "table": "t", "name": "s", "elem": {"set": [[{"concat": [{"range": [1, 10]}, 10]}, {"drop": null}]]}}}}
diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
index 8ca4f32463fd..6853a0ebbb0c 100644
--- a/tests/monitor/testcases/set-simple.t
+++ b/tests/monitor/testcases/set-simple.t
@@ -37,9 +37,10 @@ J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem"
 # make sure half open before other element works
 I add element ip t portrange { 1024-65535 }
 I add element ip t portrange { 100-200 }
-O -
-J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
+O add element ip t portrange { 100-200 }
+O add element ip t portrange { 1024-65535 }
 J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [100, 200]}]}}}}
+J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
 
 # make sure deletion of elements works
 I delete element ip t portrange { 0-10 }
-- 
2.45.2

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Pablo Neira Ayuso]

On Tue, Oct 29, 2024 at 09:12:19PM +0100, Florian Westphal wrote:
> Monitor test fails:
> 
> echo: running tests from file set-simple.t
> echo output differs!
> --- /tmp/tmp.FGtiyL99bB/tmp.2QxLSjzQqk  2024-10-29 20:54:41.308293201 +0100
> +++ /tmp/tmp.FGtiyL99bB/tmp.A5rp0Z0dBJ  2024-10-29 20:54:41.317293201 +0100
> @@ -1,2 +1,3 @@
> -add element ip t portrange { 1024-65535 }
>  add element ip t portrange { 100-200 }
> +add element ip t portrange { 1024-65535 }
> +# new generation 510 by process 129009 (nft)
> 
> I also noticed -j mode did not work correctly, add missing json annotations
> in set-concat-interval.t while at it.

Thanks for fixing up tests.

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Phil Sutter]

On Tue, Oct 29, 2024 at 09:12:19PM +0100, Florian Westphal wrote:
[...]
> diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
> index 8ca4f32463fd..6853a0ebbb0c 100644
> --- a/tests/monitor/testcases/set-simple.t
> +++ b/tests/monitor/testcases/set-simple.t
> @@ -37,9 +37,10 @@ J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem"
>  # make sure half open before other element works
>  I add element ip t portrange { 1024-65535 }
>  I add element ip t portrange { 100-200 }
> -O -
> -J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
> +O add element ip t portrange { 100-200 }
> +O add element ip t portrange { 1024-65535 }
>  J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [100, 200]}]}}}}
> +J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}

This is odd: Why does monitor output reverse input? If nft reorders
input, the test ("make sure half open before other element works") is
probably moot anyway.

Cheers, Phil

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> This is odd: Why does monitor output reverse input? If nft reorders
> input, the test ("make sure half open before other element works") is
> probably moot anyway.

No idea.  It happens since 20f1c60ac8c8

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Pablo Neira Ayuso]

On Wed, Oct 30, 2024 at 07:19:47PM +0100, Phil Sutter wrote:
> On Tue, Oct 29, 2024 at 09:12:19PM +0100, Florian Westphal wrote:
> [...]
> > diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
> > index 8ca4f32463fd..6853a0ebbb0c 100644
> > --- a/tests/monitor/testcases/set-simple.t
> > +++ b/tests/monitor/testcases/set-simple.t
> > @@ -37,9 +37,10 @@ J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem"
> >  # make sure half open before other element works
> >  I add element ip t portrange { 1024-65535 }
> >  I add element ip t portrange { 100-200 }
> > -O -
> > -J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
> > +O add element ip t portrange { 100-200 }
> > +O add element ip t portrange { 1024-65535 }
> >  J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [100, 200]}]}}}}
> > +J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
> 
> This is odd: Why does monitor output reverse input? If nft reorders
> input, the test ("make sure half open before other element works") is
> probably moot anyway.

Because elements are collapsed in one single command.

  I add element ip t portrange { 1024-65535 }
  I add element ip t portrange { 100-200 }

and there is qsort() to search for interval overlaps.

this becomes one single command with { 100-200, 1024-65535 }

I have a patch to remove the overlap check using qsort() from
userspace which should remove this reordering, but it seems there is a
overlap case that kernel does not handle yet.

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Pablo Neira Ayuso]

On Wed, Oct 30, 2024 at 10:38:11PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Oct 30, 2024 at 07:19:47PM +0100, Phil Sutter wrote:
> > On Tue, Oct 29, 2024 at 09:12:19PM +0100, Florian Westphal wrote:
> > [...]
> > > diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
> > > index 8ca4f32463fd..6853a0ebbb0c 100644
> > > --- a/tests/monitor/testcases/set-simple.t
> > > +++ b/tests/monitor/testcases/set-simple.t
> > > @@ -37,9 +37,10 @@ J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem"
> > >  # make sure half open before other element works
> > >  I add element ip t portrange { 1024-65535 }
> > >  I add element ip t portrange { 100-200 }
> > > -O -
> > > -J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
> > > +O add element ip t portrange { 100-200 }
> > > +O add element ip t portrange { 1024-65535 }
> > >  J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [100, 200]}]}}}}
> > > +J {"add": {"element": {"family": "ip", "table": "t", "name": "portrange", "elem": {"set": [{"range": [1024, 65535]}]}}}}
> > 
> > This is odd: Why does monitor output reverse input? If nft reorders
> > input, the test ("make sure half open before other element works") is
> > probably moot anyway.
> 
> Because elements are collapsed in one single command.
> 
>   I add element ip t portrange { 1024-65535 }
>   I add element ip t portrange { 100-200 }
> 
> and there is qsort() to search for interval overlaps.
> 
> this becomes one single command with { 100-200, 1024-65535 }
> 
> I have a patch to remove the overlap check using qsort() from
> userspace which should remove this reordering, but it seems there is a
> overlap case that kernel does not handle yet.

I can also revert the patch if you don't like, but it is saving _a
lot_ of memory from userspace for the silly one element per line case.

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I can also revert the patch if you don't like, but it is saving _a
> lot_ of memory from userspace for the silly one element per line case.

I think your patch is fine, the question is if we should ditch the
test case instead of reordering the expected output.

Re: [PATCH nft] tests: monitor: fix up test case breakage

[Phil Sutter]

On Thu, Oct 31, 2024 at 12:29:14AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I can also revert the patch if you don't like, but it is saving _a
> > lot_ of memory from userspace for the silly one element per line case.
> 
> I think your patch is fine, the question is if we should ditch the
> test case instead of reordering the expected output.

ACK, that was my thought. Looking at the test case and seeing how later
tests depend on these elements being present, dropping it is not
practical though. We could reorder the I lines to match output, but the
commit is upstream already, so ... nevermind. :)

Thanks for the fix and clarification, guys!

Cheers, Phil