| Message ID | 20241029201221.17865-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [nft] tests: monitor: fix up test case breakage | expand |
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.
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
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
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.
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.
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.
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
--- /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 }