From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 9410D1FF0AA for ; Tue, 03 Nov 2026 12:06:32 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A8DC5218EA; Tue, 03 Nov 2026 12:06:29 +0100 (CET) Message-ID: <48a487eb-a4cc-4267-9a52-a56dd1345b78@proxmox.com> Date: Tue, 3 Nov 2026 12:06:21 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20261030101134.2090350-1-d.csapak@proxmox.com> <20261030101134.2090350-4-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261030101134.2090350-4-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793703985226 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.319 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: Q5OKLQICZV6ZTO3V2KGXYJFZU6VISD4C X-Message-ID-Hash: Q5OKLQICZV6ZTO3V2KGXYJFZU6VISD4C X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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}");