[PATCH qemu-server v5 0/4] hotplug: improve and fix cleanup/error handling

[PATCH qemu-server v5 0/4] hotplug: improve and fix cleanup/error handling

[Dominik Csapak]

This replaces my earlier v4 9/9 patch[0]

It uses a different approach now by unifying the cleanup and removal of
disks and their controllers.

It also fixes a related bug in 1/4 that was uncovered.

changes from v4:
* rebase on master
* new bugfix (1/4)
* new patch for iothread deletion
* new patch for refactoring the cleanup

0: https://lore.proxmox.com/pve-devel/20261023130324.451252-10-d.csapak@proxmox.com/

Dominik Csapak (4):
  hotplug: scsi: only keep controller if a disk on it is still present
  hotplug: remove iothread with virtio-scsi controller together
  hotplug: factor out cleanup of disk devices
  hotplug: clean up disk devices/objects if adding them failed

 src/PVE/QemuServer.pm                         | 107 ++++++++++--------
 src/test/hotplug/disk-add-fail.conf           |  19 ++++
 src/test/hotplug/disk-add-fail.conf.expected  |  10 ++
 .../disk-add-scsi-single-disk-fail.conf       |  19 ++++
 ...sk-add-scsi-single-disk-fail.conf.expected |  12 ++
 .../hotplug/disk-add-scsi-single-fail.conf    |  19 ++++
 .../disk-add-scsi-single-fail.conf.expected   |   5 +
 .../hotplug/disk-remove-lsi-last-slot.conf    |  16 +++
 .../disk-remove-lsi-last-slot.conf.expected   |   3 +
 .../disk-remove-lsi-second-controller.conf    |  16 +++
 ...remove-lsi-second-controller.conf.expected |   4 +
 11 files changed, 183 insertions(+), 47 deletions(-)
 create mode 100644 src/test/hotplug/disk-add-fail.conf
 create mode 100644 src/test/hotplug/disk-add-fail.conf.expected
 create mode 100644 src/test/hotplug/disk-add-scsi-single-disk-fail.conf
 create mode 100644 src/test/hotplug/disk-add-scsi-single-disk-fail.conf.expected
 create mode 100644 src/test/hotplug/disk-add-scsi-single-fail.conf
 create mode 100644 src/test/hotplug/disk-add-scsi-single-fail.conf.expected
 create mode 100644 src/test/hotplug/disk-remove-lsi-last-slot.conf
 create mode 100644 src/test/hotplug/disk-remove-lsi-last-slot.conf.expected
 create mode 100644 src/test/hotplug/disk-remove-lsi-second-controller.conf
 create mode 100644 src/test/hotplug/disk-remove-lsi-second-controller.conf.expected

-- 
2.47.3

[PATCH qemu-server v5 1/4] hotplug: scsi: only keep controller if a disk on it is still present

[Dominik Csapak]

The check whether a non-single SCSI controller is still in use compared
the disk index against (maxdev - 1) * (controller + 1). For LSI (7 disks
per controller), this ignores the last slot of the first controller for
example, and treats any disk on a lower controller as user of the higher
ones.

So unplugging scsi0 with only scsi6 left unplugged the controller
that scsi6 is still attached to, while unplugging scsi7 left the then
unused second controller behind.

Use scsihw_infos() to determine the controller of each remaining
disk, like when plugging them, and only keep the controller if one of
them is on it.

Also don't rely on the drive being in the config, since on hotplug
cleanup this does not have to be the case. This combines the index
extraction with the check for 'scsi'.

The added regression tests would have failed without the code change.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm                            | 13 ++++---------
 src/test/hotplug/disk-remove-lsi-last-slot.conf  | 16 ++++++++++++++++
 .../disk-remove-lsi-last-slot.conf.expected      |  3 +++
 .../disk-remove-lsi-second-controller.conf       | 16 ++++++++++++++++
 ...sk-remove-lsi-second-controller.conf.expected |  4 ++++
 5 files changed, 43 insertions(+), 9 deletions(-)
 create mode 100644 src/test/hotplug/disk-remove-lsi-last-slot.conf
 create mode 100644 src/test/hotplug/disk-remove-lsi-last-slot.conf.expected
 create mode 100644 src/test/hotplug/disk-remove-lsi-second-controller.conf
 create mode 100644 src/test/hotplug/disk-remove-lsi-second-controller.conf.expected

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index b4a1a38d..6ac3c685 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4144,15 +4144,10 @@ sub qemu_deletescsihw {
 
     my $devices_list = vm_devices_list($vmid);
     foreach my $opt (keys %{$devices_list}) {
-        if (is_valid_drivename($opt)) {
-            my $drive = parse_drive($opt, $conf->{$opt});
-            if (
-                $drive->{interface} eq 'scsi'
-                && $drive->{index} < (($maxdev - 1) * ($controller + 1))
-            ) {
-                return 1;
-            }
-        }
+        next if $opt !~ m/^scsi(\d+)$/;
+        my $index = $1;
+        my (undef, $drive_controller) = scsihw_infos($conf->{scsihw}, $index);
+        return 1 if $drive_controller == $controller;
     }
 
     my $scsihwid = "scsihw$controller";
diff --git a/src/test/hotplug/disk-remove-lsi-last-slot.conf b/src/test/hotplug/disk-remove-lsi-last-slot.conf
new file mode 100644
index 00000000..aa50098b
--- /dev/null
+++ b/src/test/hotplug/disk-remove-lsi-last-slot.conf
@@ -0,0 +1,16 @@
+# TEST: hot-unplug a SCSI disk keeps the LSI controller if a disk in its last slot still uses it
+bootdisk: scsi6
+cores: 2
+machine: pc-i440fx-10.1
+memory: 2048
+name: hotplug
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+ostype: l26
+scsi0: local:8006/vm-8006-disk-0.qcow2,size=32G
+scsi6: local:8006/vm-8006-disk-1.qcow2,size=32G
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
+
+[PENDING]
+delete: scsi0
diff --git a/src/test/hotplug/disk-remove-lsi-last-slot.conf.expected b/src/test/hotplug/disk-remove-lsi-last-slot.conf.expected
new file mode 100644
index 00000000..e24d5479
--- /dev/null
+++ b/src/test/hotplug/disk-remove-lsi-last-slot.conf.expected
@@ -0,0 +1,3 @@
+hmp device_del scsi0
+qmp blockdev-del {"node-name":"drive-scsi0"}
+qmp object-del {"id":"throttle-drive-scsi0"}
diff --git a/src/test/hotplug/disk-remove-lsi-second-controller.conf b/src/test/hotplug/disk-remove-lsi-second-controller.conf
new file mode 100644
index 00000000..fc32cda6
--- /dev/null
+++ b/src/test/hotplug/disk-remove-lsi-second-controller.conf
@@ -0,0 +1,16 @@
+# TEST: hot-unplug the last SCSI disk of the second LSI controller also removes that controller
+bootdisk: scsi0
+cores: 2
+machine: pc-i440fx-10.1
+memory: 2048
+name: hotplug
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+ostype: l26
+scsi0: local:8006/vm-8006-disk-0.qcow2,size=32G
+scsi7: local:8006/vm-8006-disk-1.qcow2,size=32G
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
+
+[PENDING]
+delete: scsi7
diff --git a/src/test/hotplug/disk-remove-lsi-second-controller.conf.expected b/src/test/hotplug/disk-remove-lsi-second-controller.conf.expected
new file mode 100644
index 00000000..1287300a
--- /dev/null
+++ b/src/test/hotplug/disk-remove-lsi-second-controller.conf.expected
@@ -0,0 +1,4 @@
+hmp device_del scsi7
+qmp blockdev-del {"node-name":"drive-scsi7"}
+qmp object-del {"id":"throttle-drive-scsi7"}
+hmp device_del scsihw1
-- 
2.47.3

[PATCH qemu-server v5 2/4] hotplug: remove iothread with virtio-scsi controller together

[Dominik Csapak]

With virtio-scsi-single, the iothread is used by the controller
'virtioscsiN' and not by the disk 'scsiN', but it was removed when
unplugging the disk (which was done after removal of the controller).

Remove the iothread together with the controller instead, and decide
whether to remove it by checking the iothreads that actually exist
instead of the config. This way, the controller can be unplugged
without having the config of its disk, which is necessary for cleaning
up after a failed hotplug in a following commit.

Since an 'iothread-<id>' object is only used by the device with that
id, it is safe to remove it once the device is gone.

Note that this now executes an additional qmp command for disks that
don't have an iothread in the config, but since this is not called that
often (once per disk config change) it should be ok.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 6ac3c685..38a4e4f8 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3972,25 +3972,20 @@ sub vm_deviceunplug {
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
     } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
-        my $device = parse_drive($deviceid, $conf->{$deviceid});
-
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
         qemu_drivedel($vmid, $deviceid);
-        qemu_iothread_del($vmid, $deviceid, $device);
+        qemu_iothread_del($vmid, $deviceid);
     } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid, 15);
+        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
     } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
-        my $device = parse_drive($deviceid, $conf->{$deviceid});
-
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
         qemu_drivedel($vmid, $deviceid);
+        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
         qemu_deletescsihw($conf, $vmid, $deviceid);
-
-        qemu_iothread_del($vmid, "virtioscsi$device->{index}", $device)
-            if $conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single');
     } elsif ($deviceid =~ m/^(net)(\d+)$/) {
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
@@ -4031,12 +4026,10 @@ sub qemu_iothread_add {
 }
 
 sub qemu_iothread_del {
-    my ($vmid, $deviceid, $device) = @_;
+    my ($vmid, $deviceid) = @_;
 
-    if ($device->{iothread}) {
-        my $iothreads = vm_iothreads_list($vmid);
-        qemu_objectdel($vmid, "iothread-$deviceid") if $iothreads->{"iothread-$deviceid"};
-    }
+    my $iothreads = vm_iothreads_list($vmid);
+    qemu_objectdel($vmid, "iothread-$deviceid") if $iothreads->{"iothread-$deviceid"};
 }
 
 sub qemu_driveadd {
-- 
2.47.3

[PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Dominik Csapak]

Unplugging a disk or SCSI controller removes the same parts that need to
be removed when hotplugging one of them failed. Factor this out into a
helper, so it can be reused for cleaning up after a failed hotplug in a
following commit.

For that case, the helper can warn instead of die on errors, so that
the remaining parts are still removed and the original error can be
reported.

Pass the parsed drive to qemu_deletescsihw instead of parsing it from
the config again, since a disk that failed to hotplug is still pending
and not in the config yet.

No functional change intended.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 38a4e4f8..3811bff2 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3854,6 +3854,32 @@ sub vm_devices_list {
     return $devices;
 }
 
+# clean up disks and their controllers (if unused afterwards)
+sub cleanup_disk {
+    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
+
+    my $warn_or_die = sub {
+        my ($sub) = @_;
+        eval { $sub->(); };
+        if (my $err = $@) {
+            die $err if !$noerr;
+            warn $err;
+        }
+    };
+
+    $warn_or_die->(sub {
+        qemu_devicedel($vmid, $deviceid);
+        qemu_devicedelverify($vmid, $deviceid, $timeout);
+    });
+    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if $deviceid =~ m/^(virtio|scsi)\d+$/;
+    # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
+    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid, $device); }) if $deviceid =~ m/^scsi\d+$/;
+    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
+        if $deviceid =~ m/^virtio(scsi)?\d+$/;
+
+    return;
+}
+
 sub vm_deviceplug {
     my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
 
@@ -3972,20 +3998,12 @@ sub vm_deviceunplug {
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
     } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
-        qemu_devicedel($vmid, $deviceid);
-        qemu_devicedelverify($vmid, $deviceid);
-        qemu_drivedel($vmid, $deviceid);
-        qemu_iothread_del($vmid, $deviceid);
+        cleanup_disk($conf, $vmid, $deviceid);
     } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
