From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 6FEEF1FF0AA for ; Tue, 03 Nov 2026 15:42:04 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 808102189D; Tue, 03 Nov 2026 15:42:02 +0100 (CET) Message-ID: <135d8d14-bee6-4cba-90da-5a9a1200f5d3@proxmox.com> Date: Tue, 3 Nov 2026 15:41:56 +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> <48a487eb-a4cc-4267-9a52-a56dd1345b78@proxmox.com> <656adfd7-f342-4017-be46-35355a040408@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <656adfd7-f342-4017-be46-35355a040408@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793716916753 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.301 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: AOWWSN346QKJZGSMMN5SUNOUR3M65QAU X-Message-ID-Hash: AOWWSN346QKJZGSMMN5SUNOUR3M65QAU 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 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 >>> --- >>>   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; >>> +} >>> +