From: Klemens Nanni <kn@openbsd.org>
Subject: Re: installer: move code into new stop_watchdog()
To: Andrew Hewus Fresh <andrew@afresh1.com>
Cc: tech@openbsd.org
Date: Thu, 14 Mar 2024 20:21:24 +0000

On Wed, Mar 13, 2024 at 07:40:08PM -0700, Andrew Hewus Fresh wrote:
> On Wed, Mar 13, 2024 at 07:35:36PM +0000, Klemens Nanni wrote:
> > We have {reset,start}_watchdog() which are only used in unattended upgrade
> > code, but stopping the background timer is done inline for all upgrades,
> > incl. interactive ones.
> > 
> > Below would relocate the code without logical change, but makes it obvious
> > how start/stop semantics are off:
> > 
> > | case $MODE in
> > | install)	do_install;;
> > |-upgrade)	do_upgrade;;
> > |+upgrade)	do_upgrade
> > |+		stop_watchdog;;
> > | esac
> > 
> > So only stop if UU=true, exactly like start_watchdog() is called in the
> > if/else block further up besaid case block.
> > 
> > Then start/stop code fits on one screen, is strictly limited to unattended
> > upgrades and hopefully more obviously so.
> > 
> > Feedback? Objection? OK?
> 
> This seems nice.  One question below.
> 
> 
> > 
> > 
> > Index: install.sub
> > ===================================================================
> > RCS file: /cvs/src/distrib/miniroot/install.sub,v
> > diff -u -p -r1.1261 install.sub
> > --- install.sub	5 Mar 2024 19:47:58 -0000	1.1261
> > +++ install.sub	13 Mar 2024 19:11:29 -0000
> > @@ -3477,11 +3477,6 @@ do_upgrade() {
> >  
> >  	# Perform final steps common to both an install and an upgrade.
> >  	finish_up
> > -	if [ -f /tmp/wdpid ]; then
> > -		kill -KILL "$(</tmp/wdpid)" 2>/dev/null
> > -		# do not bother waiting
> > -		rm -f /tmp/wdpid
> > -	fi
> >  }
> >  
> >  check_unattendedupgrade() {
> > @@ -3528,6 +3523,15 @@ start_watchdog() {
> >  	set +m
> >  }
> >  
> > +# Stop the background timer.
> > +stop_watchdog() {
> > +	if [ -f /tmp/wdpid ]; then
> > +		kill -KILL "$(</tmp/wdpid)" 2>/dev/null
> > +		# do not bother waiting
> > +		rm -f /tmp/wdpid
> > +	fi
> > +}
> > +
> >  # return if we only want internal functions
> >  [[ -n $FUNCS_ONLY ]] && return
> >  
> > @@ -3707,6 +3711,8 @@ case $MODE in
> >  install)	do_install;;
> >  upgrade)	do_upgrade;;
> >  esac
> > +
> > +$UU && stop_watchdog
> 
> Should this be in the `elif $UU` block where we `start_watchdog` in the
> parent process?  
> 
> It seems like
> 
>     start_watchdog
>     get_responsefile
>     do_autoinstall
>     stop_watchdog
> 
> would be clearer, but maybe I'm missing why it doesn't belong in the
> parent process.  I have spent time in far too many timezones in the last
> couple weeks, so it wouldn't surprise me if there was something I am
> missing.

Should work equally well, reads better as you outline it, but isn't as
obvious and a small step as moving the code out of the very end of
do_upgrade() right after its only caller.

We made mistakes with this parent/child dance and variable handling before.
We can always improve it with later diffs.

> 
> It's also not clear to me why only UU has a watchdog and not AI, but I
> think that's a separate question.

Yes.

> 
> In either case, I think this makes it easier to follow.  OK afresh1@
> 
> 
> >  # In case of autoinstall, this is a second process of install.sub.
> >  # Exiting here returns to the original process, which handles the
>