-        qemu_devicedel($vmid, $deviceid);
-        qemu_devicedelverify($vmid, $deviceid, 15);
-        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
+        cleanup_disk($conf, $vmid, $deviceid, undef, 15);
     } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
-        qemu_devicedel($vmid, $deviceid);
-        qemu_devicedelverify($vmid, $deviceid);
-        qemu_drivedel($vmid, $deviceid);
-        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
-        qemu_deletescsihw($conf, $vmid, $deviceid);
+        my $device = parse_drive($deviceid, $conf->{$deviceid});
+        cleanup_disk($conf, $vmid, $deviceid, $device);
     } elsif ($deviceid =~ m/^(net)(\d+)$/) {
         qemu_devicedel($vmid, $deviceid);
         qemu_devicedelverify($vmid, $deviceid);
@@ -4124,9 +4142,7 @@ sub qemu_findorcreatescsihw {
 }
 
 sub qemu_deletescsihw {
-    my ($conf, $vmid, $opt) = @_;
-
-    my $device = parse_drive($opt, $conf->{$opt});
+    my ($conf, $vmid, $device) = @_;
 
     if ($conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single')) {
         vm_deviceunplug($vmid, $conf, "virtioscsi$device->{index}");
-- 
2.47.3

[PATCH qemu-server v5 4/4] hotplug: clean up disk devices/objects if adding them failed

[Dominik Csapak]

Otherwise, parts of a disk are left over after a failed attempt to add
it. For VirtIO block disks this was the iothread. For SCSI disks, the
newly added controller was left over, including its iothread in case
of virtio-scsi-single, and if adding the controller itself failed, its
iothread too.

Use the same cleanup as for unplugging, so that all parts that might
have been added are removed again.

Also verify that a hotplugged SCSI disk actually shows up, like for
VirtIO block disks. Since errors of the HMP device_add command are not
reported, a failure was not noticed and the disk was moved from the
pending section into the config.

The failure can be tested by disabling hotplug, e.g. for an i440fx guest
one can use the following command line switches:

-global PIIX4_PM.acpi-root-pci-hotplug=off
-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off
-global pci-bridge.shpc=off

to disable hotplug on all PCI bridges and the PCI root bus.

The added regression tests would have failed without the code change.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/QemuServer.pm                         | 37 ++++++++++++-------
 src/test/hotplug/disk-add-fail.conf           | 19 ++++++++++
 src/test/hotplug/disk-add-fail.conf.expected  | 10 +++++
 .../disk-add-scsi-single-disk-fail.conf       | 19 ++++++++++
 ...sk-add-scsi-single-disk-fail.conf.expected | 12 ++++++
 .../hotplug/disk-add-scsi-single-fail.conf    | 19 ++++++++++
 .../disk-add-scsi-single-fail.conf.expected   |  5 +++
 7 files changed, 107 insertions(+), 14 deletions(-)
 create mode 100644 src/test/hotplug/disk-add-fail.conf
 create mode 100644 src/test/hotplug/disk-add-fail.conf.expected
 create mode 100644 src/test/hotplug/disk-add-scsi-single-disk-fail.conf
 create mode 100644 src/test/hotplug/disk-add-scsi-single-disk-fail.conf.expected
 create mode 100644 src/test/hotplug/disk-add-scsi-single-fail.conf
 create mode 100644 src/test/hotplug/disk-add-scsi-single-fail.conf.expected

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 3811bff2..7285a297 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3903,17 +3903,18 @@ sub vm_deviceplug {
             PVE::QemuServer::USB::print_usbdevice_full($conf, $deviceid, $device, {}, $1 + 1),
         );
     } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
-        qemu_iothread_add($vmid, $deviceid, $device);
-
-        qemu_driveadd($storecfg, $vmid, $device);
         my $devicefull =
             print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);
 
-        qemu_deviceadd($vmid, $devicefull);
-        eval { qemu_deviceaddverify($vmid, $deviceid); };
+        eval {
+            qemu_iothread_add($vmid, $deviceid, $device);
+            qemu_driveadd($storecfg, $vmid, $device);
+            qemu_deviceadd($vmid, $devicefull);
+            qemu_deviceaddverify($vmid, $deviceid);
+        };
+
         if (my $err = $@) {
-            eval { qemu_drivedel($vmid, $deviceid); };
-            warn $@ if $@;
+            cleanup_disk($conf, $vmid, $deviceid, $device, undef, 1);
             die $err;
         }
     } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
@@ -3924,7 +3925,6 @@ sub vm_deviceplug {
         my $devicefull = "$scsihw_type,id=$deviceid$pciaddr";
 
         if ($deviceid =~ m/^virtioscsi(\d+)$/ && $device->{iothread}) {
-            qemu_iothread_add($vmid, $deviceid, $device);
             $devicefull .= ",iothread=iothread-$deviceid";
         }
 
@@ -3932,18 +3932,27 @@ sub vm_deviceplug {
             $devicefull .= ",num_queues=$device->{queues}";
         }
 
-        qemu_deviceadd($vmid, $devicefull);
-        qemu_deviceaddverify($vmid, $deviceid);
+        eval {
+            qemu_iothread_add($vmid, $deviceid, $device) if $deviceid =~ m/^virtioscsi/;
+            qemu_deviceadd($vmid, $devicefull);
+            qemu_deviceaddverify($vmid, $deviceid);
+        };
+        if (my $err = $@) {
+            cleanup_disk($conf, $vmid, $deviceid, $device, undef, 1);
+            die $err;
+        }
     } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
         qemu_findorcreatescsihw($storecfg, $conf, $vmid, $device, $arch, $machine_type);
-        qemu_driveadd($storecfg, $vmid, $device);
 
         my $devicefull =
             print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);
