| Message ID | 20180206181847.14434-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] Enable automerge feature for anonymous sets | expand |
Hi Phil, On Tue, Feb 06, 2018 at 07:18:47PM +0100, Phil Sutter wrote: > Automatic merging of adjacent/overlapping ranges upon insertion has > clear benefits performance- and readability-wise. The drawbacks which > led to disabling it by default don't apply to anonymous sets since they > are read-only anyway, so enable this feature for them again. Question is, why someone would be adding elements with overlapping ranges in an anonymous set? Then, when listing the ruleset you will get something different to what you've added. This would also be inconsistent with regards to the existing behaviour in named sets, where this is turned off by default. For named sets, that are useful to maintain white/blacklists, I understand this simplifies complexity for people dealing with them. But not sure for anonymous sets. @Jeff: Is this also useful to you in the anonymous set use-case? IIRC we agreed that this was good for named sets, but not for anonymous sets. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 7 Feb 2018, Pablo Neira Ayuso wrote: > On Tue, Feb 06, 2018 at 07:18:47PM +0100, Phil Sutter wrote: > > Automatic merging of adjacent/overlapping ranges upon insertion has > > clear benefits performance- and readability-wise. The drawbacks which > > led to disabling it by default don't apply to anonymous sets since they > > are read-only anyway, so enable this feature for them again. > > Question is, why someone would be adding elements with overlapping > ranges in an anonymous set? Then, when listing the ruleset you will get > something different to what you've added. This would also be > inconsistent with regards to the existing behaviour in named sets, where > this is turned off by default. > > For named sets, that are useful to maintain white/blacklists, I > understand this simplifies complexity for people dealing with them. But > not sure for anonymous sets. > > @Jeff: Is this also useful to you in the anonymous set use-case? IIRC we > agreed that this was good for named sets, but not for anonymous sets. In my opinion the consistent behaviour is the most desired one. Such subleties that by default there's no automerge in named sets but it's on for anonymous sets are easily overlooked by users. Better have a flag, option to turn it on explicitly for a given anonymous set. Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo, On Wed, Feb 07, 2018 at 12:39:43AM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 06, 2018 at 07:18:47PM +0100, Phil Sutter wrote: > > Automatic merging of adjacent/overlapping ranges upon insertion has > > clear benefits performance- and readability-wise. The drawbacks which > > led to disabling it by default don't apply to anonymous sets since they > > are read-only anyway, so enable this feature for them again. > > Question is, why someone would be adding elements with overlapping > ranges in an anonymous set? Then, when listing the ruleset you will > get something different to what you've added. This would also be > inconsistent with regards to the existing behaviour in named sets, > where this is turned off by default. Recapping the pros and cons of automatic merging the discussion brought up (I probably miss some): + Overlaps may be hard to spot (and avoid) if they sit in different defines used to fill a single set. - Items added to a set can't be removed again if they were merged with others added at the same time. From those, I assume that if one uses defines to manage e.g. white/black lists, a named set is probably not used either. The con above applies only to named sets since anonymous ones can't be changed. Therefore I considered automatic merging happening in anonymous sets to be desirable. > For named sets, that are useful to maintain white/blacklists, I > understand this simplifies complexity for people dealing with them. > But not sure for anonymous sets. I'm not entirely sure about that since there are multiple ways to see things. AFAIR, Jeff's point was when he combines set elements from different sources he doesn't want to check them manually for potential overlaps (or even can't). So there automatic merging is a good thing. If my fail2ban script can't delete a range previously added after it timeouts, it fails and recovery is not trivial (find the merged element and divide it again). Here, merging is a bad thing. Overall though, I wonder whether using interval sets for white/black lists isn't a bad idea at all. :) > @Jeff: Is this also useful to you in the anonymous set use-case? IIRC > we agreed that this was good for named sets, but not for anonymous > sets. I'm curious to hearing his thoughts as well. Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 06, 2018 at 07:18:47PM +0100, Phil Sutter wrote: > Automatic merging of adjacent/overlapping ranges upon insertion has > clear benefits performance- and readability-wise. The drawbacks which > led to disabling it by default don't apply to anonymous sets since they > are read-only anyway, so enable this feature for them again. Applied, thanks Phil. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/evaluate.c b/src/evaluate.c index aa7c28e8b00ff..076855e257e77 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -87,6 +87,7 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx, set->handle.set = xstrdup(name); set->key = key; set->init = expr; + set->automerge = set->flags & NFT_SET_INTERVAL; if (ctx->table != NULL) list_add_tail(&set->list, &ctx->table->sets);
Automatic merging of adjacent/overlapping ranges upon insertion has clear benefits performance- and readability-wise. The drawbacks which led to disabling it by default don't apply to anonymous sets since they are read-only anyway, so enable this feature for them again. Cc: Jeff Kletsky <netfilter@allycomm.com> Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/evaluate.c | 1 + 1 file changed, 1 insertion(+)