| Message ID | 5735155a0e98738cdc5507385d6225e05c225465.1590324033.git.sbrivio@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Fix evaluation of anonymous sets with concatenated ranges | expand |
On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote: > Add a simple anonymous set including a concatenated range and check > it's inserted correctly. This is roughly based on the existing > 0025_anonymous_set_0 test case. I think this is pretty much redundant to what tests/py/inet/sets.t tests if you simply enable the anonymous set rule I added in commit 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges"). Cheers, Phil
On Mon, 25 May 2020 17:48:34 +0200 Phil Sutter <phil@nwl.cc> wrote: > On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote: > > Add a simple anonymous set including a concatenated range and check > > it's inserted correctly. This is roughly based on the existing > > 0025_anonymous_set_0 test case. > > I think this is pretty much redundant to what tests/py/inet/sets.t tests > if you simply enable the anonymous set rule I added in commit > 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges"). Nice, I wasn't aware of that one. Anyway, this isn't really redundant as it also checks that sets are reported back correctly (which I expected to break, even if it didn't) by comparing with the dump file, instead of just checking netlink messages. So I'd actually suggest that we keep this and I'd send another patch (should I repost this series? A separate patch?) to enable the rule you added for py tests.
Hi, On Tue, May 26, 2020 at 01:12:47AM +0200, Stefano Brivio wrote: > On Mon, 25 May 2020 17:48:34 +0200 > Phil Sutter <phil@nwl.cc> wrote: > > > On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote: > > > Add a simple anonymous set including a concatenated range and check > > > it's inserted correctly. This is roughly based on the existing > > > 0025_anonymous_set_0 test case. > > > > I think this is pretty much redundant to what tests/py/inet/sets.t tests > > if you simply enable the anonymous set rule I added in commit > > 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges"). > > Nice, I wasn't aware of that one. Anyway, this isn't really redundant > as it also checks that sets are reported back correctly (which I > expected to break, even if it didn't) by comparing with the dump file, > instead of just checking netlink messages. > > So I'd actually suggest that we keep this and I'd send another patch > (should I repost this series? A separate patch?) to enable the rule you > added for py tests. But nft-test.py does check ruleset listing, that's what the optional third part of a rule line is for. The syntax is roughly: | <rule>;(fail|ok[;<rule_out>]) It allows us to cover for asymmetric rule listings. A simple example from any/ct.t is: | ct mark or 0x23 == 0x11;ok;ct mark | 0x00000023 == 0x00000011 So nft reports mark values with leading zeroes (don't ask me why ;). Am I missing some extra your test does? Cheers, Phil
On Tue, 26 May 2020 15:39:52 +0200 Phil Sutter <phil@nwl.cc> wrote: > Hi, > > On Tue, May 26, 2020 at 01:12:47AM +0200, Stefano Brivio wrote: > > On Mon, 25 May 2020 17:48:34 +0200 > > Phil Sutter <phil@nwl.cc> wrote: > > > > > On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote: > > > > Add a simple anonymous set including a concatenated range and check > > > > it's inserted correctly. This is roughly based on the existing > > > > 0025_anonymous_set_0 test case. > > > > > > I think this is pretty much redundant to what tests/py/inet/sets.t tests > > > if you simply enable the anonymous set rule I added in commit > > > 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges"). > > > > Nice, I wasn't aware of that one. Anyway, this isn't really redundant > > as it also checks that sets are reported back correctly (which I > > expected to break, even if it didn't) by comparing with the dump file, > > instead of just checking netlink messages. > > > > So I'd actually suggest that we keep this and I'd send another patch > > (should I repost this series? A separate patch?) to enable the rule you > > added for py tests. > > But nft-test.py does check ruleset listing, that's what the optional > third part of a rule line is for. The syntax is roughly: > > | <rule>;(fail|ok[;<rule_out>]) > > It allows us to cover for asymmetric rule listings. Oh, sorry, I didn't realise that... the README actually mentions it (section C), Line 5, Part 3 of example), but I skipped that part. > A simple example from any/ct.t is: > > | ct mark or 0x23 == 0x11;ok;ct mark | 0x00000023 == 0x00000011 > > So nft reports mark values with leading zeroes (don't ask me why ;). I guess it's actually neater that way for 32-bit fields :) > Am I missing some extra your test does? No, nothing. I'll replace this patch by one that simply enables the case you already added.
diff --git a/tests/shell/testcases/sets/0048anonymous_set_concat_0 b/tests/shell/testcases/sets/0048anonymous_set_concat_0 new file mode 100755 index 000000000000..fab61231d0c0 --- /dev/null +++ b/tests/shell/testcases/sets/0048anonymous_set_concat_0 @@ -0,0 +1,7 @@ +#!/bin/sh -e +# +# 0048anonymous_sets_concat_0 - Anonymous sets with concatenated ranges + +${NFT} add table t +${NFT} add chain t c '{ type filter hook forward priority 0 ; }' +${NFT} add rule t c 'ip daddr . tcp dport { 192.0.2.1 . 49152-65535 }' diff --git a/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft b/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft new file mode 100644 index 000000000000..c54ffae9d6d2 --- /dev/null +++ b/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft @@ -0,0 +1,6 @@ +table ip t { + chain c { + type filter hook forward priority filter; policy accept; + ip daddr . tcp dport { 192.0.2.1 . 49152-65535 } + } +}
Add a simple anonymous set including a concatenated range and check it's inserted correctly. This is roughly based on the existing 0025_anonymous_set_0 test case. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- tests/shell/testcases/sets/0048anonymous_set_concat_0 | 7 +++++++ .../testcases/sets/dumps/0048anonymous_set_concat_0.nft | 6 ++++++ 2 files changed, 13 insertions(+) create mode 100755 tests/shell/testcases/sets/0048anonymous_set_concat_0 create mode 100644 tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft