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 5FF001FF0AA for ; Tue, 03 Nov 2026 16:29:42 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 69F23218F5; Tue, 03 Nov 2026 16:29:39 +0100 (CET) Message-ID: <6a23ef58-afdd-4da6-8b2e-ee3dfb4262f5@proxmox.com> Date: Tue, 3 Nov 2026 16:29:35 +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> <135d8d14-bee6-4cba-90da-5a9a1200f5d3@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793719775778 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.299 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: AWRR2CWTJNSWOH6PLIB62S3ZM5M3LTN5 X-Message-ID-Hash: AWRR2CWTJNSWOH6PLIB62S3ZM5M3LTN5 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 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.