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

[Florian Westphal <fw@strlen.de>]

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

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.

Thanks Pablo!