Re: [PATCH v2 11/23] fwd: Improve error handling in fwd_rule_add()

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 10 Apr 2026 11:02:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> fwd_rule_add() sanity checks the given rule, however all errors are fatal:
> either they're assert()s in the case of things that callers should have
> already verified, or die()s if we run out of space for the new rule.
> 
> This won't suffice any more when we allow rule updates from a
> configuration client.  We don't want to trust the input we get from
> the client any more than we have to.
> 
> Replace the assert()s and die()s with a return value.  Also include warn()s
> so that the user gets a more specific idea of the problem in the logs or
> stderr.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c     | 15 ++++++++++++---
>  fwd.c      | 41 +++++++++++++++++++++++++++++++----------
>  fwd.h      |  2 +-
>  fwd_rule.c |  3 +--
>  fwd_rule.h |  1 +
>  5 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index b871646f..5c913820 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -157,6 +157,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
>  		.proto = proto,
>  		.flags = flags,
>  	};
> +	char rulestr[FWD_RULE_STRLEN];
>  	unsigned delta = to - first;
>  	unsigned base, i;
>  
> @@ -207,20 +208,28 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
>  				rulev.addr = inany_loopback4;
>  				fwd_rule_conflict_check(&rulev,
>  							fwd->rules, fwd->count);
> -				fwd_rule_add(fwd, &rulev);
> +				if (fwd_rule_add(fwd, &rulev) < 0)
> +					goto fail;
>  			}
>  			if (c->ifi6) {
>  				rulev.addr = inany_loopback6;
>  				fwd_rule_conflict_check(&rulev,
>  							fwd->rules, fwd->count);
> -				fwd_rule_add(fwd, &rulev);
> +				if (fwd_rule_add(fwd, &rulev) < 0)
> +					goto fail;
>  			}
>  		} else {
>  			fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
> -			fwd_rule_add(fwd, &rule);
> +			if (fwd_rule_add(fwd, &rule) < 0)
> +				goto fail;
>  		}
>  		base = i - 1;
>  	}
> +	return;
> +
> +fail:
> +	die("Unable to add rule %s",
> +	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));

Similar to my comment to 10/23: I expected this die() to be gone by the
end of the series, but it's still there. Is it just a placeholder die()
that will go once pesto(1) is added?

-- 
Stefano