[pve-devel] [PATCH qemu-server] drive: volume in-use check: fix fallback path comparison

[pve-devel] [PATCH qemu-server] drive: volume in-use check: fix fallback path comparison

[Fabian Ebner]

When checking whether a volume is still referenced by a snapshot, the volid
itself is first checked. When the volid is different, we fall back to comparing
the path.

As the first value to be compared is a volume's path, the second value better be
a volume's path too, and not a snapshot's path.

See also 77019edfe0c190c949cdc0b0e3b4ad2ca37313b3 for historical context.

The error that led me here:
* had a VM with ZFS over iSCSI storage with an exsiting snapshot
* add new unused drive
* try to remove the unsued drive
* fails, because ZFS (not Pool!) Plugin does not support snapshot paths.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer/Drive.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 01ea8d7..9016a43 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -590,7 +590,7 @@ sub is_volume_in_use {
 		    next if !$storeid;
 		    my $scfg = PVE::Storage::storage_config($storecfg, $storeid, 1);
 		    next if !$scfg;
-		    return 1 if $path eq PVE::Storage::path($storecfg, $drive->{file}, $snapname);
+		    return 1 if $path eq PVE::Storage::path($storecfg, $drive->{file});
 		}
 	    }
 	}
-- 
2.20.1

Re: [pve-devel] [PATCH qemu-server] drive: volume in-use check: fix fallback path comparison

[Thomas Lamprecht]

On 15.04.21 12:10, Fabian Ebner wrote:
> When checking whether a volume is still referenced by a snapshot, the volid
> itself is first checked. When the volid is different, we fall back to comparing
> the path.
> 
> As the first value to be compared is a volume's path, the second value better be
> a volume's path too, and not a snapshot's path.
> 
> See also 77019edfe0c190c949cdc0b0e3b4ad2ca37313b3 for historical context.
> 
> The error that led me here:
> * had a VM with ZFS over iSCSI storage with an exsiting snapshot
> * add new unused drive
> * try to remove the unsued drive
> * fails, because ZFS (not Pool!) Plugin does not support snapshot paths.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer/Drive.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 01ea8d7..9016a43 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -590,7 +590,7 @@ sub is_volume_in_use {
>  		    next if !$storeid;
>  		    my $scfg = PVE::Storage::storage_config($storecfg, $storeid, 1);
>  		    next if !$scfg;
> -		    return 1 if $path eq PVE::Storage::path($storecfg, $drive->{file}, $snapname);
> +		    return 1 if $path eq PVE::Storage::path($storecfg, $drive->{file});

makes the whole $snapname param of that closure unused, which seems OK as $cref
holds the actual info, FWICT after a quick look.

So, if that patch is sound it should probably remove the $snapname from $scan_config
closure too.

>  		}
>  	    }
>  	}
> 

[pve-devel] applied: [PATCH qemu-server] drive: volume in-use check: fix fallback path comparison

[Thomas Lamprecht]

On 15.04.21 12:10, Fabian Ebner wrote:
> When checking whether a volume is still referenced by a snapshot, the void
> itself is first checked. When the volid is different, we fall back to comparing
> the path.
> 
> As the first value to be compared is a volume's path, the second value better be
> a volume's path too, and not a snapshot's path.
> 
> See also 77019edfe0c190c949cdc0b0e3b4ad2ca37313b3 for historical context.
> 
> The error that led me here:
> * had a VM with ZFS over iSCSI storage with an exsiting snapshot
> * add new unused drive
> * try to remove the unsued drive
> * fails, because ZFS (not Pool!) Plugin does not support snapshot paths.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer/Drive.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks! But see my other mail to this patch for a potential follow-up