-        eval { qemu_deviceadd($vmid, $devicefull); };
+        eval {
+            qemu_driveadd($storecfg, $vmid, $device);
+            qemu_deviceadd($vmid, $devicefull);
+            qemu_deviceaddverify($vmid, $deviceid);
+        };
         if (my $err = $@) {
-            eval { qemu_drivedel($vmid, $deviceid); };
-            warn $@ if $@;
+            cleanup_disk($conf, $vmid, $deviceid, $device, undef, 1);
             die $err;
         }
     } elsif ($deviceid =~ m/^(net)(\d+)$/) {
diff --git a/src/test/hotplug/disk-add-fail.conf b/src/test/hotplug/disk-add-fail.conf
new file mode 100644
index 00000000..800b113e
--- /dev/null
+++ b/src/test/hotplug/disk-add-fail.conf
@@ -0,0 +1,19 @@
+# TEST: a disk that does not show up after hotplug is detached again
+# FAIL_DEVICE_ADD: virtio1
+# EXPECTED_ERROR: virtio1: hotplug problem - error on hotplug device 'virtio1'
+# STAYS_PENDING: 1
+bootdisk: scsi0
+cores: 2
+machine: pc-i440fx-10.1
+memory: 2048
+name: hotplug
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+ostype: l26
+scsi0: local:8006/vm-8006-disk-0.qcow2,size=32G
+scsihw: virtio-scsi-pci
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
+
+[PENDING]
+virtio1: local:8006/vm-8006-disk-1.qcow2,iothread=1,size=32G
diff --git a/src/test/hotplug/disk-add-fail.conf.expected b/src/test/hotplug/disk-add-fail.conf.expected
new file mode 100644
index 00000000..30946720
--- /dev/null
+++ b/src/test/hotplug/disk-add-fail.conf.expected
@@ -0,0 +1,10 @@
+storage activate_volumes ["local:8006/vm-8006-disk-1.qcow2"]
+qmp object-add {"id":"iothread-virtio1","qom-type":"iothread"}
+qmp object-del {"id":"throttle-drive-virtio1"}
+qmp object-add {"id":"throttle-drive-virtio1","limits":{},"qom-type":"throttle-group"}
+qmp blockdev-add {"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/8006/vm-8006-disk-1.qcow2","node-name":"ef4c217a7f08cf59741edc1790023f5","read-only":false},"node-name":"ff4c217a7f08cf59741edc1790023f5","read-only":false},"node-name":"drive-virtio1","read-only":false,"throttle-group":"throttle-drive-virtio1"}
+hmp device_add driver=virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pci.0,addr=0xb,iothread=iothread-virtio1,write-cache=on
+hmp device_del virtio1
+qmp blockdev-del {"node-name":"drive-virtio1"}
+qmp object-del {"id":"throttle-drive-virtio1"}
+qmp object-del {"id":"iothread-virtio1"}
diff --git a/src/test/hotplug/disk-add-scsi-single-disk-fail.conf b/src/test/hotplug/disk-add-scsi-single-disk-fail.conf
new file mode 100644
index 00000000..f493f589
--- /dev/null
+++ b/src/test/hotplug/disk-add-scsi-single-disk-fail.conf
@@ -0,0 +1,19 @@
+# TEST: a disk on virtio-scsi-single that does not show up after hotplug has its controller and iothread removed again
+# FAIL_DEVICE_ADD: scsi1
+# EXPECTED_ERROR: scsi1: hotplug problem - error on hotplug device 'scsi1'
+# STAYS_PENDING: 1
+bootdisk: scsi0
+cores: 2
+machine: pc-i440fx-10.1
+memory: 2048
+name: hotplug
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+ostype: l26
+scsi0: local:8006/vm-8006-disk-0.qcow2,iothread=1,size=32G
+scsihw: virtio-scsi-single
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
+
+[PENDING]
+scsi1: local:8006/vm-8006-disk-1.qcow2,iothread=1,size=32G
diff --git a/src/test/hotplug/disk-add-scsi-single-disk-fail.conf.expected b/src/test/hotplug/disk-add-scsi-single-disk-fail.conf.expected
new file mode 100644
index 00000000..a5da2480
--- /dev/null
+++ b/src/test/hotplug/disk-add-scsi-single-disk-fail.conf.expected
@@ -0,0 +1,12 @@
+storage activate_volumes ["local:8006/vm-8006-disk-1.qcow2"]
+qmp object-add {"id":"iothread-virtioscsi1","qom-type":"iothread"}
+hmp device_add driver=virtio-scsi-pci,id=virtioscsi1,bus=pci.3,addr=0x2,iothread=iothread-virtioscsi1
+qmp object-del {"id":"throttle-drive-scsi1"}
+qmp object-add {"id":"throttle-drive-scsi1","limits":{},"qom-type":"throttle-group"}
+qmp blockdev-add {"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/8006/vm-8006-disk-1.qcow2","node-name":"e70a10e5fb0473d3fc55686f720db61","read-only":false},"node-name":"f70a10e5fb0473d3fc55686f720db61","read-only":false},"node-name":"drive-scsi1","read-only":false,"throttle-group":"throttle-drive-scsi1"}
+hmp device_add driver=scsi-hd,bus=virtioscsi1.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi1,id=scsi1,device_id=drive-scsi1,write-cache=on
+hmp device_del scsi1
+qmp blockdev-del {"node-name":"drive-scsi1"}
+qmp object-del {"id":"throttle-drive-scsi1"}
+hmp device_del virtioscsi1
+qmp object-del {"id":"iothread-virtioscsi1"}
diff --git a/src/test/hotplug/disk-add-scsi-single-fail.conf b/src/test/hotplug/disk-add-scsi-single-fail.conf
new file mode 100644
index 00000000..d9a9a30f
--- /dev/null
+++ b/src/test/hotplug/disk-add-scsi-single-fail.conf
@@ -0,0 +1,19 @@
+# TEST: a virtio-scsi-single controller that does not show up after hotplug has its iothread removed again
+# FAIL_DEVICE_ADD: virtioscsi1
+# EXPECTED_ERROR: scsi1: hotplug problem - error on hotplug device 'virtioscsi1'
+# STAYS_PENDING: 1
+bootdisk: scsi0
+cores: 2
+machine: pc-i440fx-10.1
+memory: 2048
+name: hotplug
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+ostype: l26
+scsi0: local:8006/vm-8006-disk-0.qcow2,iothread=1,size=32G
+scsihw: virtio-scsi-single
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
+
+[PENDING]
+scsi1: local:8006/vm-8006-disk-1.qcow2,iothread=1,size=32G
diff --git a/src/test/hotplug/disk-add-scsi-single-fail.conf.expected b/src/test/hotplug/disk-add-scsi-single-fail.conf.expected
new file mode 100644
index 00000000..fc291ec6
--- /dev/null
+++ b/src/test/hotplug/disk-add-scsi-single-fail.conf.expected
@@ -0,0 +1,5 @@
+storage activate_volumes ["local:8006/vm-8006-disk-1.qcow2"]
+qmp object-add {"id":"iothread-virtioscsi1","qom-type":"iothread"}
+hmp device_add driver=virtio-scsi-pci,id=virtioscsi1,bus=pci.3,addr=0x2,iothread=iothread-virtioscsi1
+hmp device_del virtioscsi1
+qmp object-del {"id":"iothread-virtioscsi1"}
-- 
2.47.3

