From mboxrd@z Thu Jan  1 00:00:00 1970
From: Florian Westphal <fw@strlen.de>
Subject: Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set
 support
Date: Wed, 16 Mar 2016 14:31:42 +0100
Message-ID: <20160316133142.GA30305@breakpoint.cc>
References: <1458058211-11147-1-git-send-email-fw@strlen.de>
 <1458058211-11147-2-git-send-email-fw@strlen.de>
 <20160315170813.GA25284@salvia>
 <20160315230921.GD8185@breakpoint.cc>
 <20160316093933.GA16938@breakpoint.cc>
 <20160316131754.GA28195@salvia>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Florian Westphal <fw@strlen.de>, netfilter-devel@vger.kernel.org
To: Pablo Neira Ayuso <pablo@netfilter.org>
Return-path: <netfilter-devel-owner@vger.kernel.org>
Received: from Chamillionaire.breakpoint.cc ([80.244.247.6]:57473 "EHLO
	Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S933804AbcCPNbo (ORCPT
	<rfc822;netfilter-devel@vger.kernel.org>);
	Wed, 16 Mar 2016 09:31:44 -0400
Content-Disposition: inline
In-Reply-To: <20160316131754.GA28195@salvia>
Sender: netfilter-devel-owner@vger.kernel.org
List-ID: <netfilter-devel.vger.kernel.org>

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!