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

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Tue, Apr 12, 2022 at 04:47:11PM +0200, Pablo Neira Ayuso wrote:
[...]
> diff --git a/src/intervals.c b/src/intervals.c
> index 16debf9cd4be..65e0531765a6 100644
> --- a/src/intervals.c
> +++ b/src/intervals.c
> @@ -255,6 +255,262 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set,
>  	return 0;
>  }
>  
> +static void remove_elem(struct expr *prev, struct set *set, struct expr *purge)
> +{
> +	struct expr *clone;
> +
> +	if (!(prev->flags & EXPR_F_REMOVE)) {

Does prev->flags ever contain EXPR_F_REMOVE? (See below.)

> +		if (prev->flags & EXPR_F_KERNEL) {
> +			clone = expr_clone(prev);
> +			list_move_tail(&clone->list, &purge->expressions);
> +		} else {
> +			list_del(&prev->list);
> +			expr_free(prev);
> +		}
> +	}
> +}
> +
> +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.

> +	expr_free(prev->key->left);
> +	prev->key->left = expr_get(i->key->right);
> +	mpz_add_ui(prev->key->left->value, prev->key->left->value, 1);
> +	list_move(&prev->list, &init->expressions);
> +}
[...]
> +static int setelem_delete(struct list_head *msgs, struct set *set,
> +			  struct expr *init, struct expr *purge,
> +			  struct expr *elems, unsigned int debug_mask)
> +{
> +	struct expr *i, *next, *prev = NULL;
> +	struct range range, prev_range;
> +	int err = 0;
> +	mpz_t rop;
> +
> +	mpz_init(prev_range.low);
> +	mpz_init(prev_range.high);
> +	mpz_init(range.low);
> +	mpz_init(range.high);
> +	mpz_init(rop);
> +
> +	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.
> +
> +		if (mpz_cmp(prev_range.low, range.low) == 0 &&
> +		    mpz_cmp(prev_range.high, range.high) == 0) {
> +			if (!(prev->flags & EXPR_F_REMOVE) &&
> +			    i->flags & EXPR_F_REMOVE) {
> +				list_move_tail(&prev->list, &purge->expressions);
> +				list_del(&i->list);
> +				expr_free(i);
> +			}
> +		} else if (set->automerge &&
> +			   setelem_adjust(set, init, purge, &prev_range, &range, prev, i) < 0) {
> +			expr_error(msgs, i, "element does not exist");
> +			err = -1;
> +			goto err;
> +		}
> +		prev = NULL;

The code above may set EXPR_F_REMOVE in 'prev', but AFAICT 'prev' is not
revisited within and cleared before next iteration.

Cheers, Phil