[nftables,0/8] add typeof keyword
mbox series

Message ID 20190816144241.11469-1-fw@strlen.de
Headers show
Series
  • add typeof keyword
Related show

Message

Florian Westphal Aug. 16, 2019, 2:42 p.m. UTC
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(-)

Comments

Pablo Neira Ayuso Aug. 17, 2019, 10:23 a.m. UTC | #1
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.
Pablo Neira Ayuso Aug. 17, 2019, 10:33 a.m. UTC | #2
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.
Florian Westphal Aug. 17, 2019, 7:26 p.m. UTC | #3
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.
Florian Westphal Aug. 17, 2019, 8:55 p.m. UTC | #4
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!
Arturo Borrero Gonzalez Aug. 18, 2019, 2:33 p.m. UTC | #5
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!
Pablo Neira Ayuso Aug. 26, 2019, 9:49 a.m. UTC | #6
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.