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 5E4111FF0AA for ; Tue, 03 Nov 2026 14:01:56 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 63160218AF; Tue, 03 Nov 2026 14:01:53 +0100 (CET) Message-ID: <656adfd7-f342-4017-be46-35355a040408@proxmox.com> Date: Tue, 3 Nov 2026 14:01:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20261030101134.2090350-1-d.csapak@proxmox.com> <20261030101134.2090350-4-d.csapak@proxmox.com> <48a487eb-a4cc-4267-9a52-a56dd1345b78@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <48a487eb-a4cc-4267-9a52-a56dd1345b78@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793710909100 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.330 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: MHCKBHNYOMQ3HVTK6ZPDYIR5RJV5QTUI X-Message-ID-Hash: MHCKBHNYOMQ3HVTK6ZPDYIR5RJV5QTUI X-MailFrom: d.csapak@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: 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 >> --- >> 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}"); >