nftables data type names

nftables data type names

[Patrick McHardy]

Before the upcoming release, I'd like to add some more consistency among
nftables data type names. We currently have the following types:

 ct_state
 ct_dir
 ct_status
 ct_label
 invalid
 verdict
 nfproto
 bitmask
 integer
 string
 lladdr
 ipv4_address
 ipv6_address
 inet_protocol
 inet_service
 mark
 time
 mh_type
 realm
 tc_handle
 ifindex
 arphrd
 uid
 gid
 icmp_type
 tcp_flag
 dccp_pkttype
 icmpv6_type
 arp_op
 etheraddr
 ethertype

In some cases we're more verbose, in other we're using abrevations.
I'd like to decide for either one.

The following ones should IMO definitely be changed:

- etheraddr => ether_address or mac_address. ether_addr would be more
  consistent with ethertype.

- ethertype => ether_type if ether_addr is used

- optionally: *_address => *_addr

- otherwise: ll_addr => ll_address

- arphrd => iftype/interface_type?

If we're deciding for more verbose names (which IMO is fine for types),
I'd also change:

- arp_op => arp_operation
- ifindex => interface_index
- nfproto => nf_protocol

otherwise:

- inet_protocol => inet_proto
- *_address -> *_addr

Basically the should be human readable, not programmer readable, should
describe what they actually are (not arphrd) and should be consistent.

Any comments or suggestions?

Re: nftables data type names

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> Before the upcoming release, I'd like to add some more consistency among
> nftables data type names. We currently have the following types:
[..]
> In some cases we're more verbose, in other we're using abrevations.
> I'd like to decide for either one.
> 
> The following ones should IMO definitely be changed:
> 
> - etheraddr => ether_address or mac_address. ether_addr would be more
>   consistent with ethertype.
> 
> - ethertype => ether_type if ether_addr is used

I like ether_type/ether_addr.

> - optionally: *_address => *_addr

We already have 'ip saddr/daddr' etc in nft rules,
so I'd prefer to use _addr everywhere.

> - arphrd => iftype/interface_type?

I read that as "arphdr"...

Since its used of iif/oiftype I think interface_type is good choice.

> If we're deciding for more verbose names (which IMO is fine for types),
> I'd also change:
> 
> - arp_op => arp_operation
> - ifindex => interface_index
> - nfproto => nf_protocol

I agree iff we go for eg. _address instead of _addr.
I would prefer _addr, i.e.

> otherwise:
> 
> - inet_protocol => inet_proto

inet_proto, too.
Looking at scanner.l we also have l3proto, l4proto, nfproto keywords.

[ I realize that there is not requirement to be consistent with
datatype names vs. nft rules but I see no reason to differ ]

> Basically the should be human readable, not programmer readable, should
> describe what they actually are (not arphrd) and should be consistent.

Fully agree, more consistency would be good.

Re: nftables data type names

[Patrick McHardy]

On Sat, Apr 12, 2014 at 12:56:46PM +0200, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > Before the upcoming release, I'd like to add some more consistency among
> > nftables data type names. We currently have the following types:
> [..]
> > In some cases we're more verbose, in other we're using abrevations.
> > I'd like to decide for either one.
> > 
> > The following ones should IMO definitely be changed:
> > 
> > - etheraddr => ether_address or mac_address. ether_addr would be more
> >   consistent with ethertype.
> > 
> > - ethertype => ether_type if ether_addr is used
> 
> I like ether_type/ether_addr.

Ok, lets take those.

> > - optionally: *_address => *_addr
> 
> We already have 'ip saddr/daddr' etc in nft rules,
> so I'd prefer to use _addr everywhere.

Good point.

> > - arphrd => iftype/interface_type?
> 
> I read that as "arphdr"...

Yeah, same here.

> Since its used of iif/oiftype I think interface_type is good choice.

Agreed, will change.

> > If we're deciding for more verbose names (which IMO is fine for types),
> > I'd also change:
> > 
> > - arp_op => arp_operation
> > - ifindex => interface_index
> > - nfproto => nf_protocol
> 
> I agree iff we go for eg. _address instead of _addr.
> I would prefer _addr, i.e.

Yep.

> > otherwise:
> > 
> > - inet_protocol => inet_proto
> 
> inet_proto, too.
> Looking at scanner.l we also have l3proto, l4proto, nfproto keywords.

I'll also change those.

> [ I realize that there is not requirement to be consistent with
> datatype names vs. nft rules but I see no reason to differ ]

Yeah, sticking to internal data type names is a bad habit from a usability
perspective IMO. iiftype / arphrd is the best example for this.

> > Basically the should be human readable, not programmer readable, should
> > describe what they actually are (not arphrd) and should be consistent.
> 
> Fully agree, more consistency would be good.

Thanks for the feedback, I'll post a patch soon.

Re: nftables data type names

[Patrick McHardy]

On Sat, Apr 12, 2014 at 01:03:53PM +0200, Patrick McHardy wrote:
> On Sat, Apr 12, 2014 at 12:56:46PM +0200, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > > Before the upcoming release, I'd like to add some more consistency among
> > > nftables data type names. We currently have the following types:
> > [..]
> > > In some cases we're more verbose, in other we're using abrevations.
> > > I'd like to decide for either one.
> > > 
> > 
> > I like ether_type/ether_addr.
> > ...
> > Fully agree, more consistency would be good.
> 
> Thanks for the feedback, I'll post a patch soon.

