On Thu, May 01, 2025 at 03:41:25PM +0000, Klemens Nanni wrote:
> Now we show all valid root disks and pick the first one, i.e. the
> alphanumerically lowest value, as default:
> 
> 	Available disks are: sd0 sd1.
> 	Which disk is the root disk? ('?' for details) [sd0] ?
> 	    sd0: VirtIO, Block Device  (0.8G)
> 	    sd1: VirtIO, Block Device  (3.0G)
> 	Available disks are: sd0 sd1.
> 	Which disk is the root disk? ('?' for details) [sd0] 


I like this.  Most of my "small" USB sticks seem to be 8G so this
probably won't help me terribly much in that case, but in the VMM case
it seems nicer.

I did hunt down a 64G stick and it shows up as 0.1G which seems
reaonable for `disklabel -pg`.

The implementation looks reasonable to me.  If there are no objections,
I think it is a nice usability improvement, OK afresh1@


> 
> Finding the "best" disk as default is hard, but I reckon we can prefer some
> disks more than others;  the example above is from a common test routine of mine:
> 
> 	# vmctl start -d install77.img -d disk.img ...
> 
> But whenever install media, small USB sticks or softraid(4) keydisks attach
> before you actual disk, defaulting to sd0 is most certainly not what you want.
> 
> An easy rule of thumb that works great for me is to reshuffle the list of valid
> root disks such that small ones come last.  This is easy to code and cannot
> break anything, so now you get:
> 
> 	Which disk is the root disk? ('?' for details) [sd1] 
> 
> Do this only on install so upgrade behaviour for tiny root disks with big data
> disks remains unchanged.
> 
> 
> NB: _default is an odd name as the variable holds a list of devices.
>     _ask() gets them, takes its first and second argument as question
>     as default answer, respectively, and ignores the rest.
> 
>     But further the variable is treated as list, exactly the same way
>     I do upfront now, so better call it _rootdisks. 
> 
> 
> Feedback? OK?
> 
> Index: install.sub
> ===================================================================
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> diff -u -p -r1.1269 install.sub
> --- install.sub	6 Apr 2025 11:54:36 -0000	1.1269
> +++ install.sub	1 May 2025 15:37:04 -0000
> @@ -262,7 +262,7 @@ dmesgtail() {
>  # Device related functions
>  # ------------------------------------------------------------------------------
>  
> -# Show device name, info, NAA and size for the provided list of disk devices.
> +# Show device name, info, NAA and size in gigabytes for the provided disks.
>  # Create device nodes as needed and cleanup afterwards.
>  diskinfo() {
>  	local _d _i _n _s
> @@ -2429,12 +2429,22 @@ is_rootdisk() {
>  
>  # Get global root information. ie. ROOTDISK, ROOTDEV and SWAPDEV.
>  get_rootinfo() {
> -	local _default=$(get_dkdevs_root) _dkdev
> +	local _rootdisks=$(get_dkdevs_root) _dkdev
>  	local _q="Which disk is the root disk? ('?' for details)"
>  
> +	
> +	if [[ $MODE == install ]]; then
> +		# Prefer disks bigger than 1G as small ones tend to be install
> +		# media, softraid(4) key disks or tiny USB sticks or SD cards.
> +		for _dkdev in $_rootdisks; do
> +			diskinfo $_dkdev | grep -q '(0\..*G)$' &&
> +				_rootdisks="$(rmel "$_dkdev" $_rootdisks) $_dkdev"
> +		done
> +	fi
> +
>  	while :; do
>  		echo "Available disks are: $(get_dkdevs_root | sed 's/^$/none/')."
> -		_ask "$_q" $_default || continue
> +		_ask "$_q" $_rootdisks || continue
>  		case $resp in
>  		"?")	diskinfo $(get_dkdevs);;
>  		'')	;;
> 

-- 
andrew

A hacker does for love what others would not do for money.