Re: [nft PATCH 4/4] segtree: Refactor ei_insert()

[Phil Sutter <phil@nwl.cc>]

On Tue, Jan 28, 2020 at 04:42:17PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 28, 2020 at 03:14:16PM +0100, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Tue, Jan 28, 2020 at 01:23:12PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Jan 23, 2020 at 03:30:49PM +0100, Phil Sutter wrote:
> > [...]
> > > > +	if (!merge) {
> > > > +		errno = EEXIST;
> > > > +		return expr_binary_error(msgs, lei->expr, new->expr,
> > > > +					 "conflicting intervals specified");
> > > >  	}
> > > 
> > > Not your fault, but I think this check is actually useless given that
> > > the overlap check happens before (unless you consider to consolidate
> > > the insertion and the overlap checks in ei_insert).
> > 
> > That's interesting, indeed. What's more interesting is how
> > interval_cmp() works: I assumed it would just sort by start element when
> > in fact interval size is the prominent aspect.
> 
> I overlook that this is ordered by the size.

Me too. %)

> > In practice, this means my changes work only as long as all
> > intervals are of equal or decreasing size. Does it make sense to
> > uphold this ordering scheme?
> 
> I think if you change the ordering scheme to use the left side
> (instead of the size) your new logic will work fine. It will also make
> it probably easier to check for overlaps in the end.

I wondered if this sorting may be used (or even necessary) for prefixes
or something. If it's not mandatory, I think sorting differently would
indeed help. Anyway, it means back to drawing board for me and self-NACK
this series.

Thanks, Phil