From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: cleanup listerner handling in session.c
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Thu, 13 Nov 2025 13:13:13 +0100

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.

> 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