applied: (subset) [PATCH qemu-server v5 0/4] hotplug: improve and fix cleanup/error handling

[Fiona Ebner]

On Fri, 30 Oct 2026 11:04:39 +0100, Dominik Csapak wrote:
> This replaces my earlier v4 9/9 patch[0]
> 
> It uses a different approach now by unifying the cleanup and removal of
> disks and their controllers.
> 
> It also fixes a related bug in 1/4 that was uncovered.
> 
> [...]

Applied the first two patches, thanks! In the first one I switched to
using parse_drive_interface().

[1/4] hotplug: scsi: only keep controller if a disk on it is still present
      commit: 346f8e55c6b8644a53001c2b747b6fa8460d41ec
[2/4] hotplug: remove iothread with virtio-scsi controller together
      commit: 183266bedc12bae384d2306eff7af652be220725

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Fiona Ebner]

Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
> Unplugging a disk or SCSI controller removes the same parts that need to
> be removed when hotplugging one of them failed. Factor this out into a
> helper, so it can be reused for cleaning up after a failed hotplug in a
> following commit.
> 
> For that case, the helper can warn instead of die on errors, so that
> the remaining parts are still removed and the original error can be
> reported.
> 
> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
> the config again, since a disk that failed to hotplug is still pending
> and not in the config yet.
> 
> No functional change intended.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 38a4e4f8..3811bff2 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>      return $devices;
>  }
>  
> +# clean up disks and their controllers (if unused afterwards)
> +sub cleanup_disk {

The name is too generic, at least here in the QemuServer module.

Ideally, such a helper should rather go into QMPHelpers.pm or
DriveDevice.pm. Will require moving a few of the other helpers too, but
I think it's a good chance to do so. The iothread helpers should go into
QMPHelpers.pm as they are not exclusive to drives, just our current
usage of them. One issue is with the vm_devices_list() helper, which
calls into Blockdev::qdev_id_to_drive_id(), but I guess
qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
should avoid a cyclic dependency.

With that we should save >100 lines of QemuServer.pm again :)

> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
> +
> +    my $warn_or_die = sub {
> +        my ($sub) = @_;
> +        eval { $sub->(); };
> +        if (my $err = $@) {
> +            die $err if !$noerr;
> +            warn $err;
> +        }
> +    };

Rather than having this with $noerr, just make the callers use our
common eval + warn pattern. That would improve readability below and on
the call sites (reading a "1" there won't tell you how it relates to
error behavior without looking it up).

> +
> +    $warn_or_die->(sub {
> +        qemu_devicedel($vmid, $deviceid);
> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
> +    });
> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if $deviceid =~ m/^(virtio|scsi)\d+$/;
> +    # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid, $device); }) if $deviceid =~ m/^scsi\d+$/;
> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
> +
> +    return;
> +}
> +
>  sub vm_deviceplug {
>      my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
>  
> @@ -3972,20 +3998,12 @@ sub vm_deviceunplug {
>          qemu_devicedel($vmid, $deviceid);
>          qemu_devicedelverify($vmid, $deviceid);
>      } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
> -        qemu_devicedel($vmid, $deviceid);
> -        qemu_devicedelverify($vmid, $deviceid);
> -        qemu_drivedel($vmid, $deviceid);
> -        qemu_iothread_del($vmid, $deviceid);
> +        cleanup_disk($conf, $vmid, $deviceid);
>      } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
> -        qemu_devicedel($vmid, $deviceid);
> -        qemu_devicedelverify($vmid, $deviceid, 15);
> -        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
> +        cleanup_disk($conf, $vmid, $deviceid, undef, 15);
>      } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
> -        qemu_devicedel($vmid, $deviceid);
> -        qemu_devicedelverify($vmid, $deviceid);
> -        qemu_drivedel($vmid, $deviceid);
> -        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
> -        qemu_deletescsihw($conf, $vmid, $deviceid);
> +        my $device = parse_drive($deviceid, $conf->{$deviceid});
> +        cleanup_disk($conf, $vmid, $deviceid, $device);
>      } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>          qemu_devicedel($vmid, $deviceid);
>          qemu_devicedelverify($vmid, $deviceid);
> @@ -4124,9 +4142,7 @@ sub qemu_findorcreatescsihw {
>  }
>  
>  sub qemu_deletescsihw {
> -    my ($conf, $vmid, $opt) = @_;
> -
> -    my $device = parse_drive($opt, $conf->{$opt});
> +    my ($conf, $vmid, $device) = @_;
>  
>      if ($conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single')) {
>          vm_deviceunplug($vmid, $conf, "virtioscsi$device->{index}");

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Dominik Csapak]

On 11/3/26 12:06 PM, Fiona Ebner wrote:
> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>> Unplugging a disk or SCSI controller removes the same parts that need to
>> be removed when hotplugging one of them failed. Factor this out into a
>> helper, so it can be reused for cleaning up after a failed hotplug in a
>> following commit.
>>
>> For that case, the helper can warn instead of die on errors, so that
>> the remaining parts are still removed and the original error can be
>> reported.
>>
>> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
>> the config again, since a disk that failed to hotplug is still pending
>> and not in the config yet.
>>
>> No functional change intended.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>> index 38a4e4f8..3811bff2 100644
>> --- a/src/PVE/QemuServer.pm
>> +++ b/src/PVE/QemuServer.pm
>> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>>       return $devices;
>>   }
>>   
>> +# clean up disks and their controllers (if unused afterwards)
>> +sub cleanup_disk {
> 
> The name is too generic, at least here in the QemuServer module.

sure, any suggestions? `cleanup_disk_devices` ?

> 
> Ideally, such a helper should rather go into QMPHelpers.pm or
> DriveDevice.pm. Will require moving a few of the other helpers too, but
> I think it's a good chance to do so. The iothread helpers should go into
> QMPHelpers.pm as they are not exclusive to drives, just our current
> usage of them. One issue is with the vm_devices_list() helper, which
> calls into Blockdev::qdev_id_to_drive_id(), but I guess
> qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
> should avoid a cyclic dependency.
> 
> With that we should save >100 lines of QemuServer.pm again :)

