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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Apr 13, 2022 at 04:02:45PM +0200, Phil Sutter wrote:
> 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?

Right, the other way around actually.

> 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.

prev never has EXPR_F_REMOVE, so it points to an existing element.