From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: rework session_notification()
To: tech@openbsd.org
Date: Tue, 16 Jan 2024 12:27:33 +0100

On Tue, Jan 16, 2024 at 12:00:00PM +0100, Claudio Jeker wrote:
> This diff is a next step towards new ibuf and imsg API.
> 
> This converts session.c session_notification() to take an ibuf and by that
> switches IMSG_UPDATE_ERR to the new API.
> 
> This adds a bit of a hack to parse_notification() since the parser code
> still does not use ibufs -- this is a much harder undertaking that it
> looks like.

ok tb

Two small comments below.

> Index: logmsg.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/logmsg.c,v
> diff -u -p -r1.10 logmsg.c
> --- logmsg.c	14 Oct 2023 09:46:14 -0000	1.10
> +++ logmsg.c	16 Jan 2024 10:45:24 -0000
> @@ -133,7 +133,7 @@ log_statechange(struct peer *peer, enum 
>  
>  void
>  log_notification(const struct peer *peer, uint8_t errcode, uint8_t subcode,
> -    u_char *data, uint16_t datalen, const char *dir)
> +    struct ibuf *data, const char *dir)

This doesn't use data at all. I assume it will be used once the XXX in
parse_notification() is resolved.

>  {
>  	char		*p;
>  	const char	*suberrname = NULL;
> Index: session.c
> ===================================================================

[...]

> @@ -1674,24 +1675,41 @@ session_update(uint32_t peerid, void *da
>  }
>  
>  void
> +session_notification_data(struct peer *p, uint8_t errcode, uint8_t subcode,
> +    void *data, size_t datalen)
> +{
> +	struct ibuf ibuf;
> +
> +	ibuf_from_buffer(&ibuf, data, datalen);
> +	session_notification(p, errcode, subcode, &ibuf);
> +}
> +
> +void
>  session_notification(struct peer *p, uint8_t errcode, uint8_t subcode,
> -    void *data, ssize_t datalen)
> +    struct ibuf *ibuf)
>  {
>  	struct bgp_msg		*buf;
>  	int			 errs = 0;
> +	size_t			 datalen = 0;
>  
>  	if (p->stats.last_sent_errcode)	/* some notification already sent */
>  		return;
>  
> -	log_notification(p, errcode, subcode, data, datalen, "sending");
> +	log_notification(p, errcode, subcode, ibuf, "sending");
>  
>  	/* cap to maximum size */
> -	if (datalen > MAX_PKTSIZE - MSGSIZE_NOTIFICATION_MIN) {
> -		log_peer_warnx(&p->conf,
> -		    "oversized notification, data trunkated");
> -		datalen = MAX_PKTSIZE - MSGSIZE_NOTIFICATION_MIN;
> +	if (ibuf != NULL) {
> +		if (ibuf_size(ibuf) >
> +		    MAX_PKTSIZE - MSGSIZE_NOTIFICATION_MIN) {
> +			log_peer_warnx(&p->conf,
> +			    "oversized notification, data trunkated");
> +			ibuf_truncate(ibuf, MAX_PKTSIZE -
> +			    MSGSIZE_NOTIFICATION_MIN);
> +		}
> +		datalen = ibuf_size(ibuf);
>  	}
>  
> +

This creates a double blank line

>  	if ((buf = session_newmsg(NOTIFICATION,
>  	    MSGSIZE_NOTIFICATION_MIN + datalen)) == NULL) {
>  		bgp_fsm(p, EVNT_CON_FATAL);