sure I'll take a stab at it

> 
>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>> +
>> +    my $warn_or_die = sub {
>> +        my ($sub) = @_;
>> +        eval { $sub->(); };
>> +        if (my $err = $@) {
>> +            die $err if !$noerr;
>> +            warn $err;
>> +        }
>> +    };
> 
> Rather than having this with $noerr, just make the callers use our
> common eval + warn pattern. That would improve readability below and on
> the call sites (reading a "1" there won't tell you how it relates to
> error behavior without looking it up).
> 

If i understand you correctly this would change the semantic though?

now with noerr = 1, every cleanup here runs regardless if it fails or not.

if i just check on the call site, the first error skips the cleanup
of the rest?

>> +
>> +    $warn_or_die->(sub {
>> +        qemu_devicedel($vmid, $deviceid);
>> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
>> +    });
>> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if $deviceid =~ m/^(virtio|scsi)\d+$/;
>> +    # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
>> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid, $device); }) if $deviceid =~ m/^scsi\d+$/;
>> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
>> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
>> +
>> +    return;
>> +}
>> +
>>   sub vm_deviceplug {
>>       my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
>>   
>> @@ -3972,20 +3998,12 @@ sub vm_deviceunplug {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        qemu_iothread_del($vmid, $deviceid);
>> +        cleanup_disk($conf, $vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid, 15);
>> -        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
>> +        cleanup_disk($conf, $vmid, $deviceid, undef, 15);
>>       } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
>> -        qemu_deletescsihw($conf, $vmid, $deviceid);
>> +        my $device = parse_drive($deviceid, $conf->{$deviceid});
>> +        cleanup_disk($conf, $vmid, $deviceid, $device);
>>       } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>> @@ -4124,9 +4142,7 @@ sub qemu_findorcreatescsihw {
>>   }
>>   
>>   sub qemu_deletescsihw {
>> -    my ($conf, $vmid, $opt) = @_;
>> -
>> -    my $device = parse_drive($opt, $conf->{$opt});
>> +    my ($conf, $vmid, $device) = @_;
>>   
>>       if ($conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single')) {
>>           vm_deviceunplug($vmid, $conf, "virtioscsi$device->{index}");
> 

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Dominik Csapak]

On 11/3/26 12:06 PM, Fiona Ebner wrote:
> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>> Unplugging a disk or SCSI controller removes the same parts that need to
>> be removed when hotplugging one of them failed. Factor this out into a
>> helper, so it can be reused for cleaning up after a failed hotplug in a
>> following commit.
>>
>> For that case, the helper can warn instead of die on errors, so that
>> the remaining parts are still removed and the original error can be
>> reported.
>>
>> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
>> the config again, since a disk that failed to hotplug is still pending
>> and not in the config yet.
>>
>> No functional change intended.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>> index 38a4e4f8..3811bff2 100644
>> --- a/src/PVE/QemuServer.pm
>> +++ b/src/PVE/QemuServer.pm
>> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>>       return $devices;
>>   }
>>   
>> +# clean up disks and their controllers (if unused afterwards)
>> +sub cleanup_disk {
> 
> The name is too generic, at least here in the QemuServer module.
> 
> Ideally, such a helper should rather go into QMPHelpers.pm or
> DriveDevice.pm. Will require moving a few of the other helpers too, but
> I think it's a good chance to do so. The iothread helpers should go into
> QMPHelpers.pm as they are not exclusive to drives, just our current
> usage of them. One issue is with the vm_devices_list() helper, which
> calls into Blockdev::qdev_id_to_drive_id(), but I guess
> qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
> should avoid a cyclic dependency.
> 

thinking about this, we'd either have to break the recursion
of cleanup_disk -> qemu_deletescsihw -> vm_deviceunplug -> cleanup_disk

somehow, or move vm_deviceunplug also to drivedevice/qmphelpers (which
seems wrong either way?)

do you have any thoughts on that?

> With that we should save >100 lines of QemuServer.pm again :)
> 
>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>> +
>> +    my $warn_or_die = sub {
>> +        my ($sub) = @_;
>> +        eval { $sub->(); };
>> +        if (my $err = $@) {
>> +            die $err if !$noerr;
>> +            warn $err;
>> +        }
>> +    };
> 
> Rather than having this with $noerr, just make the callers use our
> common eval + warn pattern. That would improve readability below and on
> the call sites (reading a "1" there won't tell you how it relates to
> error behavior without looking it up).
> 
>> +
>> +    $warn_or_die->(sub {
>> +        qemu_devicedel($vmid, $deviceid);
>> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
>> +    });
>> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if $deviceid =~ m/^(virtio|scsi)\d+$/;
>> +    # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
>> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid, $device); }) if $deviceid =~ m/^scsi\d+$/;
>> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
>> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
>> +
>> +    return;
>> +}
>> +
>>   sub vm_deviceplug {
>>       my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
>>   
>> @@ -3972,20 +3998,12 @@ sub vm_deviceunplug {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        qemu_iothread_del($vmid, $deviceid);
>> +        cleanup_disk($conf, $vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid, 15);
>> -        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
>> +        cleanup_disk($conf, $vmid, $deviceid, undef, 15);
>>       } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
>> -        qemu_deletescsihw($conf, $vmid, $deviceid);
>> +        my $device = parse_drive($deviceid, $conf->{$deviceid});
>> +        cleanup_disk($conf, $vmid, $deviceid, $device);
>>       } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>> @@ -4124,9 +4142,7 @@ sub qemu_findorcreatescsihw {
>>   }
>>   
>>   sub qemu_deletescsihw {
>> -    my ($conf, $vmid, $opt) = @_;
>> -
>> -    my $device = parse_drive($opt, $conf->{$opt});
>> +    my ($conf, $vmid, $device) = @_;
>>   
>>       if ($conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single')) {
>>           vm_deviceunplug($vmid, $conf, "virtioscsi$device->{index}");
> 

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Fiona Ebner]

Am 03.11.26 um 2:01 PM schrieb Dominik Csapak:
> On 11/3/26 12:06 PM, Fiona Ebner wrote:
>> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>>> Unplugging a disk or SCSI controller removes the same parts that need to
>>> be removed when hotplugging one of them failed. Factor this out into a
>>> helper, so it can be reused for cleaning up after a failed hotplug in a
>>> following commit.
>>>
>>> For that case, the helper can warn instead of die on errors, so that
>>> the remaining parts are still removed and the original error can be
>>> reported.
>>>
>>> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
>>> the config again, since a disk that failed to hotplug is still pending
>>> and not in the config yet.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>>> index 38a4e4f8..3811bff2 100644
>>> --- a/src/PVE/QemuServer.pm
>>> +++ b/src/PVE/QemuServer.pm
>>> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>>>       return $devices;
>>>   }
>>>   +# clean up disks and their controllers (if unused afterwards)
>>> +sub cleanup_disk {
>>
>> The name is too generic, at least here in the QemuServer module.
> 
> sure, any suggestions? `cleanup_disk_devices` ?

Yes, that's better. Or similar 'delete_disk_hardware'?

>>
>> Ideally, such a helper should rather go into QMPHelpers.pm or
>> DriveDevice.pm. Will require moving a few of the other helpers too, but
>> I think it's a good chance to do so. The iothread helpers should go into
>> QMPHelpers.pm as they are not exclusive to drives, just our current
>> usage of them. One issue is with the vm_devices_list() helper, which
>> calls into Blockdev::qdev_id_to_drive_id(), but I guess
>> qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
>> should avoid a cyclic dependency.
>>
>> With that we should save >100 lines of QemuServer.pm again :)
> 
> sure I'll take a stab at it
> 

