[pve-devel] [PATCH qemu-server 1/2] fix #3010: add 'bootorder' parameter for better control of boot devices

[pve-devel] [PATCH qemu-server 1/2] fix #3010: add 'bootorder' parameter for better control of boot devices

[Stefan Reiter]

(also fixes #3011)

Deprecates the old 'boot' and 'bootdisk' options (they still work the
same, but will get removed if a user sets a 'bootorder' parameter and
ignored if both are set).

This allows a user to specify more than one disk in the boot order,
helping with newer versions of SeaBIOS/OVMF where disks without a
bootindex won't be initialized at all (breaks soft-raid and some LVM
setups).

This also allows specifying a bootindex for USB and hostpci devices,
which was not possible before. Floppy boot support is not supported in
the new model, but I doubt that will be a problem (AFAICT we can't even
attach floppy disks to a VM?).

Default behaviour is intended to stay the same, i.e. while new VMs will
receive the new 'bootorder' property, it will be set so the VM starts
the same as before (see get_default_bootorder).

The API is updated to handle the deprecation correctly, i.e. when
updating the 'bootorder' attribute, the old properties are removed
(would now be useless). When removing a device that is in the bootorder
list, it will be removed from the aforementioned. Note that non-existing
devices in the list will not cause an error - they will simply be
ignored - but it's still nice to not have them in there.

Includes a cfg2cmd test.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

