From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: allow quick open on startup
To: tech@openbsd.org
Date: Thu, 23 Jul 2026 12:47:07 +0200

On Thu, Jul 23, 2026 at 12:07:24PM +0200, Claudio Jeker wrote:
> With the change to the IdleHoldTimer I did a few days ago the regress
> tests starterd to blow up. I remember I tested them but I probably screwed
> up something on my side.
> 
> The problem is that on startup we keep seesion in Idle for
> SESSION_CLEAR_DELAY seconds. In that state no connection is accepted.
> So this applies the same logic and moves the peer directly to Active
> and adds an extra hack into the bgp_fsm EVNT_START handling to reduce
> the Timer_ConnectRetry to SESSION_CLEAR_DELAY.
> 
> In init_peer() instead of passing via the timer just call the bgp_fsm().
> Also there is no need to stop a timer that is not running.
> 
> The initial delay I added here was to ensure that the async bits of the
> initial config load have finished in the RDE. This just prevents extra
> work on startup for setups with big configs and many peers. Maybe the RDE
> should signal the SE when finished so we could use that signal to start all
> sessions on startup.
> 
> With the diff below regress is passing again.
> -- 
> :wq Claudio
> 
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.538 session.c
> --- session.c	21 Jul 2026 08:23:44 -0000	1.538
> +++ session.c	23 Jul 2026 09:14:32 -0000
> @@ -593,10 +593,8 @@ init_peer(struct peer *p, struct bgpd_co
>  	peer_cnt++;
>  
>  	change_state(p, STATE_IDLE, EVNT_NONE);
> -	if (p->conf.down)
> -		timer_stop(&p->timers, Timer_IdleHold); /* no autostart */
> -	else
> -		timer_set(&p->timers, Timer_IdleHold, SESSION_CLEAR_DELAY);
> +	if (!p->conf.down)
> +		bgp_fsm(p, EVNT_START, NULL);
>  
>  	p->stats.last_updown = getmonotime();
>  
> Index: session_bgp.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v
> diff -u -p -r1.11 session_bgp.c
> --- session_bgp.c	21 Jul 2026 08:23:44 -0000	1.11
> +++ session_bgp.c	23 Jul 2026 09:10:01 -0000
> @@ -1554,16 +1554,19 @@ bgp_fsm(struct peer *peer, enum session_
>  			timer_stop(&peer->timers, Timer_Keepalive);
>  			timer_stop(&peer->timers, Timer_IdleHold);
>  
> -			if (!peer->depend_ok)
> +			if (!peer->depend_ok) {
>  				timer_stop(&peer->timers, Timer_ConnectRetry);
> -			else if (peer->conf.passive || peer->conf.template) {
> +			} else if (peer->conf.passive || peer->conf.template) {
>  				change_state(peer, STATE_ACTIVE, event);
>  				timer_stop(&peer->timers, Timer_ConnectRetry);
> -			} else if (peer->IdleHoldTime ==
> -			    INTERVAL_IDLE_HOLD_INITIAL) {
> +			} else if (peer->prev_state == STATE_NONE ||
> +			    peer->IdleHoldTime == INTERVAL_IDLE_HOLD_INITIAL) {
> +				u_int holdtime = INTERVAL_IDLE_HOLD_INITIAL;

I would add an empty line after the declaration.

ok tb either way.

> +				if (peer->prev_state == STATE_NONE)
> +					holdtime = SESSION_CLEAR_DELAY;
>  				change_state(peer, STATE_ACTIVE, event);
>  				timer_set(&peer->timers, Timer_ConnectRetry,
> -				    INTERVAL_IDLE_HOLD_INITIAL);
> +				    holdtime);
>  			} else {
>  				change_state(peer, STATE_CONNECT, event);
>  				timer_set(&peer->timers, Timer_ConnectRetry,
>