From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: cleanup listerner handling in session.c
To: tech@openbsd.org
Date: Thu, 13 Nov 2025 13:16:52 +0100

On Thu, Nov 13, 2025 at 01:13:13PM +0100, Claudio Jeker wrote:
> On Thu, Nov 13, 2025 at 11:48:12AM +0100, Theo Buehler wrote:
> > On Thu, Nov 13, 2025 at 11:09:41AM +0100, Claudio Jeker wrote:
> > > I dislike how the listener handling in session.c RECONF_DONE is spread
> > > over multiple functions. Just move everything into one place
> > > (setup_listeners) and call it a day.
> > > 
> > > I think the result is much simpler to read.
> > 
> > Yes, it is much more pleasant. It is plausible but not entirely obvious
> > to me that none of the nconf->listen_addrs has ->reconf == RECONF_NONE.
> > 
> > If you are sure about that,
> 
> If you look at the IMSG_RECONF_LISTENER code you see:
> 
>                         if (la == NULL) {
>                                 if (nla.reconf != RECONF_REINIT)
>                                         fatalx("king bula sez: "
>                                             "expected REINIT");	
> 			...
>                                 la->reconf = RECONF_REINIT;
>                                 TAILQ_INSERT_TAIL(nconf->listen_addrs, la,
>                                     entry);
> 			}
> 
> It is the only place where listeners are added to nconf->listen_addrs.
> So none of them has ->reconf == RECONF_NONE.

Great, thanks. ok tb then.
> 
> > ok
> > 
> > > -- 
> > > :wq Claudio
> > > 
> > > Index: session.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> > > diff -u -p -r1.527 session.c
> > > --- session.c	30 Oct 2025 12:43:18 -0000	1.527
> > > +++ session.c	13 Nov 2025 09:40:03 -0000
> > > @@ -108,10 +108,24 @@ int
> > >  setup_listeners(u_int *la_cnt)
> > >  {
> > >  	int			 ttl = 255;
> > > -	struct listen_addr	*la;
> > > +	struct listen_addr	*la, *next;
> > >  	u_int			 cnt = 0;
> > >  
> > > -	TAILQ_FOREACH(la, conf->listen_addrs, entry) {
> > > +	/* add new listeners */
> > > +	TAILQ_CONCAT(conf->listen_addrs, nconf->listen_addrs, entry);
> > > +
> > > +	TAILQ_FOREACH_SAFE(la, conf->listen_addrs, entry, next) {
> > > +		/* check for and delete no longer used listeners */
> > > +		if (la->reconf == RECONF_NONE) {
> > > +			log_info("not listening on %s any more",
> > > +			    log_sockaddr((struct sockaddr *)
> > > +			    &la->sa, la->sa_len));
> > > +			TAILQ_REMOVE(conf->listen_addrs, la, entry);
> > > +			close(la->fd);
> > > +			free(la);
> > > +			continue;
> > > +		}
> > > +
> > >  		la->reconf = RECONF_NONE;
> > >  		cnt++;
> > >  
> > > @@ -1173,7 +1187,7 @@ session_dispatch_imsg(struct imsgbuf *im
> > >  	struct mrt		*mrt;
> > >  	struct imsgbuf		*i;
> > >  	struct peer		*p;
> > > -	struct listen_addr	*la, *next, nla;
> > > +	struct listen_addr	*la, nla;
> > >  	struct session_dependon	 sdon;
> > >  	struct bgpd_config	 tconf;
> > >  	uint32_t		 peerid;
> > > @@ -1342,25 +1356,8 @@ session_dispatch_imsg(struct imsgbuf *im
> > >  			copy_config(conf, nconf);
> > >  			merge_peers(conf, nconf);
> > >  
> > > -			/* delete old listeners */
> > > -			TAILQ_FOREACH_SAFE(la, conf->listen_addrs, entry,
> > > -			    next) {
> > > -				if (la->reconf == RECONF_NONE) {
> > > -					log_info("not listening on %s any more",
> > > -					    log_sockaddr((struct sockaddr *)
> > > -					    &la->sa, la->sa_len));
> > > -					TAILQ_REMOVE(conf->listen_addrs, la,
> > > -					    entry);
> > > -					close(la->fd);
> > > -					free(la);
> > > -				}
> > > -			}
> > > -
> > > -			/* add new listeners */
> > > -			TAILQ_CONCAT(conf->listen_addrs, nconf->listen_addrs,
> > > -			    entry);
> > > -
> > >  			setup_listeners(listener_cnt);
> > > +
> > >  			free_config(nconf);
> > >  			nconf = NULL;
> > >  			pending_reconf = 0;
> > > 
> > 
> 
> -- 
> :wq Claudio