Re: [PATCH nft,v4 7/7] intervals: support to partial deletion with automerge

[Phil Sutter <phil@nwl.cc>]

On Wed, Apr 13, 2022 at 03:13:30PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Apr 13, 2022 at 02:54:34PM +0200, Phil Sutter wrote:
[...]
> > > +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i,
> > > +			       struct expr *init)
> > > +{
> > > +	prev->flags &= EXPR_F_KERNEL;
> > 
> > This looks odd. You're intentionally stripping all flags other than
> > EXPR_F_KERNEL (if set)?
> > IIUC, you're just dropping EXPR_F_REMOVE if set. If so, explicit
> > 'prev->flags &= ~EXPR_F_REMOVE' is more clear, no?
> > Maybe it's also irrelevant after all WRT above question.
> 
> Yes, this should be prev->flags &= ~EXPR_F_KERNEL, I'll fix it.

Ah, OK!

> This element is moved to the list of elements to be added. This flag
> is irrelevant though at this stage, but in case you look at the list
> of elements to be added, you should not see EXPR_F_KERNEL there.

I guess none of the flags are relevant at this point anymore since your
code cleared them all and apparently passed testing? Or none of the
relevant ones were set, which is my suspicion with EXPR_F_REMOVE.

[...]
> > > +	list_for_each_entry_safe(i, next, &elems->expressions, list) {
> > > +		if (i->key->etype == EXPR_SET_ELEM_CATCHALL)
> > > +			continue;
> > > +
> > > +		range_expr_value_low(range.low, i);
> > > +		range_expr_value_high(range.high, i);
> > > +
> > > +		if (!prev && i->flags & EXPR_F_REMOVE) {
> > > +			expr_error(msgs, i, "element does not exist");
> > > +			err = -1;
> > > +			goto err;
> > > +		}
> > > +
> > > +		if (!(i->flags & EXPR_F_REMOVE)) {
> > > +			prev = i;
> > > +			mpz_set(prev_range.low, range.low);
> > > +			mpz_set(prev_range.high, range.high);
> > > +			continue;
> > > +		}
> > 
> > The loop assigns to 'prev' only if EXPR_F_REMOVE is not set.
> 
> Yes, this annotates is a element candidate to be removed.
> 
> The list of elements is merged-sorted, coming the element with
> EXPR_F_REMOVE before the element that needs to be removed.

The one with EXPR_F_REMOVE comes *after* the one to be removed, right?

My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set?
Maybe I miss something, but to me it looks like not although the code
expects it.

Cheers, Phil