| Message ID | 20190816144241.11469-1-fw@strlen.de |
|---|---|
| Headers | show |
| Series | add typeof keyword | expand |
Hi Florian, On Fri, Aug 16, 2019 at 04:42:33PM +0200, Florian Westphal wrote: > This patch series adds the typeof keyword. > > The only dependency is a small change to libnftnl to add two new > UDATA_SET_TYPEOF enum values. Thanks for working on this. > named set can be configured as follows: > > set os { > type typeof(osf name) > elements = { "Linux", "Windows" } > } > > or > nft add set ip filter allowed "{ type typeof(ip daddr) . typeof(tcp dport); }" I know I sent a RFC using typeof(), I wonder if you could just use the selector instead, it's a bit of a lot of type typeof() . typeof() probably. So this is left as this: type osf name in concatenations, like this: nft add set ip filter allowed "{ type ip daddr . tcp dport; }" Probably I would ask my sysadmin friends what they think. I spent too much time on coding, so all these typeof() look natural to me, but it might be a bit too much syntactic sugar for someone that is more in network operations, not sure. P.S: patch 1/8 and 2/8 are related to this patchset? After quick glance, not obvious to me or if they are again related to multiple nft_ctx_new() calls.
On Sat, Aug 17, 2019 at 12:23:51PM +0200, Pablo Neira Ayuso wrote: [...] > On Fri, Aug 16, 2019 at 04:42:33PM +0200, Florian Westphal wrote: [..] > P.S: patch 1/8 and 2/8 are related to this patchset? After quick > glance, not obvious to me or if they are again related to multiple > nft_ctx_new() calls. I found it, it is in patch 6/8, it making a call to set_make_key to get the datatype to translate the selector to datatype using the parser API.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Sat, Aug 17, 2019 at 12:23:51PM +0200, Pablo Neira Ayuso wrote: > [...] > > On Fri, Aug 16, 2019 at 04:42:33PM +0200, Florian Westphal wrote: > [..] > > P.S: patch 1/8 and 2/8 are related to this patchset? After quick > > glance, not obvious to me or if they are again related to multiple > > nft_ctx_new() calls. > > I found it, it is in patch 6/8, it making a call to set_make_key to > get the datatype to translate the selector to datatype using the > parser API. Yes, I could push patch #1 independently though.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > I know I sent a RFC using typeof(), I wonder if you could just use the > selector instead, it's a bit of a lot of type typeof() . typeof() > probably. > > So this is left as this: > > type osf name > > in concatenations, like this: > > nft add set ip filter allowed "{ type ip daddr . tcp dport; }" > > Probably I would ask my sysadmin friends what they think. Yes, please do, it would be good to get a non-developer perspective. I'm very used to things like sizeof(), so typeof() felt natural to me. Might be very un-intuitive for non-developers though, so it would be good to get outside perspective. Thanks!
On 8/17/19 10:55 PM, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> I know I sent a RFC using typeof(), I wonder if you could just use the >> selector instead, it's a bit of a lot of type typeof() . typeof() >> probably. >> >> So this is left as this: >> >> type osf name >> >> in concatenations, like this: >> >> nft add set ip filter allowed "{ type ip daddr . tcp dport; }" >> >> Probably I would ask my sysadmin friends what they think. > > Yes, please do, it would be good to get a non-developer perspective. > > I'm very used to things like sizeof(), so typeof() felt natural to me. > > Might be very un-intuitive for non-developers though, so it would be > good to get outside perspective. > From my point of view, this is a rather advanced operation. As long as it is properly documented, I don't see any problem with `typeof()`. Also, just `typeof` would work of course. Up to you. Thanks for working on this!
Hi Florian, On Sun, Aug 18, 2019 at 04:33:15PM +0200, Arturo Borrero Gonzalez wrote: > On 8/17/19 10:55 PM, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > >> I know I sent a RFC using typeof(), I wonder if you could just use the > >> selector instead, it's a bit of a lot of type typeof() . typeof() > >> probably. > >> > >> So this is left as this: > >> > >> type osf name > >> > >> in concatenations, like this: > >> > >> nft add set ip filter allowed "{ type ip daddr . tcp dport; }" > >> > >> Probably I would ask my sysadmin friends what they think. > > > > Yes, please do, it would be good to get a non-developer perspective. > > > > I'm very used to things like sizeof(), so typeof() felt natural to me. > > > > Might be very un-intuitive for non-developers though, so it would be > > good to get outside perspective. > > From my point of view, this is a rather advanced operation. As long as it is > properly documented, I don't see any problem with `typeof()`. > > Also, just `typeof` would work of course. Up to you. My suggestion is: typeof ip saddr . tcp dport . meta mark but, not to allow the use of the existing datatypes with typeof. So either the user picks: type ipv4_addr . inet_service . mark or this: typeof ip saddr . tcp dport . meta mark Then, for the notation: integer,bits I would suggest this one can only be used from 'type'. Goal is basically: * Avoiding repetitive typeof() that looks a bit like C programming. * Mixing datatype with inferred datatypes looks a bit sloppy to me from syntax consistency point of view. This approach is flexible enough to cover all use cases this patchset is dealing with I think. If you like this approach, I would suggest you just update the patchset to support this and then push this out, I'll catch up later on to revisit this before the next release, in case I can propose you some refinement for your review. Let me know, thanks.
This patch series adds the typeof keyword. The only dependency is a small change to libnftnl to add two new UDATA_SET_TYPEOF enum values. named set can be configured as follows: set os { type typeof(osf name) elements = { "Linux", "Windows" } } or nft add set ip filter allowed "{ type typeof(ip daddr) . typeof(tcp dport); }" ... which is the same as the "old" 'type ipv4_addr . inet_service". The type is stored in the kernel via the udata set infrastructure, on listing -- if a udata type is present -- nft will validate that this type matches the set key length. This initial submission doesn't include a documentation update because I'd like to get feedback on the chosen syntax first. Florian Westphal (8): src: libnftnl: run single-initcalls only once src: libnftnl: split nft_ctx_new/free src: store expr, not dtype to track data in sets src: parser: add syntax to provide bitsize for non-spcific types src: add "typeof" keyword src: add "typeof" print support src: netlink: remove assertion tests: add typeof test cases include/datatype.h | 1 include/netlink.h | 1 include/nftables.h | 3 include/rule.h | 6 src/datatype.c | 5 src/evaluate.c | 58 ++++-- src/expression.c | 2 src/json.c | 4 src/libnftables.c | 48 +++-- src/mnl.c | 39 ++++ src/monitor.c | 2 src/netlink.c | 176 ++++++++++++++++++--- src/netlink_delinearize.c | 15 + src/parser_bison.y | 26 ++- src/parser_json.c | 8 src/rule.c | 35 +++- src/scanner.l | 1 src/segtree.c | 8 tests/shell/testcases/maps/dumps/typeof_maps_0.nft | 16 + tests/shell/testcases/maps/typeof_maps_0 | 26 +++ tests/shell/testcases/sets/dumps/typeof_sets_0.nft | 31 +++ tests/shell/testcases/sets/typeof_sets_0 | 40 ++++ 22 files changed, 459 insertions(+), 92 deletions(-)