The diff of the network part in config_to_command looks a bit weird, that's
because the indentation was off by one in the entire block.

 PVE/API2/Qemu.pm                |  32 +++++-
 PVE/CLI/qm.pm                   |   4 +-
 PVE/QemuServer.pm               | 178 ++++++++++++++++++++++++--------
 PVE/QemuServer/Drive.pm         |  29 ++++--
 PVE/QemuServer/PCI.pm           |   3 +-
 PVE/QemuServer/USB.pm           |  14 ++-
 test/cfg2cmd/bootorder.conf     |  16 +++
 test/cfg2cmd/bootorder.conf.cmd |  38 +++++++
 8 files changed, 254 insertions(+), 60 deletions(-)
 create mode 100644 test/cfg2cmd/bootorder.conf
 create mode 100644 test/cfg2cmd/bootorder.conf.cmd

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8da616a..f9be475 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -316,6 +316,7 @@ my $vmpoweroptions = {
 my $diskoptions = {
     'boot' => 1,
     'bootdisk' => 1,
+    'bootorder' => 1,
     'vmstatestorage' => 1,
 };
 
@@ -656,9 +657,9 @@ __PACKAGE__->register_method({
 		eval {
 		    $vollist = &$create_disks($rpcenv, $authuser, $conf, $arch, $storecfg, $vmid, $pool, $param, $storage);
 
-		    if (!$conf->{bootdisk}) {
-			my $firstdisk = PVE::QemuServer::Drive::resolve_first_disk($conf);
-			$conf->{bootdisk} = $firstdisk if $firstdisk;
+		    if (!$conf->{bootorder}) {
+			my $order = PVE::QemuServer::get_default_bootorder($conf);
+			$conf->{bootorder} = $order;
 		    }
 
 		    # auto generate uuid if user did not specify smbios1 option
@@ -1191,6 +1192,9 @@ my $update_vm_api  = sub {
 
 	    my $modified = {}; # record what $option we modify
 
+	    my @bootorder = PVE::Tools::split_list($conf->{bootorder}) if $conf->{bootorder};
+	    my $bootorder_deleted = grep {$_ eq 'bootorder'} @delete;
+
 	    foreach my $opt (@delete) {
 		$modified->{$opt} = 1;
 		$conf = PVE::QemuConfig->load_config($vmid); # update/reload
@@ -1205,6 +1209,13 @@ my $update_vm_api  = sub {
 		my $is_pending_val = defined($conf->{pending}->{$opt});
 		delete $conf->{pending}->{$opt};
 
+		# remove from bootorder if necessary
+		if (!$bootorder_deleted && @bootorder && grep {$_ eq $opt} @bootorder) {
+		    @bootorder = grep {$_ ne $opt} @bootorder;
+		    $conf->{pending}->{bootorder} = join(',', @bootorder);
+		    $modified->{bootorder} = 1;
+		}
+
 		if ($opt =~ m/^unused/) {
 		    my $drive = PVE::QemuServer::parse_drive($opt, $val);
 		    PVE::QemuConfig->check_protection($conf, "can't remove unused disk '$drive->{file}'");
@@ -1283,6 +1294,21 @@ my $update_vm_api  = sub {
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} else {
 		    $conf->{pending}->{$opt} = $param->{$opt};
+
+		    if ($opt eq 'bootorder') {
+			for my $dev (PVE::Tools::split_list($param->{$opt})) {
+			    my $exists = $conf->{$dev} || $conf->{pending}->{$dev};
+			    my $deleted = grep {$_ eq $dev} @delete;
+			    die "invalid bootorder: device '$dev' does not exist'\n"
+				if !$exists || $deleted;
+			}
+
+			# remove legacy boot order settings if new one set
+			PVE::QemuConfig->add_to_pending_delete($conf, "boot")
+			    if $conf->{boot};
+			PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
+			    if $conf->{bootdisk};
+		    }
 		}
 		PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
 		PVE::QemuConfig->write_config($vmid, $conf);
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 282fa86..ff16636 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -656,8 +656,8 @@ __PACKAGE__->register_method ({
 
 	    # reload after disks entries have been created
 	    $conf = PVE::QemuConfig->load_config($vmid);
-	    my $firstdisk = PVE::QemuServer::Drive::resolve_first_disk($conf);
-	    $conf->{bootdisk} = $firstdisk if $firstdisk;
+	    my $order = PVE::QemuServer::get_default_bootorder($conf);
+	    $conf->{bootorder} = $order;
 	    PVE::QemuConfig->write_config($vmid, $conf);
 	};
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2747c66..3689ef1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -398,15 +398,33 @@ EODESC
     boot => {
 	optional => 1,
 	type => 'string',
-	description => "Boot on floppy (a), hard disk (c), CD-ROM (d), or network (n).",
+	description => "Boot on floppy (a), hard disk (c), CD-ROM (d), or network (n)."
+		     . " Deprecated: Use 'bootorder' instead.",
 	pattern => '[acdn]{1,4}',
 	default => 'cdn',
     },
     bootdisk => {
 	optional => 1,
 	type => 'string', format => 'pve-qm-bootdisk',
-	description => "Enable booting from specified disk.",
-	pattern => '(ide|sata|scsi|virtio)\d+',
+	description => "Enable booting from specified disk. Deprecated: Use 'bootorder' instead.",
+    },
+    bootorder => {
+	optional => 1,
+	type => 'string',
+	format => 'pve-qm-bootdev-list',
+	description => <<EODESC,
+The guest will attempt to boot from devices in the order they appear here.
+
+Disks, optical drives and passed-through storage USB devices will be directly
+booted from, NICs will load PXE, and PCIe devices will either behave like disks
+(e.g. NVMe) or load an option ROM (e.g. RAID controller, hardware NIC).
+
+Note that only devices in this list will be marked as bootable and thus loaded
+by the guest firmware (BIOS/UEFI). If you require multiple disks for booting
+(e.g. software-raid), you need to specify all of them here.
+
+Overrides the deprecated 'boot' and 'bootdisk' properties when given.
+EODESC
     },
     smp => {
 	optional => 1,
@@ -689,6 +707,27 @@ EODESCR
     },
 };
 
+PVE::JSONSchema::register_format('pve-qm-bootdev', \&verify_bootdev);
+sub verify_bootdev {
+    my ($dev, $noerr) = @_;
+
+    return $dev if PVE::QemuServer::Drive::is_valid_drivename($dev) && $dev !~ m/^efidisk/;
+
+    my $check = sub {
+	my ($base) = @_;
+	return 0 if $dev !~ m/^$base\d+$/;
+	return 0 if !$confdesc->{$dev};
+	return 1;
+    };
+
+    return $dev if $check->("net");
+    return $dev if $check->("usb");
+    return $dev if $check->("hostpci");
+
+    return undef if $noerr;
+    die "invalid boot device '$dev'\n";
+}
+
 my $cicustom_fmt = {
     meta => {
 	type => 'string',
@@ -1552,8 +1591,6 @@ sub print_drive_commandline_full {
 sub print_netdevice_full {
     my ($vmid, $conf, $net, $netid, $bridges, $use_old_bios_files, $arch, $machine_type) = @_;
 
-    my $bootorder = $conf->{boot} || $confdesc->{boot}->{default};
-
     my $device = $net->{model};
     if ($net->{model} eq 'virtio') {
          $device = 'virtio-net-pci';
@@ -3151,17 +3188,29 @@ sub config_to_command {
 	push @$devices, '-device', $kbd if defined($kbd);
     }
 
+    my $bootorder = {};
+    if ($conf->{bootorder}) {
+	# start at 100 to allow user to insert devices before us with -args
+	my $i = 100;
+	for my $dev (PVE::Tools::split_list($conf->{bootorder})) {
+	    $bootorder->{$dev} = $i++;
+	}
+    } else {
+	$bootorder = bootorder_from_legacy($conf);
+    }
+
     # host pci device passthrough
     my ($kvm_off, $gpu_passthrough, $legacy_igd) = PVE::QemuServer::PCI::print_hostpci_devices(
-	$vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type);
+	$vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type, $bootorder);
 
     # usb devices
     my $usb_dev_features = {};
     $usb_dev_features->{spice_usb3} = 1 if min_version($machine_version, 4, 0);
 
     my @usbdevices = PVE::QemuServer::USB::get_usb_devices(
-        $conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features);
+        $conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features, $bootorder);
     push @$devices, @usbdevices if @usbdevices;
+
     # serial devices
     for (my $i = 0; $i < $MAX_SERIAL_PORTS; $i++)  {
 	if (my $path = $conf->{"serial$i"}) {
@@ -3229,15 +3278,6 @@ sub config_to_command {
     }
     push @$cmd, '-nodefaults';
 
-    my $bootorder = $conf->{boot} || $confdesc->{boot}->{default};
-
-    my $bootindex_hash = {};
-    my $i = 1;
-    foreach my $o (split(//, $bootorder)) {
-	$bootindex_hash->{$o} = $i*100;
-	$i++;
-    }
-
     push @$cmd, '-boot', "menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg";
 
     push @$cmd, '-no-acpi' if defined($conf->{acpi}) && $conf->{acpi} == 0;
@@ -3407,17 +3447,7 @@ sub config_to_command {
 
 	$use_virtio = 1 if $ds =~ m/^virtio/;
 
-	if (drive_is_cdrom ($drive)) {
-	    if ($bootindex_hash->{d}) {
-		$drive->{bootindex} = $bootindex_hash->{d};
-		$bootindex_hash->{d} += 1;
-	    }
-	} else {
-	    if ($bootindex_hash->{c}) {
-		$drive->{bootindex} = $bootindex_hash->{c} if $conf->{bootdisk} && ($conf->{bootdisk} eq $ds);
-		$bootindex_hash->{c} += 1;
-	    }
-	}
+	$drive->{bootindex} = $bootorder->{$ds} if $bootorder->{$ds};
 
 	if ($drive->{interface} eq 'virtio'){
            push @$cmd, '-object', "iothread,id=iothread-$ds" if $drive->{iothread};
@@ -3468,24 +3498,23 @@ sub config_to_command {
     });
 
     for (my $i = 0; $i < $MAX_NETS; $i++) {
-	 next if !$conf->{"net$i"};
-	 my $d = parse_net($conf->{"net$i"});
-	 next if !$d;
+	my $netname = "net$i";
 
-	 $use_virtio = 1 if $d->{model} eq 'virtio';
+	next if !$conf->{$netname};
+	my $d = parse_net($conf->{$netname});
+	next if !$d;
 
-	 if ($bootindex_hash->{n}) {
-	    $d->{bootindex} = $bootindex_hash->{n};
-	    $bootindex_hash->{n} += 1;
-	 }
+	$use_virtio = 1 if $d->{model} eq 'virtio';
 
-	 my $netdevfull = print_netdev_full($vmid, $conf, $arch, $d, "net$i");
-	 push @$devices, '-netdev', $netdevfull;
+	$d->{bootindex} = $bootorder->{$netname} if $bootorder->{$netname};
 
-	 my $netdevicefull = print_netdevice_full(
-	    $vmid, $conf, $d, "net$i", $bridges, $use_old_bios_files, $arch, $machine_type);
+	my $netdevfull = print_netdev_full($vmid, $conf, $arch, $d, $netname);
+	push @$devices, '-netdev', $netdevfull;
 
-	 push @$devices, '-device', $netdevicefull;
+	my $netdevicefull = print_netdevice_full(
+	$vmid, $conf, $d, $netname, $bridges, $use_old_bios_files, $arch, $machine_type);
+
+	push @$devices, '-device', $netdevicefull;
     }
 
     if ($conf->{ivshmem}) {
@@ -3765,7 +3794,8 @@ sub vm_deviceunplug {
     my $devices_list = vm_devices_list($vmid);
     return 1 if !defined($devices_list->{$deviceid});
 
-    die "can't unplug bootdisk" if $conf->{bootdisk} && $conf->{bootdisk} eq $deviceid;
+    my $bootdisks = PVE::QemuServer::Drive::get_bootdisks($conf);
+    die "can't unplug bootdisk '$deviceid'\n" if grep {$_ eq $deviceid} @$bootdisks;
 
     if ($deviceid eq 'tablet' || $deviceid eq 'keyboard') {
 
@@ -7152,6 +7182,72 @@ sub clear_reboot_request {
     return $res;
 }
 
+sub bootorder_from_legacy {
+    my ($conf) = @_;
+
+    my $boot = $conf->{boot} || $confdesc->{boot}->{default};
+    my $bootindex_hash = {};
+    my $i = 1;
+    foreach my $o (split(//, $boot)) {
+	$bootindex_hash->{$o} = $i*100;
+	$i++;
+    }
+
+    my $bootorder = {};
+
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($ds, $drive) = @_;
+
+	if (drive_is_cdrom ($drive, 1)) {
+	    if ($bootindex_hash->{d}) {
+		$bootorder->{$ds} = $bootindex_hash->{d};
+		$bootindex_hash->{d} += 1;
+	    }
+	} elsif ($bootindex_hash->{c} && $conf->{bootdisk} && $conf->{bootdisk} eq $ds) {
+	    $bootorder->{$ds} = $bootindex_hash->{c};
+	}
+    });
+
+    if ($bootindex_hash->{n}) {
+	for (my $i = 0; $i < $MAX_NETS; $i++) {
+	    my $netname = "net$i";
+	    next if !$conf->{$netname};
+	    $bootorder->{$netname} = $bootindex_hash->{n};
+	    $bootindex_hash->{n} += 1;
+	}
+    }
+
+    return $bootorder;
+}
+
+# Matches legacy default boot order, but with explicit device names. This is
+# somewhat important, since the fallback for when neither 'bootorder' nor the
+# old 'boot'/'bootdisk' is specified relies on 'bootorder_from_legacy' above,
+# and it would be confusing if this diverges.
+sub get_default_bootorder {
+    my ($conf) = @_;
+
+    my @ret = ();
+
+    # harddisk
+    my $first = PVE::QemuServer::Drive::resolve_first_disk($conf, 0);
+    push @ret, $first if $first;
+
+    # cdrom
+    $first = PVE::QemuServer::Drive::resolve_first_disk($conf, 1);
+    push @ret, $first if $first;
+
+    # network
+    for (my $i = 0; $i < $MAX_NETS; $i++) {
+	my $netname = "net$i";
+	next if !$conf->{$netname};
+	push @ret, $netname;
+	last;
+    }
+
+    return join(',', @ret);
+}
+
 # bash completion helper
 
 sub complete_backup_archives {
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 91c33f8..af25882 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -501,11 +501,25 @@ sub print_drive {
     return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
 }
 
+sub get_bootdisks {
+    my ($conf) = @_;
+
+    if (!$conf->{bootorder}) {
+	return [$conf->{bootdisk}] if $conf->{bootdisk};
+	return [];
+    }
+
+    my @list = PVE::Tools::split_list($conf->{bootorder});
+    @list = grep {is_valid_drivename($_)} @list;
+    return \@list;
+}
+
 sub bootdisk_size {
     my ($storecfg, $conf) = @_;
 
-    my $bootdisk = $conf->{bootdisk};
-    return undef if !$bootdisk;
+    my $bootdisks = get_bootdisks($conf);
+    return undef if !@$bootdisks;
+    my $bootdisk = $bootdisks->[0];
     return undef if !is_valid_drivename($bootdisk);
 
     return undef if !$conf->{$bootdisk};
@@ -584,16 +598,15 @@ sub is_volume_in_use {
 }
 
 sub resolve_first_disk {
-    my $conf = shift;
+    my ($conf, $cdrom) = @_;
     my @disks = valid_drive_names();
-    my $firstdisk;
-    foreach my $ds (reverse @disks) {
+    foreach my $ds (@disks) {
 	next if !$conf->{$ds};
 	my $disk = parse_drive($ds, $conf->{$ds});
-	next if drive_is_cdrom($disk);
-	$firstdisk = $ds;
+	next if !(drive_is_cdrom($disk) xor $cdrom);
+	return $ds;
     }
-    return $firstdisk;
+    return undef;
 }
 
 1;
diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index cb36845..2df2708 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -357,7 +357,7 @@ sub parse_hostpci {
 }
 
 sub print_hostpci_devices {
-    my ($vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type) = @_;
+    my ($vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type, $bootorder) = @_;
 
     my $kvm_off = 0;
     my $gpu_passthrough = 0;
@@ -446,6 +446,7 @@ sub print_hostpci_devices {
 		$devicestr .= "$xvga";
 		$devicestr .= ",multifunction=on" if $multifunction;
 		$devicestr .= ",romfile=/usr/share/kvm/$d->{romfile}" if $d->{romfile};
+		$devicestr .= ",bootindex=$bootorder->{$id}" if $bootorder->{$id};
 	    }
 
 	    push @$devices, '-device', $devicestr;
diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
index d328148..4a843cd 100644
--- a/PVE/QemuServer/USB.pm
+++ b/PVE/QemuServer/USB.pm
@@ -74,13 +74,14 @@ sub get_usb_controllers {
 }
 
 sub get_usb_devices {
-    my ($conf, $format, $max_usb_devices, $features) = @_;
+    my ($conf, $format, $max_usb_devices, $features, $bootorder) = @_;
 
     my $devices = [];
 
     for (my $i = 0; $i < $max_usb_devices; $i++)  {
-	next if !$conf->{"usb$i"};
-	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{"usb$i"}) };
+	my $devname = "usb$i";
+	next if !$conf->{$devname};
+	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{$devname}) };
 	next if !$d;
 
 	if (defined($d->{host})) {
@@ -93,8 +94,10 @@ sub get_usb_devices {
 
 		push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
 		push @$devices, '-device', "usb-redir,chardev=usbredirchardev$i,id=usbredirdev$i,bus=$bus.0";
+
+		warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname};
 	    } else {
-		push @$devices, '-device', print_usbdevice_full($conf, "usb$i", $hostdevice);
+		push @$devices, '-device', print_usbdevice_full($conf, $devname, $hostdevice, $bootorder);
 	    }
 	}
     }
@@ -103,7 +106,7 @@ sub get_usb_devices {
 }
 
 sub print_usbdevice_full {
-    my ($conf, $deviceid, $device) = @_;
+    my ($conf, $deviceid, $device, $bootorder) = @_;
 
     return if !$device;
     my $usbdevice = "usb-host";
@@ -120,6 +123,7 @@ sub print_usbdevice_full {
     }
 
     $usbdevice .= ",id=$deviceid";
+    $usbdevice .= ",bootindex=$bootorder->{$deviceid}" if $bootorder->{$deviceid};
     return $usbdevice;
 }
 
diff --git a/test/cfg2cmd/bootorder.conf b/test/cfg2cmd/bootorder.conf
new file mode 100644
index 0000000..7d296da
--- /dev/null
+++ b/test/cfg2cmd/bootorder.conf
@@ -0,0 +1,16 @@
+# TEST: Test for a specific bootorder given by 'bootorder' parameter
+# QEMU_VERSION: 5.1
+cores: 3
+bootorder: virtio1,net0,scsi4,ide2
+ide2: none,media=cdrom
+memory: 768
+name: simple
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+numa: 0
+ostype: l26
+scsi4: local:8006/vm-8006-disk-0.qcow2,discard=on,size=104858K
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+virtio0: local:8006/vm-8006-disk-0.qcow2,discard=on,iothread=1,size=104858K
+virtio1: local:8006/vm-8006-disk-0.qcow2,discard=on,iothread=1,size=104858K
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
diff --git a/test/cfg2cmd/bootorder.conf.cmd b/test/cfg2cmd/bootorder.conf.cmd
new file mode 100644
index 0000000..86cae07
--- /dev/null
+++ b/test/cfg2cmd/bootorder.conf.cmd
@@ -0,0 +1,38 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name simple \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server,nowait' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
+  -smp '3,sockets=1,cores=3,maxcpus=3' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc unix:/var/run/qemu-server/8006.vnc,password \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 768 \
+  -object 'iothread,id=iothread-virtio0' \
+  -object 'iothread,id=iothread-virtio1' \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=c773c261-d800-4348-9f5d-167fadd53cf8' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide2,media=cdrom,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=103' \
+  -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-scsi4,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
+  -device 'scsi-hd,bus=scsihw0.0,scsi-id=4,drive=drive-scsi4,id=scsi4,bootindex=102' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-virtio0,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
+  -device 'virtio-blk-pci,drive=drive-virtio0,id=virtio0,bus=pci.0,addr=0xa,iothread=iothread-virtio0' \
+  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-virtio1,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
+  -device 'virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=A2:C0:43:77:08:A0,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=101' \
+  -machine 'type=pc+pve0'
-- 
2.20.1

[pve-devel] [PATCH manager 2/2] ui: improve boot order editor with 'bootorder' support

[Stefan Reiter]

The new 'bootorder' property can express many more scenarios than the
old 'boot'/'bootdisk' ones. Update the editor so it can handle it.

Features a grid with all supported boot devices which can be reordered
using drag-and-drop, as well as toggled on and off with an inline
checkbox.

Support for configs still using the old properties is given, with the
first write automatically updating the VM config to use the new one.

The renderer for the Options panel is updated with support for the new
format, and the display format for the fallback is changed to make it
look uniform.

Note that it is very well possible to disable all boot devices, in which
case an empty 'bootorder: ' will be stored to the config file. I'm not
sure what that would be useful for, but there's no reason to forbid it
either, just warn the user that it's probably not what he wants.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 www/manager6/qemu/BootOrderEdit.js | 310 +++++++++++++++++------------
 www/manager6/qemu/Options.js       |  32 ++-
 2 files changed, 210 insertions(+), 132 deletions(-)

diff --git a/www/manager6/qemu/BootOrderEdit.js b/www/manager6/qemu/BootOrderEdit.js
index 19d5d50a..b1236bbd 100644
--- a/www/manager6/qemu/BootOrderEdit.js
+++ b/www/manager6/qemu/BootOrderEdit.js
@@ -1,149 +1,208 @@
+Ext.define('pve-boot-order-entry', {
+    extend: 'Ext.data.Model',
+    fields: [
+	{name: 'name', type: 'string'},
+	{name: 'enabled', type: 'bool'},
+	{name: 'desc', type: 'string'},
+    ]
+});
+
 Ext.define('PVE.qemu.BootOrderPanel', {
     extend: 'Proxmox.panel.InputPanel',
     alias: 'widget.pveQemuBootOrderPanel',
+
     vmconfig: {}, // store loaded vm config
+    store: undefined,
+    originalValue: undefined,
 
-    bootdisk: undefined,
-    selection: [],
-    list: [],
-    comboboxes: [],
-
-    isBootDisk: function(value) {
+    isDisk: function(value, cdrom) {
 	return PVE.Utils.bus_match.test(value);
     },
 
-    setVMConfig: function(vmconfig) {
-	var me = this;
-	me.vmconfig = vmconfig;
-	var order = me.vmconfig.boot || 'cdn';
-	me.bootdisk = me.vmconfig.bootdisk || undefined;
-
-	// get the first 3 characters
-	// ignore the rest (there should never be more than 3)
-	me.selection = order.split('').slice(0,3);
-
-	// build bootdev list
-	me.list = [];
-	Ext.Object.each(me.vmconfig, function(key, value) {
-	    if (me.isBootDisk(key) &&
-		!(/media=cdrom/).test(value)) {
-		me.list.push([key, "Disk '" + key + "'"]);
-	    }
-	});
-
-	me.list.push(['d', 'CD-ROM']);
-	me.list.push(['n', gettext('Network')]);
-	me.list.push(['__none__', Proxmox.Utils.noneText]);
-
-	me.recomputeList();
-
-	me.comboboxes.forEach(function(box) {
-	    box.resetOriginalValue();
-	});
+    isBootdev: function(dev, value) {
+	return this.isDisk(dev) ||
+	    (/^net\d+/).test(dev) ||
+	    (/^hostpci\d+/).test(dev) ||
+	    ((/^usb\d+/).test(dev) && !(/spice/).test(value));
     },
 
-    onGetValues: function(values) {
-	var me = this;
-	var order = me.selection.join('');
-	var res = { boot: order };
+    setVMConfig: function(vmconfig) {
+	let me = this;
+	me.vmconfig = vmconfig;
 
-	if  (me.bootdisk && order.indexOf('c') !== -1) {
-	    res.bootdisk = me.bootdisk;
+	me.store.removeAll();
+
+	let bootorder;
+	if (me.vmconfig.bootorder) {
+	    bootorder = (/^\s*$/).test(me.vmconfig.bootorder) ? [] :
+		me.vmconfig.bootorder
+		    .split(',')
+		    .map(dev => ({name: dev, enabled: true}));
 	} else {
-	    res['delete'] = 'bootdisk';
+	    // legacy style, transform to new bootorder
+	    let order = me.vmconfig.boot || 'cdn';
+	    let bootdisk = me.vmconfig.bootdisk || undefined;
+
+	    // get the first 3 characters
+	    // ignore the rest (there should never be more than 3)
+	    let orderList = order.split('').slice(0,3);
+
+	    // build bootdev list
+	    bootorder = [];
+	    for (let i = 0; i < orderList.length; i++) {
+		let list = [];
+		if (orderList[i] === 'c') {
+		    if (bootdisk !== undefined && me.vmconfig[bootdisk]) {
+			list.push(bootdisk);
+		    }
+		} else if (orderList[i] === 'd') {
+		    Ext.Object.each(me.vmconfig, function(key, value) {
+			if (me.isDisk(key) && (/media=cdrom/).test(value)) {
+			    list.push(key);
+			}
+		    });
+		} else if (orderList[i] === 'n') {
+		    Ext.Object.each(me.vmconfig, function(key, value) {
+			if ((/^net\d+/).test(key)) {
+			    list.push(key);
+			}
+		    });
+		}
+
+		// Object.each iterates in random order, sort alphabetically
+		list.sort();
+		list.forEach(dev => bootorder.push({name: dev, enabled: true}));
+	    }
 	}
 
+	// add disabled devices as well
+	let disabled = [];
+	Ext.Object.each(me.vmconfig, function(key, value) {
+	    if (me.isBootdev(key, value) &&
+		!Ext.Array.some(bootorder, x => x.name === key))
+	    {
+		disabled.push(key);
+	    }
+	});
+	disabled.sort();
+	disabled.forEach(dev => bootorder.push({name: dev, enabled: false}));
+
+	bootorder.forEach(entry => {
+	    entry.desc = me.vmconfig[entry.name];
+	});
+
+	me.store.insert(0, bootorder);
+	me.store.fireEvent("update");
+    },
+
+    calculateValue: function() {
+	let me = this;
+	return me.store.getData().items
+	    .filter(x => x.data.enabled)
+	    .map(x => x.data.name)
+	    .join(',');
+    },
+
+    onGetValues: function() {
+	let me = this;
+	// Note: we allow an empty value, so no 'delete' option
+	let res = { bootorder: me.calculateValue() };
 	return res;
     },
 
-    recomputeSelection: function(combobox, newVal, oldVal) {
-	var me = this.up('#inputpanel');
-	me.selection = [];
-	me.comboboxes.forEach(function(item) {
-	    var val = item.getValue();
-
-	    // when selecting an already selected item,
-	    // switch it around
-	    if ((val === newVal || (me.isBootDisk(val) && me.isBootDisk(newVal))) &&
-		item.name !== combobox.name &&
-		newVal !== '__none__') {
-		// swap items
-		val = oldVal;
-	    }
-
-	    // push 'c','d' or 'n' in the array
-	    if (me.isBootDisk(val)) {
-		me.selection.push('c');
-		me.bootdisk = val;
-	    } else if (val === 'd' ||
-		       val === 'n') {
-		me.selection.push(val);
-	    }
-	});
-
-	me.recomputeList();
-    },
-
-    recomputeList: function(){
-	var me = this;
-	// set the correct values in the kvcomboboxes
-	var cnt = 0;
-	me.comboboxes.forEach(function(item) {
-	    if (cnt === 0) {
-		// never show 'none' on first combobox
-		item.store.loadData(me.list.slice(0, me.list.length-1));
-	    } else {
-		item.store.loadData(me.list);
-	    }
-	    item.suspendEvent('change');
-	    if (cnt < me.selection.length) {
-		item.setValue((me.selection[cnt] !== 'c')?me.selection[cnt]:me.bootdisk);
-	    } else if (cnt === 0){
-		item.setValue('');
-	    } else {
-		item.setValue('__none__');
-	    }
-	    cnt++;
-	    item.resumeEvent('change');
-	    item.validate();
-	});
-    },
-
     initComponent : function() {
-	var me = this;
+	let me = this;
 
-	// this has to be done here, because of
-	// the way our inputPanel class handles items
-	me.comboboxes = [
-		Ext.createWidget('proxmoxKVComboBox', {
-		fieldLabel: gettext('Boot device') + " 1",
-		labelWidth: 120,
-		name: 'bd1',
-		allowBlank: false,
-		listeners: {
-		    change: me.recomputeSelection
+	// for dirty marking and reset detection
+	let inUpdate = false;
+	let marker = Ext.create('Ext.form.field.Base', {
+	    hidden: true,
+	    itemId: 'marker',
+	    setValue: function(val) {
+		// on form reset, go back to original state
+		if (!inUpdate) {
+		    me.setVMConfig(me.vmconfig);
 		}
-	    }),
-		Ext.createWidget('proxmoxKVComboBox', {
-		fieldLabel: gettext('Boot device') + " 2",
-		labelWidth: 120,
-		name: 'bd2',
-		allowBlank: false,
-		listeners: {
-		    change: me.recomputeSelection
+
+		// not a subclass, so no callParent; just do it manually
+		this.setRawValue(this.valueToRaw(val));
+		return this.mixins.field.setValue.call(this, val);
+	    }
+	});
+
+	let emptyWarning = Ext.create('Ext.form.field.Display', {
+	    userCls: 'pmx-hint',
+	    value: gettext('Warning: No devices selected, the VM will probably not boot!'),
+	});
+
+	me.store = Ext.create('Ext.data.Store', {
+	    model: 'pve-boot-order-entry',
+	    listeners: {
+		update: function() {
+		    this.commitChanges();
+		    let val = me.calculateValue();
+		    if (!marker.originalValue) {
+			marker.originalValue = val;
+		    }
+		    inUpdate = true;
+		    marker.setValue(val);
+		    inUpdate = false;
+		    marker.checkDirty();
+		    emptyWarning.setHidden(val !== '');
 		}
-	    }),
-		Ext.createWidget('proxmoxKVComboBox', {
-		fieldLabel: gettext('Boot device') + " 3",
-		labelWidth: 120,
-		name: 'bd3',
-		allowBlank: false,
-		listeners: {
-		    change: me.recomputeSelection
+	    }
+	});
+
+	let grid = Ext.create('Ext.grid.Panel', {
+	    store: me.store,
+	    margin: '0 0 5 0',
+	    columns: [
+		{
+		    xtype: 'checkcolumn',
+		    header: gettext('Enabled'),
+		    dataIndex: 'enabled',
+		    width: 70,
+		    sortable: false,
+		    hideable: false,
+		    draggable: false,
+		},
+		{
+		    header: gettext('Device'),
+		    dataIndex: 'name',
+		    width: 70,
+		    sortable: false,
+		    hideable: false,
+		    draggable: false,
+		},
+		{
+		    header: gettext('Description'),
+		    dataIndex: 'desc',
+		    flex: true,
+		    sortable: false,
+		    hideable: false,
+		    draggable: false,
+		},
+	    ],
+	    viewConfig: {
+		plugins: {
+		    ptype: 'gridviewdragdrop',
+		    dragText:gettext('Drag and drop to reorder'),
 		}
-	    })
-	];
-	Ext.apply(me, { items: me.comboboxes });
+	    },
+	    listeners: {
+		drop: function() {
+		    // doesn't fire automatically on reorder
+		    me.store.fireEvent("update");
+		}
+	    },
+	});
+
+	let info = Ext.create('Ext.Component', {
+	    html: gettext('Drag and drop to reorder'),
+	});
+
+	Ext.apply(me, { items: [grid, info, emptyWarning, marker] });
+
 	me.callParent();
     }
 });
@@ -157,9 +216,10 @@ Ext.define('PVE.qemu.BootOrderEdit', {
     }],
 
     subject: gettext('Boot Order'),
+    width: 600,
 
     initComponent : function() {
-	var me = this;
+	let me = this;
 	me.callParent();
 	me.load({
 	    success: function(response, options) {
diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
index 20f6ffbb..490d2f54 100644
--- a/www/manager6/qemu/Options.js
+++ b/www/manager6/qemu/Options.js
@@ -86,12 +86,30 @@ Ext.define('PVE.qemu.Options', {
 	    bootdisk: {
 		visible: false
 	    },
+	    bootorder: {
+		visible: false
+	    },
 	    boot: {
 		header: gettext('Boot Order'),
 		defaultValue: 'cdn',
 		editor: caps.vms['VM.Config.Disk'] ? 'PVE.qemu.BootOrderEdit' : undefined,
-		multiKey: ['boot', 'bootdisk'],
+		multiKey: ['boot', 'bootdisk', 'bootorder'],
 		renderer: function(order, metaData, record, rowIndex, colIndex, store, pending) {
+		    let bootorder = me.getObjectValue('bootorder', undefined, pending);
+		    if (bootorder) {
+			let list = bootorder.split(',');
+			let ret = '';
+			list.forEach(dev => {
+			    if (ret !== '') {
+				ret += ' > ';
+			    }
+			    let desc = dev;
+			    ret += desc;
+			});
+			return ret;
+		    }
+
+		    // legacy style and fallback
 		    var i;
 		    var text = '';
 		    var bootdisk = me.getObjectValue('bootdisk', undefined, pending);
@@ -99,20 +117,20 @@ Ext.define('PVE.qemu.Options', {
 		    for (i = 0; i < order.length; i++) {
 			var sel = order.substring(i, i + 1);
 			if (text) {
-			    text += ', ';
+			    text += ' > ';
 			}
 			if (sel === 'c') {
 			    if (bootdisk) {
-				text += "Disk '" + bootdisk + "'";
+				text += bootdisk;
 			    } else {
-				text += "Disk";
+				text += "disk";
 			    }
 			} else if (sel === 'n') {
-			    text += 'Network';
+			    text += 'any net';
 			} else if (sel === 'a') {
-			    text += 'Floppy';
+			    text += 'floppy';
 			} else if (sel === 'd') {
-			    text += 'CD-ROM';
+			    text += 'any cdrom';
 			} else {
 			    text += sel;
 			}
-- 
2.20.1

Re: [pve-devel] [PATCH manager 2/2] ui: improve boot order editor with 'bootorder' support

[Thomas Lamprecht]

On 24.09.20 16:11, Stefan Reiter wrote:
> The new 'bootorder' property can express many more scenarios than the
> old 'boot'/'bootdisk' ones. Update the editor so it can handle it.
> 
> Features a grid with all supported boot devices which can be reordered
> using drag-and-drop, as well as toggled on and off with an inline
> checkbox.
> 
> Support for configs still using the old properties is given, with the
> first write automatically updating the VM config to use the new one.
> 
> The renderer for the Options panel is updated with support for the new
> format, and the display format for the fallback is changed to make it
> look uniform.
> 
> Note that it is very well possible to disable all boot devices, in which
> case an empty 'bootorder: ' will be stored to the config file. I'm not
> sure what that would be useful for, but there's no reason to forbid it
> either, just warn the user that it's probably not what he wants.
> 

It's not bad, I like it in general! I'd a few things though.


* Add a order icon ( https://fontawesome.com/v4.7.0/icon/bars has "reorder" as
  alias) to rows
* Add a # column which shows the "order", i.e., similar to our firewall rules
* Use another renderer for the Options view, IMO the "A > B > C" makes people
  think to hard, rather just use number, i.e., to something like
  "1. scsi0, 2. Any CD-ROM, ..."#
  Also, please keep the case and style of CD-ROM as is, no point in changing that.

> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  www/manager6/qemu/BootOrderEdit.js | 310 +++++++++++++++++------------
>  www/manager6/qemu/Options.js       |  32 ++-
>  2 files changed, 210 insertions(+), 132 deletions(-)
> 
> diff --git a/www/manager6/qemu/BootOrderEdit.js b/www/manager6/qemu/BootOrderEdit.js
> index 19d5d50a..b1236bbd 100644
> --- a/www/manager6/qemu/BootOrderEdit.js
> +++ b/www/manager6/qemu/BootOrderEdit.js
> @@ -1,149 +1,208 @@
> +Ext.define('pve-boot-order-entry', {
> +    extend: 'Ext.data.Model',
> +    fields: [
> +	{name: 'name', type: 'string'},
> +	{name: 'enabled', type: 'bool'},
> +	{name: 'desc', type: 'string'},
> +    ]
> +});
> +
>  Ext.define('PVE.qemu.BootOrderPanel', {
>      extend: 'Proxmox.panel.InputPanel',
>      alias: 'widget.pveQemuBootOrderPanel',
> +
>      vmconfig: {}, // store loaded vm config
> +    store: undefined,
> +    originalValue: undefined,
>  
> -    bootdisk: undefined,
> -    selection: [],
> -    list: [],
> -    comboboxes: [],
> -
> -    isBootDisk: function(value) {
> +    isDisk: function(value, cdrom) {
>  	return PVE.Utils.bus_match.test(value);
>      },
>  
> -    setVMConfig: function(vmconfig) {
> -	var me = this;
> -	me.vmconfig = vmconfig;
> -	var order = me.vmconfig.boot || 'cdn';
> -	me.bootdisk = me.vmconfig.bootdisk || undefined;
> -
> -	// get the first 3 characters
> -	// ignore the rest (there should never be more than 3)
> -	me.selection = order.split('').slice(0,3);
> -
> -	// build bootdev list
> -	me.list = [];
> -	Ext.Object.each(me.vmconfig, function(key, value) {
> -	    if (me.isBootDisk(key) &&
> -		!(/media=cdrom/).test(value)) {
> -		me.list.push([key, "Disk '" + key + "'"]);
> -	    }
> -	});
> -
> -	me.list.push(['d', 'CD-ROM']);
> -	me.list.push(['n', gettext('Network')]);
> -	me.list.push(['__none__', Proxmox.Utils.noneText]);
> -
> -	me.recomputeList();
> -
> -	me.comboboxes.forEach(function(box) {
> -	    box.resetOriginalValue();
> -	});
> +    isBootdev: function(dev, value) {
> +	return this.isDisk(dev) ||
> +	    (/^net\d+/).test(dev) ||
> +	    (/^hostpci\d+/).test(dev) ||
> +	    ((/^usb\d+/).test(dev) && !(/spice/).test(value));
>      },
>  
> -    onGetValues: function(values) {
> -	var me = this;
> -	var order = me.selection.join('');
> -	var res = { boot: order };
> +    setVMConfig: function(vmconfig) {
> +	let me = this;
> +	me.vmconfig = vmconfig;
>  
> -	if  (me.bootdisk && order.indexOf('c') !== -1) {
> -	    res.bootdisk = me.bootdisk;
> +	me.store.removeAll();
> +
> +	let bootorder;
> +	if (me.vmconfig.bootorder) {
> +	    bootorder = (/^\s*$/).test(me.vmconfig.bootorder) ? [] :
> +		me.vmconfig.bootorder
> +		    .split(',')
> +		    .map(dev => ({name: dev, enabled: true}));
>  	} else {
> -	    res['delete'] = 'bootdisk';
> +	    // legacy style, transform to new bootorder
> +	    let order = me.vmconfig.boot || 'cdn';
> +	    let bootdisk = me.vmconfig.bootdisk || undefined;
> +
> +	    // get the first 3 characters
> +	    // ignore the rest (there should never be more than 3)
> +	    let orderList = order.split('').slice(0,3);
> +
> +	    // build bootdev list
> +	    bootorder = [];
> +	    for (let i = 0; i < orderList.length; i++) {
> +		let list = [];
> +		if (orderList[i] === 'c') {
> +		    if (bootdisk !== undefined && me.vmconfig[bootdisk]) {
> +			list.push(bootdisk);
> +		    }
> +		} else if (orderList[i] === 'd') {
> +		    Ext.Object.each(me.vmconfig, function(key, value) {
> +			if (me.isDisk(key) && (/media=cdrom/).test(value)) {
> +			    list.push(key);
> +			}
> +		    });
> +		} else if (orderList[i] === 'n') {
> +		    Ext.Object.each(me.vmconfig, function(key, value) {
> +			if ((/^net\d+/).test(key)) {
> +			    list.push(key);
> +			}
> +		    });
> +		}
> +
> +		// Object.each iterates in random order, sort alphabetically
> +		list.sort();
> +		list.forEach(dev => bootorder.push({name: dev, enabled: true}));
> +	    }
>  	}
>  
> +	// add disabled devices as well
> +	let disabled = [];
> +	Ext.Object.each(me.vmconfig, function(key, value) {
> +	    if (me.isBootdev(key, value) &&
> +		!Ext.Array.some(bootorder, x => x.name === key))
> +	    {
> +		disabled.push(key);
> +	    }
> +	});
> +	disabled.sort();
> +	disabled.forEach(dev => bootorder.push({name: dev, enabled: false}));
> +
> +	bootorder.forEach(entry => {
> +	    entry.desc = me.vmconfig[entry.name];
> +	});
> +
> +	me.store.insert(0, bootorder);
> +	me.store.fireEvent("update");
> +    },
> +
> +    calculateValue: function() {
> +	let me = this;
> +	return me.store.getData().items
> +	    .filter(x => x.data.enabled)
> +	    .map(x => x.data.name)
> +	    .join(',');
> +    },
> +
> +    onGetValues: function() {
> +	let me = this;
> +	// Note: we allow an empty value, so no 'delete' option
> +	let res = { bootorder: me.calculateValue() };
>  	return res;
>      },
>  
> -    recomputeSelection: function(combobox, newVal, oldVal) {
> -	var me = this.up('#inputpanel');
> -	me.selection = [];
> -	me.comboboxes.forEach(function(item) {
> -	    var val = item.getValue();
> -
> -	    // when selecting an already selected item,
> -	    // switch it around
> -	    if ((val === newVal || (me.isBootDisk(val) && me.isBootDisk(newVal))) &&
> -		item.name !== combobox.name &&
> -		newVal !== '__none__') {
> -		// swap items
> -		val = oldVal;
> -	    }
> -
> -	    // push 'c','d' or 'n' in the array
> -	    if (me.isBootDisk(val)) {
> -		me.selection.push('c');
> -		me.bootdisk = val;
> -	    } else if (val === 'd' ||
> -		       val === 'n') {
> -		me.selection.push(val);
> -	    }
> -	});
> -
> -	me.recomputeList();
> -    },
> -
> -    recomputeList: function(){
> -	var me = this;
> -	// set the correct values in the kvcomboboxes
> -	var cnt = 0;
> -	me.comboboxes.forEach(function(item) {
> -	    if (cnt === 0) {
> -		// never show 'none' on first combobox
> -		item.store.loadData(me.list.slice(0, me.list.length-1));
> -	    } else {
> -		item.store.loadData(me.list);
> -	    }
> -	    item.suspendEvent('change');
> -	    if (cnt < me.selection.length) {
> -		item.setValue((me.selection[cnt] !== 'c')?me.selection[cnt]:me.bootdisk);
> -	    } else if (cnt === 0){
> -		item.setValue('');
> -	    } else {
> -		item.setValue('__none__');
> -	    }
> -	    cnt++;
> -	    item.resumeEvent('change');
> -	    item.validate();
> -	});
> -    },
> -
>      initComponent : function() {
> -	var me = this;
> +	let me = this;
>  
> -	// this has to be done here, because of
> -	// the way our inputPanel class handles items
> -	me.comboboxes = [
> -		Ext.createWidget('proxmoxKVComboBox', {
> -		fieldLabel: gettext('Boot device') + " 1",
> -		labelWidth: 120,
> -		name: 'bd1',
> -		allowBlank: false,
> -		listeners: {
> -		    change: me.recomputeSelection
> +	// for dirty marking and reset detection
> +	let inUpdate = false;
> +	let marker = Ext.create('Ext.form.field.Base', {
> +	    hidden: true,
> +	    itemId: 'marker',
> +	    setValue: function(val) {
> +		// on form reset, go back to original state
> +		if (!inUpdate) {
> +		    me.setVMConfig(me.vmconfig);
>  		}
> -	    }),
> -		Ext.createWidget('proxmoxKVComboBox', {
> -		fieldLabel: gettext('Boot device') + " 2",
> -		labelWidth: 120,
> -		name: 'bd2',
> -		allowBlank: false,
> -		listeners: {
> -		    change: me.recomputeSelection
> +
> +		// not a subclass, so no callParent; just do it manually
> +		this.setRawValue(this.valueToRaw(val));
> +		return this.mixins.field.setValue.call(this, val);
> +	    }
> +	});
> +
> +	let emptyWarning = Ext.create('Ext.form.field.Display', {
> +	    userCls: 'pmx-hint',
> +	    value: gettext('Warning: No devices selected, the VM will probably not boot!'),
> +	});
> +
> +	me.store = Ext.create('Ext.data.Store', {
> +	    model: 'pve-boot-order-entry',
> +	    listeners: {
> +		update: function() {
> +		    this.commitChanges();
> +		    let val = me.calculateValue();
> +		    if (!marker.originalValue) {
> +			marker.originalValue = val;
> +		    }
> +		    inUpdate = true;
> +		    marker.setValue(val);
> +		    inUpdate = false;
> +		    marker.checkDirty();
> +		    emptyWarning.setHidden(val !== '');
>  		}
> -	    }),
> -		Ext.createWidget('proxmoxKVComboBox', {
> -		fieldLabel: gettext('Boot device') + " 3",
> -		labelWidth: 120,
> -		name: 'bd3',
> -		allowBlank: false,
> -		listeners: {
> -		    change: me.recomputeSelection
> +	    }
> +	});
> +
> +	let grid = Ext.create('Ext.grid.Panel', {
> +	    store: me.store,


It seems to me that this all could be made more schematic without to much
work, could be nice.

> +	    margin: '0 0 5 0',
> +	    columns: [
> +		{
> +		    xtype: 'checkcolumn',
> +		    header: gettext('Enabled'),
> +		    dataIndex: 'enabled',
> +		    width: 70,
> +		    sortable: false,
> +		    hideable: false,
> +		    draggable: false,
> +		},
> +		{
> +		    header: gettext('Device'),
> +		    dataIndex: 'name',
> +		    width: 70,
> +		    sortable: false,
> +		    hideable: false,
> +		    draggable: false,
> +		},
> +		{
> +		    header: gettext('Description'),
> +		    dataIndex: 'desc',
> +		    flex: true,
> +		    sortable: false,
> +		    hideable: false,
> +		    draggable: false,
> +		},
> +	    ],
> +	    viewConfig: {
> +		plugins: {
> +		    ptype: 'gridviewdragdrop',
> +		    dragText:gettext('Drag and drop to reorder'),
>  		}
> -	    })
> -	];
> -	Ext.apply(me, { items: me.comboboxes });
> +	    },
> +	    listeners: {
> +		drop: function() {
> +		    // doesn't fire automatically on reorder
> +		    me.store.fireEvent("update");
> +		}
> +	    },
> +	});
> +
> +	let info = Ext.create('Ext.Component', {
> +	    html: gettext('Drag and drop to reorder'),
> +	});
> +
> +	Ext.apply(me, { items: [grid, info, emptyWarning, marker] });
> +
>  	me.callParent();
>      }
>  });
> @@ -157,9 +216,10 @@ Ext.define('PVE.qemu.BootOrderEdit', {
>      }],
>  
>      subject: gettext('Boot Order'),
> +    width: 600,
>  
>      initComponent : function() {
> -	var me = this;
> +	let me = this;
>  	me.callParent();
>  	me.load({
>  	    success: function(response, options) {
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index 20f6ffbb..490d2f54 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -86,12 +86,30 @@ Ext.define('PVE.qemu.Options', {
>  	    bootdisk: {
>  		visible: false
>  	    },
> +	    bootorder: {
> +		visible: false
> +	    },
>  	    boot: {
>  		header: gettext('Boot Order'),
>  		defaultValue: 'cdn',
>  		editor: caps.vms['VM.Config.Disk'] ? 'PVE.qemu.BootOrderEdit' : undefined,
> -		multiKey: ['boot', 'bootdisk'],
> +		multiKey: ['boot', 'bootdisk', 'bootorder'],
>  		renderer: function(order, metaData, record, rowIndex, colIndex, store, pending) {
> +		    let bootorder = me.getObjectValue('bootorder', undefined, pending);
> +		    if (bootorder) {
> +			let list = bootorder.split(',');
> +			let ret = '';
> +			list.forEach(dev => {
> +			    if (ret !== '') {
> +				ret += ' > ';
> +			    }
> +			    let desc = dev;
> +			    ret += desc;
> +			});
> +			return ret;
> +		    }
> +
> +		    // legacy style and fallback
>  		    var i;
>  		    var text = '';
>  		    var bootdisk = me.getObjectValue('bootdisk', undefined, pending);
> @@ -99,20 +117,20 @@ Ext.define('PVE.qemu.Options', {
>  		    for (i = 0; i < order.length; i++) {
>  			var sel = order.substring(i, i + 1);
>  			if (text) {
> -			    text += ', ';
> +			    text += ' > ';
>  			}
>  			if (sel === 'c') {
>  			    if (bootdisk) {
> -				text += "Disk '" + bootdisk + "'";
> +				text += bootdisk;
>  			    } else {
> -				text += "Disk";
> +				text += "disk";
>  			    }
>  			} else if (sel === 'n') {
> -			    text += 'Network';
> +			    text += 'any net';
>  			} else if (sel === 'a') {
> -			    text += 'Floppy';
> +			    text += 'floppy';
>  			} else if (sel === 'd') {
> -			    text += 'CD-ROM';
> +			    text += 'any cdrom';
>  			} else {
>  			    text += sel;
>  			}
> 

Re: [pve-devel] [PATCH qemu-server 1/2] fix #3010: add 'bootorder' parameter for better control of boot devices

[Thomas Lamprecht]

On 24.09.20 16:11, Stefan Reiter wrote:
> (also fixes #3011)
> 
> Deprecates the old 'boot' and 'bootdisk' options (they still work the
> same, but will get removed if a user sets a 'bootorder' parameter and
> ignored if both are set).

I'd rather re-use boot instead of adding a new property.

Move the current boot format out into it's own format definition, and
change it to a formatsting key=value list with the old format as default_key
for backward compatibillity. The new stuff can be added as new format there,
e.g., "order=<boot-order-list>".

> 
> This allows a user to specify more than one disk in the boot order,
> helping with newer versions of SeaBIOS/OVMF where disks without a
> bootindex won't be initialized at all (breaks soft-raid and some LVM
> setups).
> 
> This also allows specifying a bootindex for USB and hostpci devices,
> which was not possible before. Floppy boot support is not supported in
> the new model, but I doubt that will be a problem (AFAICT we can't even
> attach floppy disks to a VM?).
> 
> Default behaviour is intended to stay the same, i.e. while new VMs will
> receive the new 'bootorder' property, it will be set so the VM starts
> the same as before (see get_default_bootorder).

We probably want to add a boot and/or bootorder config2cmd test before
this patch, helps to guarantee that.

> 
> The API is updated to handle the deprecation correctly, i.e. when
> updating the 'bootorder' attribute, the old properties are removed
> (would now be useless). When removing a device that is in the bootorder
> list, it will be removed from the aforementioned. Note that non-existing
> devices in the list will not cause an error - they will simply be
> ignored - but it's still nice to not have them in there.

But you do not always rewrite it to the new format, i.e., if just another,
unrelated, config property changed, or?


some other comments inline.

> Includes a cfg2cmd test.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> The diff of the network part in config_to_command looks a bit weird, that's
> because the indentation was off by one in the entire block.
> 
>  PVE/API2/Qemu.pm                |  32 +++++-
>  PVE/CLI/qm.pm                   |   4 +-
>  PVE/QemuServer.pm               | 178 ++++++++++++++++++++++++--------
>  PVE/QemuServer/Drive.pm         |  29 ++++--
>  PVE/QemuServer/PCI.pm           |   3 +-
>  PVE/QemuServer/USB.pm           |  14 ++-
>  test/cfg2cmd/bootorder.conf     |  16 +++
>  test/cfg2cmd/bootorder.conf.cmd |  38 +++++++
>  8 files changed, 254 insertions(+), 60 deletions(-)
>  create mode 100644 test/cfg2cmd/bootorder.conf
>  create mode 100644 test/cfg2cmd/bootorder.conf.cmd
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8da616a..f9be475 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -316,6 +316,7 @@ my $vmpoweroptions = {
>  my $diskoptions = {
>      'boot' => 1,
>      'bootdisk' => 1,
> +    'bootorder' => 1,
>      'vmstatestorage' => 1,
>  };
>  
> @@ -656,9 +657,9 @@ __PACKAGE__->register_method({
>  		eval {
>  		    $vollist = &$create_disks($rpcenv, $authuser, $conf, $arch, $storecfg, $vmid, $pool, $param, $storage);
>  
> -		    if (!$conf->{bootdisk}) {
> -			my $firstdisk = PVE::QemuServer::Drive::resolve_first_disk($conf);
> -			$conf->{bootdisk} = $firstdisk if $firstdisk;
> +		    if (!$conf->{bootorder}) {
> +			my $order = PVE::QemuServer::get_default_bootorder($conf);
> +			$conf->{bootorder} = $order;
>  		    }
>  
>  		    # auto generate uuid if user did not specify smbios1 option
> @@ -1191,6 +1192,9 @@ my $update_vm_api  = sub {
>  
>  	    my $modified = {}; # record what $option we modify
>  
> +	    my @bootorder = PVE::Tools::split_list($conf->{bootorder}) if $conf->{bootorder};
> +	    my $bootorder_deleted = grep {$_ eq 'bootorder'} @delete;
> +
>  	    foreach my $opt (@delete) {
>  		$modified->{$opt} = 1;
>  		$conf = PVE::QemuConfig->load_config($vmid); # update/reload
> @@ -1205,6 +1209,13 @@ my $update_vm_api  = sub {
>  		my $is_pending_val = defined($conf->{pending}->{$opt});
>  		delete $conf->{pending}->{$opt};
>  
> +		# remove from bootorder if necessary
> +		if (!$bootorder_deleted && @bootorder && grep {$_ eq $opt} @bootorder) {
> +		    @bootorder = grep {$_ ne $opt} @bootorder;
> +		    $conf->{pending}->{bootorder} = join(',', @bootorder);
> +		    $modified->{bootorder} = 1;
> +		}
> +
>  		if ($opt =~ m/^unused/) {
>  		    my $drive = PVE::QemuServer::parse_drive($opt, $val);
>  		    PVE::QemuConfig->check_protection($conf, "can't remove unused disk '$drive->{file}'");
> @@ -1283,6 +1294,21 @@ my $update_vm_api  = sub {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
> +
> +		    if ($opt eq 'bootorder') {
> +			for my $dev (PVE::Tools::split_list($param->{$opt})) {
> +			    my $exists = $conf->{$dev} || $conf->{pending}->{$dev};
> +			    my $deleted = grep {$_ eq $dev} @delete;
> +			    die "invalid bootorder: device '$dev' does not exist'\n"
> +				if !$exists || $deleted;
> +			}
> +
> +			# remove legacy boot order settings if new one set
> +			PVE::QemuConfig->add_to_pending_delete($conf, "boot")
> +			    if $conf->{boot};
> +			PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
> +			    if $conf->{bootdisk};
> +		    }
>  		}
>  		PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
>  		PVE::QemuConfig->write_config($vmid, $conf);
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 282fa86..ff16636 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -656,8 +656,8 @@ __PACKAGE__->register_method ({
>  
>  	    # reload after disks entries have been created
>  	    $conf = PVE::QemuConfig->load_config($vmid);
> -	    my $firstdisk = PVE::QemuServer::Drive::resolve_first_disk($conf);
> -	    $conf->{bootdisk} = $firstdisk if $firstdisk;
> +	    my $order = PVE::QemuServer::get_default_bootorder($conf);
> +	    $conf->{bootorder} = $order;
>  	    PVE::QemuConfig->write_config($vmid, $conf);
>  	};
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2747c66..3689ef1 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -398,15 +398,33 @@ EODESC
>      boot => {
>  	optional => 1,
>  	type => 'string',
> -	description => "Boot on floppy (a), hard disk (c), CD-ROM (d), or network (n).",
> +	description => "Boot on floppy (a), hard disk (c), CD-ROM (d), or network (n)."
> +		     . " Deprecated: Use 'bootorder' instead.",
>  	pattern => '[acdn]{1,4}',
>  	default => 'cdn',
>      },
>      bootdisk => {
>  	optional => 1,
>  	type => 'string', format => 'pve-qm-bootdisk',
> -	description => "Enable booting from specified disk.",
> -	pattern => '(ide|sata|scsi|virtio)\d+',
> +	description => "Enable booting from specified disk. Deprecated: Use 'bootorder' instead.",
> +    },
> +    bootorder => {
> +	optional => 1,
> +	type => 'string',
> +	format => 'pve-qm-bootdev-list',
> +	description => <<EODESC,
> +The guest will attempt to boot from devices in the order they appear here.
> +
> +Disks, optical drives and passed-through storage USB devices will be directly
> +booted from, NICs will load PXE, and PCIe devices will either behave like disks
> +(e.g. NVMe) or load an option ROM (e.g. RAID controller, hardware NIC).
> +
> +Note that only devices in this list will be marked as bootable and thus loaded
> +by the guest firmware (BIOS/UEFI). If you require multiple disks for booting
> +(e.g. software-raid), you need to specify all of them here.
> +
> +Overrides the deprecated 'boot' and 'bootdisk' properties when given.
> +EODESC
>      },
>      smp => {
>  	optional => 1,
> @@ -689,6 +707,27 @@ EODESCR
>      },
>  };
>  
> +PVE::JSONSchema::register_format('pve-qm-bootdev', \&verify_bootdev);
> +sub verify_bootdev {
> +    my ($dev, $noerr) = @_;
> +
> +    return $dev if PVE::QemuServer::Drive::is_valid_drivename($dev) && $dev !~ m/^efidisk/;
> +
> +    my $check = sub {
> +	my ($base) = @_;
> +	return 0 if $dev !~ m/^$base\d+$/;
> +	return 0 if !$confdesc->{$dev};
> +	return 1;
> +    };
> +
> +    return $dev if $check->("net");
> +    return $dev if $check->("usb");
> +    return $dev if $check->("hostpci");
> +
> +    return undef if $noerr;
> +    die "invalid boot device '$dev'\n";
> +}
> +
>  my $cicustom_fmt = {
>      meta => {
>  	type => 'string',
> @@ -1552,8 +1591,6 @@ sub print_drive_commandline_full {
>  sub print_netdevice_full {
>      my ($vmid, $conf, $net, $netid, $bridges, $use_old_bios_files, $arch, $machine_type) = @_;
>  
> -    my $bootorder = $conf->{boot} || $confdesc->{boot}->{default};
> -
>      my $device = $net->{model};
>      if ($net->{model} eq 'virtio') {
>           $device = 'virtio-net-pci';
> @@ -3151,17 +3188,29 @@ sub config_to_command {
>  	push @$devices, '-device', $kbd if defined($kbd);
>      }
>  
> +    my $bootorder = {};
> +    if ($conf->{bootorder}) {
> +	# start at 100 to allow user to insert devices before us with -args
> +	my $i = 100;
> +	for my $dev (PVE::Tools::split_list($conf->{bootorder})) {
> +	    $bootorder->{$dev} = $i++;
> +	}
> +    } else {
> +	$bootorder = bootorder_from_legacy($conf);
> +    }
> +
>      # host pci device passthrough
>      my ($kvm_off, $gpu_passthrough, $legacy_igd) = PVE::QemuServer::PCI::print_hostpci_devices(
> -	$vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type);
> +	$vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type, $bootorder);
>  
>      # usb devices
>      my $usb_dev_features = {};
>      $usb_dev_features->{spice_usb3} = 1 if min_version($machine_version, 4, 0);
>  
>      my @usbdevices = PVE::QemuServer::USB::get_usb_devices(
> -        $conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features);
> +        $conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features, $bootorder);
>      push @$devices, @usbdevices if @usbdevices;
> +
>      # serial devices
>      for (my $i = 0; $i < $MAX_SERIAL_PORTS; $i++)  {
>  	if (my $path = $conf->{"serial$i"}) {
> @@ -3229,15 +3278,6 @@ sub config_to_command {
>      }
>      push @$cmd, '-nodefaults';
>  
> -    my $bootorder = $conf->{boot} || $confdesc->{boot}->{default};
> -
> -    my $bootindex_hash = {};
> -    my $i = 1;
> -    foreach my $o (split(//, $bootorder)) {
> -	$bootindex_hash->{$o} = $i*100;
> -	$i++;
> -    }
> -
>      push @$cmd, '-boot', "menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg";
>  
>      push @$cmd, '-no-acpi' if defined($conf->{acpi}) && $conf->{acpi} == 0;
> @@ -3407,17 +3447,7 @@ sub config_to_command {
>  
>  	$use_virtio = 1 if $ds =~ m/^virtio/;
>  
> -	if (drive_is_cdrom ($drive)) {
> -	    if ($bootindex_hash->{d}) {
> -		$drive->{bootindex} = $bootindex_hash->{d};
> -		$bootindex_hash->{d} += 1;
> -	    }
> -	} else {
> -	    if ($bootindex_hash->{c}) {
> -		$drive->{bootindex} = $bootindex_hash->{c} if $conf->{bootdisk} && ($conf->{bootdisk} eq $ds);
> -		$bootindex_hash->{c} += 1;
> -	    }
> -	}
> +	$drive->{bootindex} = $bootorder->{$ds} if $bootorder->{$ds};
>  
>  	if ($drive->{interface} eq 'virtio'){
>             push @$cmd, '-object', "iothread,id=iothread-$ds" if $drive->{iothread};
> @@ -3468,24 +3498,23 @@ sub config_to_command {
>      });
>  
>      for (my $i = 0; $i < $MAX_NETS; $i++) {
> -	 next if !$conf->{"net$i"};
> -	 my $d = parse_net($conf->{"net$i"});
> -	 next if !$d;
> +	my $netname = "net$i";
>  
> -	 $use_virtio = 1 if $d->{model} eq 'virtio';
> +	next if !$conf->{$netname};
> +	my $d = parse_net($conf->{$netname});
> +	next if !$d;
>  

those changes here seem mostly unrelated to this patch, maybe pull them out in a extra
patch.

> -	 if ($bootindex_hash->{n}) {
> -	    $d->{bootindex} = $bootindex_hash->{n};
> -	    $bootindex_hash->{n} += 1;
> -	 }
> +	$use_virtio = 1 if $d->{model} eq 'virtio';
>  
> -	 my $netdevfull = print_netdev_full($vmid, $conf, $arch, $d, "net$i");
> -	 push @$devices, '-netdev', $netdevfull;
> +	$d->{bootindex} = $bootorder->{$netname} if $bootorder->{$netname};
>  
> -	 my $netdevicefull = print_netdevice_full(
> -	    $vmid, $conf, $d, "net$i", $bridges, $use_old_bios_files, $arch, $machine_type);
> +	my $netdevfull = print_netdev_full($vmid, $conf, $arch, $d, $netname);
> +	push @$devices, '-netdev', $netdevfull;
>  
> -	 push @$devices, '-device', $netdevicefull;
> +	my $netdevicefull = print_netdevice_full(
> +	$vmid, $conf, $d, $netname, $bridges, $use_old_bios_files, $arch, $machine_type);
> +
> +	push @$devices, '-device', $netdevicefull;
>      }
>  
>      if ($conf->{ivshmem}) {
> @@ -3765,7 +3794,8 @@ sub vm_deviceunplug {
>      my $devices_list = vm_devices_list($vmid);
>      return 1 if !defined($devices_list->{$deviceid});
>  
> -    die "can't unplug bootdisk" if $conf->{bootdisk} && $conf->{bootdisk} eq $deviceid;
> +    my $bootdisks = PVE::QemuServer::Drive::get_bootdisks($conf);
> +    die "can't unplug bootdisk '$deviceid'\n" if grep {$_ eq $deviceid} @$bootdisks;
>  
>      if ($deviceid eq 'tablet' || $deviceid eq 'keyboard') {
>  
> @@ -7152,6 +7182,72 @@ sub clear_reboot_request {
>      return $res;
>  }
>  
> +sub bootorder_from_legacy {
> +    my ($conf) = @_;

we could add that in a patch before this already, switching call sites over,
that would reduce the size (reviewabillity) of this.

I.e., new order could be:

# general cleanups, no new stuff
# test for current boot/bootorder stuff
# factor out current stuff
# add new functionality


> +
> +    my $boot = $conf->{boot} || $confdesc->{boot}->{default};
> +    my $bootindex_hash = {};
> +    my $i = 1;
> +    foreach my $o (split(//, $boot)) {
> +	$bootindex_hash->{$o} = $i*100;
> +	$i++;
> +    }
> +
> +    my $bootorder = {};
> +
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +
> +	if (drive_is_cdrom ($drive, 1)) {
> +	    if ($bootindex_hash->{d}) {
> +		$bootorder->{$ds} = $bootindex_hash->{d};
> +		$bootindex_hash->{d} += 1;
> +	    }
> +	} elsif ($bootindex_hash->{c} && $conf->{bootdisk} && $conf->{bootdisk} eq $ds) {
> +	    $bootorder->{$ds} = $bootindex_hash->{c};
> +	}
> +    });
> +
> +    if ($bootindex_hash->{n}) {
> +	for (my $i = 0; $i < $MAX_NETS; $i++) {
> +	    my $netname = "net$i";
> +	    next if !$conf->{$netname};
> +	    $bootorder->{$netname} = $bootindex_hash->{n};
> +	    $bootindex_hash->{n} += 1;
> +	}
> +    }
> +
> +    return $bootorder;
> +}
> +
> +# Matches legacy default boot order, but with explicit device names. This is
> +# somewhat important, since the fallback for when neither 'bootorder' nor the
> +# old 'boot'/'bootdisk' is specified relies on 'bootorder_from_legacy' above,
> +# and it would be confusing if this diverges.
> +sub get_default_bootorder {
> +    my ($conf) = @_;
> +
> +    my @ret = ();
> +
> +    # harddisk
> +    my $first = PVE::QemuServer::Drive::resolve_first_disk($conf, 0);
> +    push @ret, $first if $first;
> +
> +    # cdrom
> +    $first = PVE::QemuServer::Drive::resolve_first_disk($conf, 1);
> +    push @ret, $first if $first;
> +
> +    # network
> +    for (my $i = 0; $i < $MAX_NETS; $i++) {
> +	my $netname = "net$i";
> +	next if !$conf->{$netname};
> +	push @ret, $netname;
> +	last;
> +    }
> +
> +    return join(',', @ret);
> +}
> +
>  # bash completion helper
>  
>  sub complete_backup_archives {
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 91c33f8..af25882 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -501,11 +501,25 @@ sub print_drive {
>      return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
>  }
>  
> +sub get_bootdisks {
> +    my ($conf) = @_;
> +
> +    if (!$conf->{bootorder}) {
> +	return [$conf->{bootdisk}] if $conf->{bootdisk};
> +	return [];
> +    }
> +
> +    my @list = PVE::Tools::split_list($conf->{bootorder});
> +    @list = grep {is_valid_drivename($_)} @list;
> +    return \@list;
> +}
> +
>  sub bootdisk_size {
>      my ($storecfg, $conf) = @_;
>  
> -    my $bootdisk = $conf->{bootdisk};
> -    return undef if !$bootdisk;
> +    my $bootdisks = get_bootdisks($conf);
> +    return undef if !@$bootdisks;
> +    my $bootdisk = $bootdisks->[0];
>      return undef if !is_valid_drivename($bootdisk);
>  
>      return undef if !$conf->{$bootdisk};
> @@ -584,16 +598,15 @@ sub is_volume_in_use {
>  }
>  
>  sub resolve_first_disk {
> -    my $conf = shift;
> +    my ($conf, $cdrom) = @_;
>      my @disks = valid_drive_names();
> -    my $firstdisk;
> -    foreach my $ds (reverse @disks) {
> +    foreach my $ds (@disks) {
>  	next if !$conf->{$ds};
>  	my $disk = parse_drive($ds, $conf->{$ds});
> -	next if drive_is_cdrom($disk);
> -	$firstdisk = $ds;
> +	next if !(drive_is_cdrom($disk) xor $cdrom);
> +	return $ds;
>      }
> -    return $firstdisk;
> +    return undef;
>  }
>  
>  1;
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index cb36845..2df2708 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -357,7 +357,7 @@ sub parse_hostpci {
>  }
>  
>  sub print_hostpci_devices {
> -    my ($vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type) = @_;
> +    my ($vmid, $conf, $devices, $winversion, $q35, $bridges, $arch, $machine_type, $bootorder) = @_;
>  
>      my $kvm_off = 0;
>      my $gpu_passthrough = 0;
> @@ -446,6 +446,7 @@ sub print_hostpci_devices {
>  		$devicestr .= "$xvga";
>  		$devicestr .= ",multifunction=on" if $multifunction;
>  		$devicestr .= ",romfile=/usr/share/kvm/$d->{romfile}" if $d->{romfile};
> +		$devicestr .= ",bootindex=$bootorder->{$id}" if $bootorder->{$id};
>  	    }
>  
>  	    push @$devices, '-device', $devicestr;
> diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
> index d328148..4a843cd 100644
> --- a/PVE/QemuServer/USB.pm
> +++ b/PVE/QemuServer/USB.pm
> @@ -74,13 +74,14 @@ sub get_usb_controllers {
>  }
>  
>  sub get_usb_devices {
> -    my ($conf, $format, $max_usb_devices, $features) = @_;
> +    my ($conf, $format, $max_usb_devices, $features, $bootorder) = @_;
>  
>      my $devices = [];
>  
>      for (my $i = 0; $i < $max_usb_devices; $i++)  {
> -	next if !$conf->{"usb$i"};
> -	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{"usb$i"}) };
> +	my $devname = "usb$i";
> +	next if !$conf->{$devname};
> +	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{$devname}) };
>  	next if !$d;
>  
>  	if (defined($d->{host})) {
> @@ -93,8 +94,10 @@ sub get_usb_devices {
>  
>  		push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
>  		push @$devices, '-device', "usb-redir,chardev=usbredirchardev$i,id=usbredirdev$i,bus=$bus.0";
> +
> +		warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname};
>  	    } else {
> -		push @$devices, '-device', print_usbdevice_full($conf, "usb$i", $hostdevice);
> +		push @$devices, '-device', print_usbdevice_full($conf, $devname, $hostdevice, $bootorder);
>  	    }
>  	}
>      }
> @@ -103,7 +106,7 @@ sub get_usb_devices {
>  }
>  
>  sub print_usbdevice_full {
> -    my ($conf, $deviceid, $device) = @_;
> +    my ($conf, $deviceid, $device, $bootorder) = @_;
>  
>      return if !$device;
>      my $usbdevice = "usb-host";
> @@ -120,6 +123,7 @@ sub print_usbdevice_full {
>      }
>  
>      $usbdevice .= ",id=$deviceid";
> +    $usbdevice .= ",bootindex=$bootorder->{$deviceid}" if $bootorder->{$deviceid};
>      return $usbdevice;
>  }
>  
> diff --git a/test/cfg2cmd/bootorder.conf b/test/cfg2cmd/bootorder.conf
> new file mode 100644
> index 0000000..7d296da
> --- /dev/null
> +++ b/test/cfg2cmd/bootorder.conf
> @@ -0,0 +1,16 @@
> +# TEST: Test for a specific bootorder given by 'bootorder' parameter
> +# QEMU_VERSION: 5.1
> +cores: 3
> +bootorder: virtio1,net0,scsi4,ide2
> +ide2: none,media=cdrom
> +memory: 768
> +name: simple
> +net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
> +numa: 0
> +ostype: l26
> +scsi4: local:8006/vm-8006-disk-0.qcow2,discard=on,size=104858K
> +smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
> +sockets: 1
> +virtio0: local:8006/vm-8006-disk-0.qcow2,discard=on,iothread=1,size=104858K
> +virtio1: local:8006/vm-8006-disk-0.qcow2,discard=on,iothread=1,size=104858K
> +vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
> diff --git a/test/cfg2cmd/bootorder.conf.cmd b/test/cfg2cmd/bootorder.conf.cmd
> new file mode 100644
> index 0000000..86cae07
> --- /dev/null
> +++ b/test/cfg2cmd/bootorder.conf.cmd
> @@ -0,0 +1,38 @@
> +/usr/bin/kvm \
> +  -id 8006 \
> +  -name simple \
> +  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server,nowait' \
> +  -mon 'chardev=qmp,mode=control' \
> +  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
> +  -mon 'chardev=qmp-event,mode=control' \
> +  -pidfile /var/run/qemu-server/8006.pid \
> +  -daemonize \
> +  -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
> +  -smp '3,sockets=1,cores=3,maxcpus=3' \
> +  -nodefaults \
> +  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> +  -vnc unix:/var/run/qemu-server/8006.vnc,password \
> +  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
> +  -m 768 \
> +  -object 'iothread,id=iothread-virtio0' \
> +  -object 'iothread,id=iothread-virtio1' \
> +  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
> +  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
> +  -device 'vmgenid,guid=c773c261-d800-4348-9f5d-167fadd53cf8' \
> +  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
> +  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
> +  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
> +  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
> +  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
> +  -drive 'if=none,id=drive-ide2,media=cdrom,aio=threads' \
> +  -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=103' \
> +  -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
> +  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-scsi4,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
> +  -device 'scsi-hd,bus=scsihw0.0,scsi-id=4,drive=drive-scsi4,id=scsi4,bootindex=102' \
> +  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-virtio0,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
> +  -device 'virtio-blk-pci,drive=drive-virtio0,id=virtio0,bus=pci.0,addr=0xa,iothread=iothread-virtio0' \
> +  -drive 'file=/var/lib/vz/images/8006/vm-8006-disk-0.qcow2,if=none,id=drive-virtio1,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap' \
> +  -device 'virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1,bootindex=100' \
> +  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
> +  -device 'virtio-net-pci,mac=A2:C0:43:77:08:A0,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=101' \
> +  -machine 'type=pc+pve0'
> 

Re: [pve-devel] [PATCH qemu-server 1/2] fix #3010: add 'bootorder' parameter for better control of boot devices

[Stefan Reiter]

On 9/28/20 2:18 PM, Thomas Lamprecht wrote:
> On 24.09.20 16:11, Stefan Reiter wrote:
>> (also fixes #3011)
>>
>> Deprecates the old 'boot' and 'bootdisk' options (they still work the
>> same, but will get removed if a user sets a 'bootorder' parameter and
>> ignored if both are set).
> 
> I'd rather re-use boot instead of adding a new property.
> 
> Move the current boot format out into it's own format definition, and
> change it to a formatsting key=value list with the old format as default_key
> for backward compatibillity. The new stuff can be added as new format there,
> e.g., "order=<boot-order-list>".
> 

Makes sense, I'll change it.

>>
>> This allows a user to specify more than one disk in the boot order,
>> helping with newer versions of SeaBIOS/OVMF where disks without a
>> bootindex won't be initialized at all (breaks soft-raid and some LVM
>> setups).
>>
>> This also allows specifying a bootindex for USB and hostpci devices,
>> which was not possible before. Floppy boot support is not supported in
>> the new model, but I doubt that will be a problem (AFAICT we can't even
>> attach floppy disks to a VM?).
>>
>> Default behaviour is intended to stay the same, i.e. while new VMs will
>> receive the new 'bootorder' property, it will be set so the VM starts
>> the same as before (see get_default_bootorder).
> 
> We probably want to add a boot and/or bootorder config2cmd test before
> this patch, helps to guarantee that.
> 

Existing tests already check 'bootdisk' property, but I'll add one for 
'boot' too.

>>
>> The API is updated to handle the deprecation correctly, i.e. when
>> updating the 'bootorder' attribute, the old properties are removed
>> (would now be useless). When removing a device that is in the bootorder
>> list, it will be removed from the aforementioned. Note that non-existing
>> devices in the list will not cause an error - they will simply be
>> ignored - but it's still nice to not have them in there.
> 
> But you do not always rewrite it to the new format, i.e., if just another,
> unrelated, config property changed, or?
> 

No, only when bootorder is added (or order= is modified in new version 
then).

I'll also split it into multiple patches.