Thanks! Ah, and I notice there also is qemu_drivedel() which requires
Blockdev::detach(), so that one should go into Blockdev.pm and then your
new helper must then not go into QMPHelpers because that should not
depend Blockdev, but DriveDevice may depend on Blockdev.

> thinking about this, we'd either have to break the recursion
> of cleanup_disk -> qemu_deletescsihw -> vm_deviceunplug -> cleanup_disk
> 
> somehow, or move vm_deviceunplug also to drivedevice/qmphelpers (which
> seems wrong either way?)
> 
> do you have any thoughts on that? 

Of course things are more complicated than expected ^^
So maybe it's not worth going for right now. I think breaking the
recursion might be preferable, with first constructing the list of
things we want to unplug in which order and then attempting to do so.
But that seems very unrelated to your original series now. So keeping
your helper in QemuServer would also be fine by me.

>>
>>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>>> +
>>> +    my $warn_or_die = sub {
>>> +        my ($sub) = @_;
>>> +        eval { $sub->(); };
>>> +        if (my $err = $@) {
>>> +            die $err if !$noerr;
>>> +            warn $err;
>>> +        }
>>> +    };
>>
>> Rather than having this with $noerr, just make the callers use our
>> common eval + warn pattern. That would improve readability below and on
>> the call sites (reading a "1" there won't tell you how it relates to
>> error behavior without looking it up).
>>
> 
> If i understand you correctly this would change the semantic though?
> 
> now with noerr = 1, every cleanup here runs regardless if it fails or not.
> 
> if i just check on the call site, the first error skips the cleanup
> of the rest?

Right, but isn't that actually what we want? The objects depend on each
other, so we should only delete them in order. We do/should not fail if
an object is already deleted. Otherwise your next patch would introduce
a bunch of additional warnings in most error cases when attempting to
remove stuff that was never added.

> 
>>> +
>>> +    $warn_or_die->(sub {
>>> +        qemu_devicedel($vmid, $deviceid);
>>> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
>>> +    });
>>> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if
>>> $deviceid =~ m/^(virtio|scsi)\d+$/;
>>> +    # the iothread of a virtio-scsi-single disk belongs to its
>>> controller and is removed there
>>> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid,
>>> $device); }) if $deviceid =~ m/^scsi\d+$/;
>>> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
>>> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
>>> +
>>> +    return;
>>> +}
>>> +

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Dominik Csapak]