What I've got now is:

Address types:

 ll_addr
 ipv4_addr
 ipv6_addr
 ether_addr

Protocol types:

 nf_proto
 inet_proto

(l3proto and l4proto don't exist as types)

Conntrack types:

 ct_state
 ct_dir
 ct_status
 ct_label

Packet type related types:

 mh_type
 iface_type
 icmp_type
 dccp_pkttype
 icmpv6_type
 ether_type

Interface related types:

 ifindex
 iface_type

Arp types:

 arp_op

Other types:

 mark
 time
 realm
 uid
 gid

And a few base types that are fine as they are.

The things I'm not sure about are:

 ifindex: this is a well established term I think, however it would be more
          consistent to use iface_index

 mark/realm: pkt_mark and pkt_realm/route_realm perhaps. Not sure

 uid/gid: sk_uid/sk_gid?

Any opinions on these?

Re: nftables data type names

[Pablo Neira Ayuso]

On Sun, Apr 13, 2014 at 12:57:53PM +0200, Patrick McHardy wrote:
> On Sat, Apr 12, 2014 at 01:03:53PM +0200, Patrick McHardy wrote:
> > On Sat, Apr 12, 2014 at 12:56:46PM +0200, Florian Westphal wrote:
> > > Patrick McHardy <kaber@trash.net> wrote:
> > > > Before the upcoming release, I'd like to add some more consistency among
> > > > nftables data type names. We currently have the following types:
> > > [..]
> > > > In some cases we're more verbose, in other we're using abrevations.
> > > > I'd like to decide for either one.
> > > > 
> > > 
> > > I like ether_type/ether_addr.
> > > ...
> > > Fully agree, more consistency would be good.
> > 
> > Thanks for the feedback, I'll post a patch soon.
> 
> What I've got now is:
> 
> Address types:
> 
>  ll_addr
>  ipv4_addr
>  ipv6_addr
>  ether_addr
> 
> Protocol types:
> 
>  nf_proto
>  inet_proto
> 
> (l3proto and l4proto don't exist as types)
> 
> Conntrack types:
> 
>  ct_state
>  ct_dir
>  ct_status
>  ct_label
> 
> Packet type related types:
> 
>  mh_type
>  iface_type
>  icmp_type
>  dccp_pkttype
>  icmpv6_type
>  ether_type
> 
> Interface related types:
> 
>  ifindex
>  iface_type
> 
> Arp types:
> 
>  arp_op
> 
> Other types:
> 
>  mark
>  time
>  realm
>  uid
>  gid
> 
> And a few base types that are fine as they are.
> 
> The things I'm not sure about are:
> 
>  ifindex: this is a well established term I think, however it would be more
>           consistent to use iface_index

We can have an alias for this perhaps so both work?

>  mark/realm: pkt_mark and pkt_realm/route_realm perhaps. Not sure

if we would ever have ct_mark, then the initial pkt_ prefix is good to
have.

>  uid/gid: sk_uid/sk_gid?

I like the sk_ prefix also clearly specifies to users that this is
related to the socket information.

So following prefix_keytype looks good to me. The prefix just denotes
the scope for which the keytype applies.

Re: nftables data type names

[Patrick McHardy]

On Sun, Apr 13, 2014 at 02:37:28PM +0200, Pablo Neira Ayuso wrote:
> On Sun, Apr 13, 2014 at 12:57:53PM +0200, Patrick McHardy wrote:
> > 
> > What I've got now is:
> > 
> > Address types:
> > 
> >  ll_addr
> >  ipv4_addr
> >  ipv6_addr
> >  ether_addr
> > 
> > Protocol types:
> > 
> >  nf_proto
> >  inet_proto
> > 
> > (l3proto and l4proto don't exist as types)
> > 
> > Conntrack types:
> > 
> >  ct_state
> >  ct_dir
> >  ct_status
> >  ct_label
> > 
> > Packet type related types:
> > 
> >  mh_type
> >  iface_type
> >  icmp_type
> >  dccp_pkttype
> >  icmpv6_type
> >  ether_type
> > 
> > Interface related types:
> > 
> >  ifindex
> >  iface_type
> > 
> > Arp types:
> > 
> >  arp_op
> > 
> > Other types:
> > 
> >  mark
> >  time
> >  realm
> >  uid
> >  gid
> > 
> > And a few base types that are fine as they are.
> > 
> > The things I'm not sure about are:
> > 
> >  ifindex: this is a well established term I think, however it would be more
> >           consistent to use iface_index
> 
> We can have an alias for this perhaps so both work?

Sure. We have to decide for one for output however. I'd prefer to use iface_
for consistency.

> >  mark/realm: pkt_mark and pkt_realm/route_realm perhaps. Not sure
> 
> if we would ever have ct_mark, then the initial pkt_ prefix is good to
> have.

Well, its the data type, so ct_mark is unlikely to exist since the ct marks
are effectively packet marks. This is also the reason why I chose "mark"
without a prefix in the first place, but I now think using pkt_mark for
both is more precise about what the meaning of these values is.

> >  uid/gid: sk_uid/sk_gid?
> 
> I like the sk_ prefix also clearly specifies to users that this is
> related to the socket information.

Ok, will change.

> So following prefix_keytype looks good to me. The prefix just denotes
> the scope for which the keytype applies.

Yep.