On Tue, Jan 16, 2024 at 12:27:33PM +0100, Theo Buehler wrote:
> 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.

This is not affected by that XXX. The problem is more that we never
added the code to dump this binary blob. The XXX is more about the fact
that parse_notification() should use an ibuf in the first place.
We probably need a hexdump function of some sorts (iked has something that
could be lifted).
 
> >  {
> >  	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

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

-- 
:wq Claudio