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

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.

> 
> -- 
> :wq Claudio
> 
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> diff -u -p -r1.27 rtr_proto.c
> --- rtr_proto.c	9 Jan 2024 15:13:49 -0000	1.27
> +++ rtr_proto.c	10 Jan 2024 11:36:48 -0000
> @@ -246,7 +246,7 @@ log_rtr_type(enum rtr_pdu_type type)
>  	case ERROR_REPORT:
>  		return "error report";
>  	case ASPA:
> -		return "aspa pdu";
> +		return "aspa";
>  	default:
>  		snprintf(buf, sizeof(buf), "unknown %u", type);
>  		return buf;
> @@ -296,23 +296,31 @@ rtr_newmsg(struct rtr_session *rs, enum 
>  	return NULL;
>  }
>  
> +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?

>  /*
>   * Try to send an error PDU to cache, put connection into error
>   * state.
>   */
>  static void
> -rtr_send_error(struct rtr_session *rs, enum rtr_error err, char *msg,
> -    struct ibuf *pdu)
> +rtr_send_error(struct rtr_session *rs, struct ibuf *pdu, enum rtr_error err,
> +    const char *fmt, ...)
>  {
>  	struct ibuf *buf;
> +	va_list ap;
> +	char msg[REASON_LEN] = { 0 };
>  	size_t len = 0, mlen = 0;
>  
>  	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.

> -	} else
> -		memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
> +		va_end(ap);
> +	}
>  
>  	if (pdu != NULL) {
>  		ibuf_rewind(pdu);
> @@ -336,7 +344,7 @@ rtr_send_error(struct rtr_session *rs, e
>  	ibuf_close(&rs->w, buf);
>  
>  	log_warnx("rtr %s: sending error: %s%s%s", log_rtr(rs),
> -	    log_rtr_error(err), msg ? ": " : "", msg ? msg : "");
> +	    log_rtr_error(err), mlen > 0 ? ": " : "", msg); 
>  
>  	rtr_fsm(rs, RTR_EVNT_SEND_ERROR);
>  	return;