From: Andrew Hewus Fresh <andrew@afresh1.com>
Subject: Re: installer: sets: do not offer http/nfs without network
To: Klemens Nanni <kn@openbsd.org>
Cc: OpenBSD tech <tech@openbsd.org>
Date: Thu, 8 May 2025 18:00:05 -0700

On Thu, May 08, 2025 at 08:47:34AM +0000, Klemens Nanni wrote:
> 03.05.2025 22:21, Klemens Nanni пишет:
> > 03.05.2025 05:30, Andrew Hewus Fresh пишет:
> >> Does this change to prefer cd0 over http if both cddev and network are
> >> available?  I'm not sure I like that choice.  I like the existing order
> >> of http, cd, nfs, disk.
> > 
> > No, both -current and my diff follow the same order of first two defaults:
> > 
> > 1. $CGI_METHOD - whatever ftplist.o.o remembers, if at all, if there's internet
> > 2. cd0 - if there is a CD present
> > 
> > Only then is networking considered and my diff changes behaviour.
> > 
> > So whether or not HTTP is actually possible, once cd0 attached, it always is the
> > default unless ftplist.o.o already said otherwise.
> > 
> > The installer picks HTTP over CD if and only if /mnt/etc/installurl exists, i.e.
> > on upgrades where users did not remove the file.
> > 
> > 
> > It could make sense to prefer HTTP over CD for install as well, but that's another diff.
> 
> Ping.  Same diff rebased after the recent commit.

I still think that before this patch, in the `ifconfig netboot` case, it
preferred http over cd0, and after it prefers cd0 over http.

If that is OK with folks, then it is (grudgingly) OK with me.



> ---
> 
> Offline installs/upgrades, i.e. when there are no interfaces, still default
> to fetching sets over HTTP, which makes no sense (minimal reproducer):
> 
> 	# vmctl create -1g disk; vmctl start -c -b /bsd.rd -d disk offline
> 	...
> 	Let's install the sets!
> 	Location of sets? (disk http nfs or 'done') [http] 
> 
> At this point, interfaces are configured already and state is basically known,
> yet NFS and HTTP are offered unconditionally.
> 
> Skip these two methodos unless there is a physical interface around:
> 
> 	Let's install the sets!
> 	Location of sets? (disk or 'done') [disk] 
> 
> Shorten and clarify the comments while here.
> 
> 
> NB: The shell idiom ${var:=val} means "set $var to 'val' unless already non-empty".
> 
>     This diff reads much better with `sdiff -w165 old new' than in unified form.
> 
> 
> Feedback? OK?
> 
> Index: install.sub
> ===================================================================
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> diff -u -p -r1.1270 install.sub
> --- install.sub	4 May 2025 12:32:41 -0000	1.1270
> +++ install.sub	8 May 2025 08:46:13 -0000
> @@ -2806,27 +2806,30 @@ feed_random() {
>  # selects from that location. Repeat as many times as the user needs to get all
>  # desired sets.
>  install_sets() {
> -	local _cddevs=$(get_cddevs) _d _im _locs="disk http" _src
> +	# Default to method recorded last time, if any.
> +	local _cddevs=$(get_cddevs) _d=$CGI_METHOD _im _locs=disk _src
>  
> -	echo
> +	[[ -n $_cddevs ]] && : ${_d:=cd0}
>  
> -	# Set default location to method recorded last time.
> -	_d=$CGI_METHOD
> +	# Offer network methods if there are physical interfaces.
> +        if [[ -n $(get_ifs) ]]; then
> +		_locs="$_locs http"
>  
> -	# Set default location to HTTP in case we netbooted.
> -	ifconfig netboot >/dev/null 2>&1 && : ${_d:=http}
> +		# Default to HTTP if netbooted.
> +		[[ -n $(get_ifs netboot) ]] && : ${_d:=http}
>  
> -	# Set default location to HTTP if installurl(5) exists.
> -	[[ -s /mnt/etc/installurl ]] && _d=http
> +		# Prefer HTTP to any existing default if installurl(5) exists.
> +		[[ -s /mnt/etc/installurl ]] && _d=http
>  
> -	# Set default location to the first cdrom device if any are found.
> -	[[ -n $_cddevs ]] && : ${_d:=cd0}
> +		# Offer NFS if the ramdisk supports it.
> +		[[ -x /sbin/mount_nfs ]] && _locs="$_locs nfs"
>  
> -	# Add NFS to set locations if the boot kernel supports it.
> -	[[ -x /sbin/mount_nfs ]] && _locs="$_locs nfs"
> +		# Default to HTTP.
> +		: ${_d:=http}
> +	fi
>  
> -	# In case none of the above applied, set HTTP as default location.
> -	: ${_d:=http}
> +	# Without CD and network, disk is the only option.
> +	: ${_d:=disk}
>  
>  	# If the default location set so far is not one of the cdrom devices or
>  	# is not in the list of valid locations, set a sane default.
> @@ -2836,7 +2839,7 @@ install_sets() {
>  		done
>  	fi
>  
> -	echo "Let's $MODE the sets!"
> +	echo "\nLet's $MODE the sets!"
>  	while :; do
>  		# Get list of cdroms again in case one just got plugged in.
>  		_cddevs=$(get_cddevs)
>