Re: [pve-devel] [PATCH qemu-server v6 2/3] print warning for PVE::Storage::path errors instead of failing

From: Fiona Ebner <f.ebner@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>,
	"Michael Köppl" <m.koeppl@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server v6 2/3] print warning for PVE::Storage::path errors instead of failing
Date: Wed, 21 May 2025 13:02:33 +0200	[thread overview]
Message-ID: <16761db2-de14-453d-acba-5a226cd1b29c@proxmox.com> (raw)
In-Reply-To: <20250520090818.44881-7-m.koeppl@proxmox.com>

I'd prefer a commit title like "fix #3711: make removal of VM possible
if storage doesn't exit anymore" and add a brief rationale to the commit
message. The current title does not convey any information about the "why".

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> Co-authored-by: Stefan Hrdlicka

As already, mentioned, needs to be adapted.

> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>

With the comments addressed
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
>  PVE/QemuServer.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e02bf7d4..68bbf4ce 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2132,7 +2132,8 @@ sub destroy_vm {
>  	return if !$volid || $volid =~ m|^/|;
>  	return if $volids->{$volid};
>  
> -	my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
> +	my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
> +	log_warn("failed to get path and owner of volume '$volid': $@\n") if $@;

No need for the trailing "\n". In fact, it will be duplicate if $@
already contains it. Semi-related note: log_warn() will add a trailing
newline if the passed-in message doesn't contain it.

>  	return if !$path || !$owner || ($owner != $vmid);
>  
>  	$volids->{$volid} = 1;

There are two warnings further below in the function when vdisk_free()
fails, please adapt those to use log_warn() too for consistency and
better visibility for users. Should be its own patch.

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel