From: Andrew Hewus Fresh <andrew@afresh1.com>
Subject: Re: installer: move code into new stop_watchdog()
To: tech@openbsd.org
Date: Wed, 13 Mar 2024 19:40:08 -0700

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.

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

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