From: Florian Obser <florian@openbsd.org>
Subject: Re: [PATCH 1/4] evtimer_del(): remove redundant pending checks in sbin/
To: Bjorn Ketelaars <bket@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 21 Oct 2026 21:19:23 +0200

OK florian

I've tested dhcpleased & slaacd and only compile tested iked &
dhcp6leased.

On 2026-10-16 14:32 +02, Bjorn Ketelaars <bket@openbsd.org> wrote:
> While reading event(3), I noticed that it says event_del() has no effect
> if an event “has already executed or has never been added”. The manual
> describes evtimer_del() as an abbreviation for the same event API, so
> checking evtimer_pending() immediately before deleting an initialized
> timer is unnecessary.
>
> This patch removes those checks from timer state transitions in
> dhcp6leased, dhcpleased, and slaacd, and from timer replacement in iked.
>
> These are mechanical cleanups; timer scheduling and callback behavior
> are unchanged.
>
> Comments/OK?
>
> Index: sbin/dhcp6leased/engine.c
> ===================================================================
> RCS file: /cvs/src/sbin/dhcp6leased/engine.c,v
> diff -u -p -r1.42 engine.c
> --- sbin/dhcp6leased/engine.c	4 Aug 2026 12:50:43 -0000	1.42
> +++ sbin/dhcp6leased/engine.c	16 Oct 2026 12:07:35 -0000
> @@ -1212,10 +1212,9 @@ state_transition(struct dhcp6leased_ifac
>  	    "?" : if_name, if_state_name[old_state], if_state_name[new_state],
>  	    iface->timo.tv_sec);
>  
> -	if (iface->timo.tv_sec == -1) {
> -		if (evtimer_pending(&iface->timer, NULL))
> -			evtimer_del(&iface->timer);
> -	} else
> +	if (iface->timo.tv_sec == -1)
> +		evtimer_del(&iface->timer);
> +	else
>  		evtimer_add(&iface->timer, &iface->timo);
>  }
>  
> Index: sbin/dhcpleased/engine.c
> ===================================================================
> RCS file: /cvs/src/sbin/dhcpleased/engine.c,v
> diff -u -p -r1.70 engine.c
> --- sbin/dhcpleased/engine.c	26 Aug 2026 17:25:13 -0000	1.70
> +++ sbin/dhcpleased/engine.c	16 Oct 2026 12:07:35 -0000
> @@ -1544,10 +1544,9 @@ state_transition(struct dhcpleased_iface
>  	    if_state_name[old_state], if_state_name[new_state],
>  	    iface->timo.tv_sec);
>  
> -	if (iface->timo.tv_sec == -1) {
> -		if (evtimer_pending(&iface->timer, NULL))
> -			evtimer_del(&iface->timer);
> -	} else
> +	if (iface->timo.tv_sec == -1)
> +		evtimer_del(&iface->timer);
> +	else
>  		evtimer_add(&iface->timer, &iface->timo);
>  }
>  
> Index: sbin/iked/timer.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/timer.c,v
> diff -u -p -r1.13 timer.c
> --- sbin/iked/timer.c	13 Sep 2016 10:49:52 -0000	1.13
> +++ sbin/iked/timer.c	16 Oct 2026 12:07:35 -0000
> @@ -37,8 +37,7 @@ void
>  timer_set(struct iked *env, struct iked_timer *tmr,
>      void (*cb)(struct iked *, void *), void *arg)
>  {
> -	if (evtimer_initialized(&tmr->tmr_ev) &&
> -	    evtimer_pending(&tmr->tmr_ev, NULL))
> +	if (evtimer_initialized(&tmr->tmr_ev))
>  		evtimer_del(&tmr->tmr_ev);
>  
>  	tmr->tmr_env = env;
> Index: sbin/slaacd/engine.c
> ===================================================================
> RCS file: /cvs/src/sbin/slaacd/engine.c,v
> diff -u -p -r1.105 engine.c
> --- sbin/slaacd/engine.c	4 Aug 2026 12:48:01 -0000	1.105
> +++ sbin/slaacd/engine.c	16 Oct 2026 12:07:35 -0000
> @@ -1029,10 +1029,9 @@ iface_state_transition(struct slaacd_ifa
>  		    if_state_name(new_state), iface->timo.tv_sec);
>  	}
>  
> -	if (iface->timo.tv_sec == -1) {
> -		if (evtimer_pending(&iface->timer, NULL))
> -			evtimer_del(&iface->timer);
> -	} else
> +	if (iface->timo.tv_sec == -1)
> +		evtimer_del(&iface->timer);
> +	else
>  		evtimer_add(&iface->timer, &iface->timo);
>  }
>  
> @@ -1110,10 +1109,9 @@ void addr_proposal_state_transition(stru
>  		    proposal_state_name(new_state), addr_proposal->timo.tv_sec);
>  	}
>  
> -	if (addr_proposal->timo.tv_sec == -1) {
> -		if (evtimer_pending(&addr_proposal->timer, NULL))
> -			evtimer_del(&addr_proposal->timer);
> -	} else
> +	if (addr_proposal->timo.tv_sec == -1)
> +		evtimer_del(&addr_proposal->timer);
> +	else
>  		evtimer_add(&addr_proposal->timer, &addr_proposal->timo);
>  }
>  
> @@ -1186,10 +1184,9 @@ void dfr_proposal_state_transition(struc
>  		    proposal_state_name(new_state), dfr_proposal->timo.tv_sec);
>  	}
>  
> -	if (dfr_proposal->timo.tv_sec == -1) {
> -		if (evtimer_pending(&dfr_proposal->timer, NULL))
> -			evtimer_del(&dfr_proposal->timer);
> -	} else
> +	if (dfr_proposal->timo.tv_sec == -1)
> +		evtimer_del(&dfr_proposal->timer);
> +	else
>  		evtimer_add(&dfr_proposal->timer, &dfr_proposal->timo);
>  
>  }
> @@ -1263,10 +1260,9 @@ void rdns_proposal_state_transition(stru
>  		    proposal_state_name(new_state), rdns_proposal->timo.tv_sec);
>  	}
>  
> -	if (rdns_proposal->timo.tv_sec == -1) {
> -		if (evtimer_pending(&rdns_proposal->timer, NULL))
> -			evtimer_del(&rdns_proposal->timer);
> -	} else
> +	if (rdns_proposal->timo.tv_sec == -1)
> +		evtimer_del(&rdns_proposal->timer);
> +	else
>  		evtimer_add(&rdns_proposal->timer, &rdns_proposal->timo);
>  }
>  
>

-- 
In my defence, I have been left unsupervised.