From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: better rtr_send_error()
To: tech@openbsd.org
Date: Wed, 10 Jan 2024 17:01:31 +0100

On Wed, Jan 10, 2024 at 04:43:02PM +0100, Claudio Jeker wrote:
> On Wed, Jan 10, 2024 at 04:08:00PM +0100, Theo Buehler wrote:
> > On Wed, Jan 10, 2024 at 02:38:44PM +0100, Claudio Jeker wrote:
> > > This diff removes the log_warnx() before the rtr_send_error() and depends
> > > on rtr_send_error() to do this logging (which it kind of does).
> > > Also improve the error messages by allowing a format string for the
> > > message passed to rtr_send_error().
> > 
> > ok as is, two small questions/comments inline.
> > 
> > > +static void rtr_send_error(struct rtr_session *, struct ibuf *, enum rtr_error,
> > > +    const char *, ...) __attribute__((__format__ (printf, 4, 5)));
> > > +
> > 
> > Can the attribute not be added to the function definition itself or
> > does this make some compilers unhappy?
> 
> clang is unhappy with that.

Of course...

> > >  	rs->last_sent_error = err;
> > > -	if (msg != NULL) {
> > > +	memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
> > > +	if (fmt != NULL) {
> > > +		va_start(ap, fmt);
> > > +		vsnprintf(msg, sizeof(msg), fmt, ap);
> > >  		mlen = strlen(msg);
> > >  		strlcpy(rs->last_sent_msg, msg, sizeof(rs->last_sent_msg));
> > 
> > Not sure it makes things simpler, but can't you vsnprintf() directly
> > to the rs->last_sent_msg? It would avoid some extra copying/zeroing.
> 
> I wondered if we need a longer msg buffer but lets do that for now.
> Main goal here is to print something even for the "out of memory" case.
> Because of that I shuffled the log_warnx() up since the rtr_newmsg() will
> most probably fail as well in low memory situations.

That makes sense.

ok tb