From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: sendHoldTimer cleanup
To: tech@openbsd.org
Date: Tue, 26 Mar 2024 12:30:37 +1100

On Mon, Mar 25, 2024 at 01:37:08PM +0100, Claudio Jeker wrote:
> While reviewing draft-ietf-idr-bgp-sendholdtimer-03 I noticed that our
> implementation of SendHoldTimer could be a bit better. Especially we need
> to be careful to disable the timer when HoldTime is negotiated to 0.
> 
> Now the draft itself only applies the SendHoldTimer when in state
> Established while we do it all the time. This does not really matter
> IMO that's why I did not add the extra check for that.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.466 session.c
> --- session.c	22 Mar 2024 15:41:34 -0000	1.466
> +++ session.c	25 Mar 2024 11:07:36 -0000
> @@ -58,6 +58,7 @@ void	session_sighdlr(int);
>  int	setup_listeners(u_int *);
>  void	init_peer(struct peer *);
>  void	start_timer_holdtime(struct peer *);
> +void	start_timer_sendholdtime(struct peer *);
>  void	start_timer_keepalive(struct peer *);
>  void	session_close_connection(struct peer *);
>  void	change_state(struct peer *, enum session_state, enum session_events);
> @@ -836,6 +837,20 @@ start_timer_holdtime(struct peer *peer)
>  }
>  
>  void
> +start_timer_sendholdtime(struct peer *peer)
> +{
> +	uint16_t holdtime = INTERVAL_HOLD;
> +
> +	if (peer->holdtime > INTERVAL_HOLD)
> +		holdtime = peer->holdtime;
> +
> +	if (peer->holdtime > 0)
> +		timer_set(&peer->timers, Timer_SendHold, holdtime);
> +	else
> +		timer_stop(&peer->timers, Timer_SendHold);
> +}
> +
> +void
>  start_timer_keepalive(struct peer *peer)
>  {
>  	if (peer->holdtime > 0)
> @@ -1929,10 +1944,7 @@ session_dispatch_msg(struct pollfd *pfd,
>  			return (1);
>  		}
>  		p->stats.last_write = getmonotime();
> -		if (p->holdtime > 0)
> -			timer_set(&p->timers, Timer_SendHold,
> -			    p->holdtime < INTERVAL_HOLD ? INTERVAL_HOLD :
> -			    p->holdtime);
> +		start_timer_sendholdtime(p);
>  		if (p->throttled && p->wbuf.queued < SESS_MSG_LOW_MARK) {
>  			if (imsg_rde(IMSG_XON, p->conf.id, NULL, 0) == -1)
>  				log_peer_warn(&p->conf, "imsg_compose XON");
>