Hi Phil,
On Fri, 21 Feb 2020 22:17:04 +0100
Phil Sutter <phil@xxxxxx> wrote:
> Hi Stefano,
>
> On Fri, Feb 21, 2020 at 03:04:20AM +0100, Stefano Brivio wrote:
> > Patch 1/2 fixes the issue recently reported by Phil on a sequence of
> > add/flush/add operations, and patch 2/2 introduces a test case
> > covering that.
>
> This fixes my test case, thanks!
>
> I found another problem, but it's maybe on user space side (and not a
> crash this time ;):
>
> | # nft add table t
> | # nft add set t s '{ type inet_service . inet_service ; flags interval ; }
> | # nft add element t s '{ 20-30 . 40, 25-35 . 40 }'
> | # nft list ruleset
> | table ip t {
> | set s {
> | type inet_service . inet_service
> | flags interval
> | elements = { 20-30 . 40 }
> | }
> | }
>
> As you see, the second element disappears. It happens only if ranges
> overlap and non-range parts are identical.
>
> Looking at do_add_setelems(), set_to_intervals() should not be called
> for concatenated ranges, although I *think* range merging happens only
> there. So user space should cover for that already?!
Yes. I didn't consider the need for this kind of specification, given
that you can obtain the same result by simply adding two elements:
separate, partially overlapping elements can be inserted (which is, if I
recall correctly, not the case for rbtree).
If I recall correctly, we had a short discussion with Florian about
this, but I don't remember the conclusion.
However, I see the ugliness, and how this breaks probably legitimate
expectations. I guess we could call set_to_intervals() in this case,
that function might need some minor adjustments.
An alternative, and I'm not sure which one is the most desirable, would
be to refuse that kind of insertion.
--
Stefano
