Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Mar 16, 2016 at 02:31:42PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I was thinking on simplifying the codebase one we get more immediates,
> > we will at least get one more to handle ct helper assignment.
> > 
> > So from the code we can just check:
> > 
> >         if (nla[NFTA_CT_IMM]) {
> >                 err = nla_parse_nested(tb, NFTA_CT_IMM, nla, nft_ct_imm_policy);
> >                 if (err < 0)
> >                         return err;
> > 
> >                 switch (key) {
> >                 case NFT_CT_LABEL:
> >                         if (!tb[NFTA_CT_IMM_LABEL])
> >                                 return -EINVAL;
> 
> Right, something like this.
> 
> 
> > BTW, probably we should consider using some generic way to represent
> > the immediates through enum nft_data_attributes. So we have core code
> > that that every expression can reuse to handle immediates.  I think
> > the concern here is data length validation, eg. labels are 4 bytes and
> > helpers are strings limited to 16 bytes. From the libnftnl side, so
> > far we represent these as _DATA attributes:
> > 
> > enum {
> >         NFTNL_EXPR_CMP_SREG     = NFTNL_EXPR_BASE,
> >         NFTNL_EXPR_CMP_OP,
> >         NFTNL_EXPR_CMP_DATA,
> > };
> > 
> > Otherwise, we'll start seeing code in every expression handling
> > immediates in their own way (ie. in a not consolidated way).
> 
> Yes, thats a valid concern.
> 
> I guess it depends on wheter we want to support multi-action, or if
> we will just put several actions after one another.
> 
> set label bla set helper ftp

I would say not to this at this stage.

> If thats two nft_ct invocations, then generic representation
> (NTA_IMM_DATA_U32, NFTA_IMM_DATA_STRING, ...) that can be used by any
> expressions that need to carry their own immediate values is definitely
> much better.
> 
> I'll go with using generic IMM_U32 for now, inside a CT_IMM nested
> attr.

OK, give it a shot. Thanks Florian.