On Thu, Sep 19, 2019 at 04:01:44PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > > > /* Only one of these operations is supported */ > > > - if ((flags & (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT)) == > > > - (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT)) > > > + if ((flags & (NFT_SET_MAP | NFT_SET_OBJECT)) == > > > + (NFT_SET_MAP | NFT_SET_OBJECT)) > > > > That's fine by now. But look, combining map and objects should be fine > > in the future. A user might want to combine this by specifying an IP > > address as the right hand side of a mapping and a stateful counter > > (with a name) to be updated when matching on that element. This is not > > supported yet though. > > > > > + return -EOPNOTSUPP; > > > + if ((flags & (NFT_SET_EVAL | NFT_SET_OBJECT)) == > > > + (NFT_SET_EVAL | NFT_SET_OBJECT)) > > > > This looks fine. > > I'll submit a patch then. Thanks. > > > 2. avoid depreaction of 'meter', since thats what is documented everywhere > > > and appears to work fine > > > > OK, but only for anonymous meters. > > Meters are always anonymous afaiu, they are bound to the rule that > creates them. Delete the rule -- meter is gone. OK. sorry, NFT_SET_ANONYMOUS is misleading, this anonymous flag means 'set is bound to rule', it was wrong to choose this flag name, probably NFT_SET_BOUND would be better. Anyway, I was referring to meter with no name. > > > 3. patch nft to hide normal sets from 'nft list meters' output > > > > This makes no sense for anonymous meters. Since the kernel picks the > > name, I don't think nft should expose them to the user. > > See > commit 24a912eea21f9d18909c53a865cf623839616281 > parser_bison: dismiss anonymous meters > > nft frontend only allows to create meter with a name. Oh, we already deprecated meters with no name :-) And meters with a name should not be used, there's a better syntax now for this rather than this sloppy way to create an object from the rule itself without an explicit definition. > > > What to do with dynamic, I fear we have to remove it, i.e. just ignore > > > the "dynamic" flag when talking to the kernel. Otherwise sets using dynamic flag > > > will only work on future/very recent kernels. > > > > I would not go that path. > > > > You are just hitting a limitation on the existing implementation. > > People cannot make lookups on a dynamic set on existing kernels, > > that's all. > > I guess thats one way to put it. > > > There is no NFT_SET_EXPR support for nft_lookup either, is this a bug? > > Do you mean NFT_SET_EVAL? No, I mean there is no NFT_SET_EXT_EXPR handling yet, sorry I forgot the _EXT_ infix. nft_lookup should invoke the expression that is attached. Control plane code is also missing, there is no way to create the NFT_SET_EXT_EXPR from newsetelem() in nf_tables_api.c. If NFT_SET_EVAL is set or not from nft_lookup is completely irrelevant, nft_lookup should not care about this flag. > If so, NFT_SET_EVAL is handled by nft_dynset.c (this is what gets used > when you use the 'meter' syntax). > > > There is no way to combine NFT_SET_MAP with NFT_SET_OBJ, but that is > > also artificial, this is just happening again because the code is > > incomplete and needs an extension. > > Ok. We can drop the checks once that is done. Agreed.
