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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

On Thu, Apr 16, 2026 at 12:04:33AM +0200, Stefano Brivio wrote:
> 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?

No, this one's intended to stay in pesto, at least to begin with.
This is in the parsing path, not the rule insertion path.  So once
pesto is involved this will die() *in pesto*, which is correct[0].
When we receive new rules from pesto in passt we bypass this giong
direct to fwd_rule_add().

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]