On 11/3/26 3:41 PM, Fiona Ebner wrote:
> Am 03.11.26 um 2:01 PM schrieb Dominik Csapak:
>> On 11/3/26 12:06 PM, Fiona Ebner wrote:
>>> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>>>> Unplugging a disk or SCSI controller removes the same parts that need to
>>>> be removed when hotplugging one of them failed. Factor this out into a
>>>> helper, so it can be reused for cleaning up after a failed hotplug in a
>>>> following commit.
>>>>
>>>> For that case, the helper can warn instead of die on errors, so that
>>>> the remaining parts are still removed and the original error can be
>>>> reported.
>>>>
>>>> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
>>>> the config again, since a disk that failed to hotplug is still pending
>>>> and not in the config yet.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>>    src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 31 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>>>> index 38a4e4f8..3811bff2 100644
>>>> --- a/src/PVE/QemuServer.pm
>>>> +++ b/src/PVE/QemuServer.pm
>>>> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>>>>        return $devices;
>>>>    }
>>>>    +# clean up disks and their controllers (if unused afterwards)
>>>> +sub cleanup_disk {
>>>
>>> The name is too generic, at least here in the QemuServer module.
>>
>> sure, any suggestions? `cleanup_disk_devices` ?
> 
> Yes, that's better. Or similar 'delete_disk_hardware'?

sounds good to me

> 
>>>
>>> Ideally, such a helper should rather go into QMPHelpers.pm or
>>> DriveDevice.pm. Will require moving a few of the other helpers too, but
>>> I think it's a good chance to do so. The iothread helpers should go into
>>> QMPHelpers.pm as they are not exclusive to drives, just our current
>>> usage of them. One issue is with the vm_devices_list() helper, which
>>> calls into Blockdev::qdev_id_to_drive_id(), but I guess
>>> qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
>>> should avoid a cyclic dependency.
>>>
>>> With that we should save >100 lines of QemuServer.pm again :)
>>
>> sure I'll take a stab at it
>>
> 
> Thanks! Ah, and I notice there also is qemu_drivedel() which requires
> Blockdev::detach(), so that one should go into Blockdev.pm and then your
> new helper must then not go into QMPHelpers because that should not
> depend Blockdev, but DriveDevice may depend on Blockdev.
> 
>> thinking about this, we'd either have to break the recursion
>> of cleanup_disk -> qemu_deletescsihw -> vm_deviceunplug -> cleanup_disk
>>
>> somehow, or move vm_deviceunplug also to drivedevice/qmphelpers (which
>> seems wrong either way?)
>>
>> do you have any thoughts on that?
> 
> Of course things are more complicated than expected ^^
> So maybe it's not worth going for right now. I think breaking the
> recursion might be preferable, with first constructing the list of
> things we want to unplug in which order and then attempting to do so.
> But that seems very unrelated to your original series now. So keeping
> your helper in QemuServer would also be fine by me.

alright

> 
>>>
>>>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>>>> +
>>>> +    my $warn_or_die = sub {
>>>> +        my ($sub) = @_;
>>>> +        eval { $sub->(); };
>>>> +        if (my $err = $@) {
>>>> +            die $err if !$noerr;
>>>> +            warn $err;
>>>> +        }
>>>> +    };
>>>
>>> Rather than having this with $noerr, just make the callers use our
>>> common eval + warn pattern. That would improve readability below and on
>>> the call sites (reading a "1" there won't tell you how it relates to
>>> error behavior without looking it up).
>>>
>>
>> If i understand you correctly this would change the semantic though?
>>
>> now with noerr = 1, every cleanup here runs regardless if it fails or not.
>>
>> if i just check on the call site, the first error skips the cleanup
>> of the rest?
> 
> Right, but isn't that actually what we want? The objects depend on each
> other, so we should only delete them in order. We do/should not fail if
> an object is already deleted. Otherwise your next patch would introduce
> a bunch of additional warnings in most error cases when attempting to
> remove stuff that was never added.

well it might happen that devicedelverify is failing even though
it's actually removed if i understand 
https://bugzilla.proxmox.com/show_bug.cgi?id=8273 correctly?

so we might want to continue anyway? they shouldn't be removable
if something depends on it, but trying in any case works around
some timeouts i think...

> 
>>
>>>> +
>>>> +    $warn_or_die->(sub {
>>>> +        qemu_devicedel($vmid, $deviceid);
>>>> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
>>>> +    });
>>>> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if
>>>> $deviceid =~ m/^(virtio|scsi)\d+$/;
>>>> +    # the iothread of a virtio-scsi-single disk belongs to its
>>>> controller and is removed there
>>>> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid,
>>>> $device); }) if $deviceid =~ m/^scsi\d+$/;
>>>> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
>>>> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
>>>> +
>>>> +    return;
>>>> +}
>>>> +

Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

[Fiona Ebner]

Am 03.11.26 um 4:11 PM schrieb Dominik Csapak:
> On 11/3/26 3:41 PM, Fiona Ebner wrote:
>> Am 03.11.26 um 2:01 PM schrieb Dominik Csapak:
>>> On 11/3/26 12:06 PM, Fiona Ebner wrote:
>>>> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>>>>
>>>>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>>>>> +
>>>>> +    my $warn_or_die = sub {
>>>>> +        my ($sub) = @_;
>>>>> +        eval { $sub->(); };
>>>>> +        if (my $err = $@) {
>>>>> +            die $err if !$noerr;
>>>>> +            warn $err;
>>>>> +        }
>>>>> +    };
>>>>
>>>> Rather than having this with $noerr, just make the callers use our
>>>> common eval + warn pattern. That would improve readability below and on
>>>> the call sites (reading a "1" there won't tell you how it relates to
>>>> error behavior without looking it up).
>>>>
>>>
>>> If i understand you correctly this would change the semantic though?
>>>
>>> now with noerr = 1, every cleanup here runs regardless if it fails or
>>> not.
>>>
>>> if i just check on the call site, the first error skips the cleanup
>>> of the rest?
>>
>> Right, but isn't that actually what we want? The objects depend on each
>> other, so we should only delete them in order. We do/should not fail if
>> an object is already deleted. Otherwise your next patch would introduce
>> a bunch of additional warnings in most error cases when attempting to
>> remove stuff that was never added.
> 
> well it might happen that devicedelverify is failing even though
> it's actually removed if i understand https://bugzilla.proxmox.com/
> show_bug.cgi?id=8273 correctly?
> 
> so we might want to continue anyway? they shouldn't be removable
> if something depends on it, but trying in any case works around
> some timeouts i think...
If devicedelverify() reports that it failed, we should assume so. It
would just be luck if the subsequent command works because of timing
reasons.

We should make sure a subsequent cleanup removes the left-overs, but
that is independent.

I would rather bump the timeout for the relevant 'query-pci' command
(not too much if not inside a worker context) to avoid the failure in
the first place.

superseded: [PATCH qemu-server v5 0/4] hotplug: improve and fix cleanup/error handling

[Dominik Csapak]

superseded by v6:
https://lore.proxmox.com/pve-devel/20261104065043.286417-1-d.csapak@proxmox.com/T/#t