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 7C7D91FF141 for ; Tue, 30 Jun 2026 16:22:13 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AFAE021424; Tue, 30 Jun 2026 16:22:12 +0200 (CEST) Message-ID: <885d0e46-94e6-4328-a913-1353ae0a88c6@proxmox.com> Date: Tue, 30 Jun 2026 16:21:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v9 01/10] fix #3711: vdisk_free: print warning if storage does not exist To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pve-devel@lists.proxmox.com References: <20260629140439.184878-1-m.koeppl@proxmox.com> <20260629140439.184878-2-m.koeppl@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260629140439.184878-2-m.koeppl@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: 1782829285546 X-SPAM-LEVEL: Spam detection results: 0 DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) 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: 7WDP34WBLR3CZH6K2F5ATYXRGCEYBPZG X-Message-ID-Hash: 7WDP34WBLR3CZH6K2F5ATYXRGCEYBPZG 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 29.06.26 um 4:05 PM schrieb Michael Köppl: > Removing a guest or one of its volumes should not fail when the volume's > backing storage does not exist in the configuration. Instead of dying, > warn and return so the removal can proceed. From PVE's point of view, > the volume is gone anyway. Pass the noerr flag to storage_config() so > this case is actually reached. > > Signed-off-by: Michael Köppl > --- > src/PVE/Storage.pm | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 64ea9dad..55462ff6 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -1187,7 +1187,12 @@ sub vdisk_free { > my ($cfg, $volid) = @_; > > my ($storeid, $volname) = parse_volume_id($volid); > - my $scfg = storage_config($cfg, $storeid); > + my $scfg = storage_config($cfg, $storeid, 1); > + if (!$scfg) { > + log_warn("storage '$storeid' does not exist, volume '$volid' could not be freed"); > + return; > + } > + > my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); > > activate_storage($cfg, $storeid); Not sure about this one. Note that vdisk_free() is also used for the DELETE API, so the volume ID can also be user input and not something generated by our stack. While the API call still returns an error code for an invalid volume ID, that's more by chance, because of an earlier call to path(). Failing does seem more natural and callers can decide what they want to do. This change is not required for the rest of the series to work or is it?