Re: [PATCH,nf-next RFC 0/2] add NFTA_SET_ELEM_KEY_END

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

Hi Stefano,

On Mon, Dec 02, 2019 at 05:19:52PM +0100, Stefano Brivio wrote:
[...]
> On Mon,  2 Dec 2019 14:14:05 +0100
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > This patchset extends the netlink API to allow to express an interval
> > with one single element.
> > 
> > This simplifies this interface since userspace does not need to send two
> > independent elements anymore, one of the including the
> > NFT_SET_ELEM_INTERVAL_END flag.
> > 
> > The idea is to use the _DESC to specify that userspace speaks the kernel
> > that new API representation. In your case, the new description attribute
> > that tells that this set contains interval + concatenation implicitly
> > tells the kernel that userspace supports for this new API.
> 
> Thanks! I just had a quick look, I think the new set implementation
> would indeed look more elegant this way. As to design choices, I'm
> afraid I'm not familiar enough with the big picture to comment on the
> general idea, but my uninformed opinion agrees with this approach. :)
> 
> For what it's worth, I'd review this in deeper detail next.

Thanks.

> > If you're fine with this, I can scratch a bit of time to finish the
> > libnftnl part. The nft code will need a small update too. You will not
> > need to use the nft_set_pipapo object as scratchpad area anymore.
> 
> On my side, I'm almost done with nft/libnftnl/kernel changes for the
> NFT_SET_DESC_CONCAT thing. How should we proceed? Do you want me to
> share those patches so that you can add this bit on top, or should this
> come first, or in a separate series?

My suggestion is that you can take them and place them at the
beginning of your batch since it will be the first client for this new
netlink attribute, you will have to adapt pipapo to use the new
key_end value too.

> I could also just share the new nft/libnftnl patches (I should have them
> ready between today and tomorrow), and proceed adapting the kernel part
> according to your changes.

I still have to send you the libnftnl part for this.

> Related question: to avoid copying data around, I'm now dynamically
> allocating a struct nft_data_desc in nf_tables_newset() with a
> reference from struct nft_set: desc->dlen, desc->klen, desc->size would
> all live there, together with the "subkey" stuff.
> 
> Is it a bad idea? I can undo it easily, I just don't know if there's a
> specific reason why those fields are repeated in struct nft_set.

Not sure I understand, probably some code sketch? From your words it
does not look like a major issue though, but let me know.

BTW, there is also one more pending issue: I can see there is a clone
point in nft_set_pipapo, you mentioned some problems to make things
fit in into the transaction infrastructure. Could you describe how you
integrate with it? Probably there is a chance to extend the front-end
API too to make it easier for pipapo.

Thanks.