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 62CF01FF0AA for ; Tue, 03 Nov 2026 16:12:04 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 964BF218BF; Tue, 03 Nov 2026 16:12:01 +0100 (CET) Message-ID: Date: Tue, 3 Nov 2026 16:11:56 +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> <656adfd7-f342-4017-be46-35355a040408@proxmox.com> <135d8d14-bee6-4cba-90da-5a9a1200f5d3@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <135d8d14-bee6-4cba-90da-5a9a1200f5d3@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793718716842 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.325 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: KXCO2H7ELNIENVDG33PBMMNLAQSR5GAN X-Message-ID-Hash: KXCO2H7ELNIENVDG33PBMMNLAQSR5GAN 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 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 >>>> --- >>>>   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; >>>> +} >>>> +