[pve-devel] [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[pve-devel] [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Fiona Ebner]

Volume names are allowed to contain underscores, so it is impossible
to determine the snapshot name from just the volume name, e.g:
snap_vm-100-disk_with_underscore_here_s_some_more.qcow2

Therefore, we need to also rely on the volume name itself to properly
parse the snapshot name.

Fiona Ebner (3):
  lvm plugin: snapshot info: avoid superfluous argument for closure
  fix #6587: lvm plugin: snapshot info: fix parsing snapshot name
  lvm plugin: volume snapshot: actually print error when renaming

 src/PVE/Storage/LVMPlugin.pm | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.47.2



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

[pve-devel] [PATCH storage 1/3] lvm plugin: snapshot info: avoid superfluous argument for closure

[Fiona Ebner]

The $volname variable is never modified in the function, so it doesn't
need to be passed into the $get_snapname_from_path closure.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 67fcfbd..705585f 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -800,7 +800,7 @@ sub volume_snapshot_info {
     my ($class, $scfg, $storeid, $volname) = @_;
 
     my $get_snapname_from_path = sub {
-        my ($volname, $path) = @_;
+        my ($path) = @_;
 
         my $name = basename($path);
         if (my $snapname = parse_snap_name($name)) {
@@ -829,7 +829,7 @@ sub volume_snapshot_info {
     my $snapshots = $json_decode;
     for my $snap (@$snapshots) {
         my $snapfile = $snap->{filename};
-        my $snapname = $get_snapname_from_path->($volname, $snapfile);
+        my $snapname = $get_snapname_from_path->($snapfile);
         #not a proxmox snapshot
         next if !$snapname;
 
@@ -842,7 +842,7 @@ sub volume_snapshot_info {
 
         my $parentfile = $snap->{'backing-filename'};
         if ($parentfile) {
-            my $parentname = $get_snapname_from_path->($volname, $parentfile);
+            my $parentname = $get_snapname_from_path->($parentfile);
             $info->{$snapname}->{parent} = $parentname;
             $info->{$parentname}->{child} = $snapname;
         }
-- 
2.47.2



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

[pve-devel] [PATCH storage 2/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Fiona Ebner]

Volume names are allowed to contain underscores, so it is impossible
to determine the snapshot name from just the volume name, e.g:
snap_vm-100-disk_with_underscore_here_s_some_more.qcow2

Therefore, pass along the short volume name too and match against it.

Note that none of the variables from the result of parse_volname()
were actually used previously.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 705585f..2026450 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -470,9 +470,11 @@ my sub get_snap_name {
 }
 
 my sub parse_snap_name {
-    my ($name) = @_;
+    my ($name, $short_volname) = @_;
 
-    if ($name =~ m/^snap_\S+_(.*)\.qcow2$/) {
+    $short_volname =~ s/\.(qcow2)$//;
+
+    if ($name =~ m/^snap_\Q$short_volname\E_(.*)\.qcow2$/) {
         return $1;
     }
 }
@@ -799,11 +801,13 @@ sub status {
 sub volume_snapshot_info {
     my ($class, $scfg, $storeid, $volname) = @_;
 
+    my $short_volname = ($class->parse_volname($volname))[1];
+
     my $get_snapname_from_path = sub {
         my ($path) = @_;
 
         my $name = basename($path);
-        if (my $snapname = parse_snap_name($name)) {
+        if (my $snapname = parse_snap_name($name, $short_volname)) {
             return $snapname;
         } elsif ($name eq $volname) {
             return 'current';
@@ -812,8 +816,6 @@ sub volume_snapshot_info {
     };
 
     my $path = $class->filesystem_path($scfg, $volname);
-    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
-        $class->parse_volname($volname);
 
     my $json = PVE::Storage::Common::qemu_img_info($path, undef, 10, 1);
     die "failed to query file information with qemu-img\n" if !$json;
-- 
2.47.2



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

[pve-devel] [PATCH storage 3/3] lvm plugin: volume snapshot: actually print error when renaming

[Fiona Ebner]

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 2026450..e3fe9ff 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -991,7 +991,7 @@ sub volume_snapshot {
 
     #rename current volume to snap volume
     eval { $class->rename_snapshot($scfg, $storeid, $volname, 'current', $snap) };
-    die "error rename $volname to $snap\n" if $@;
+    die "error rename $volname to $snap - $@\n" if $@;
 
     eval { alloc_snap_image($class, $storeid, $scfg, $volname, $snap) };
     if ($@) {
-- 
2.47.2



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

Re: [pve-devel] [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Max R. Carrara]

On Wed Jul 30, 2025 at 6:20 PM CEST, Fiona Ebner wrote:
> Volume names are allowed to contain underscores, so it is impossible
> to determine the snapshot name from just the volume name, e.g:
> snap_vm-100-disk_with_underscore_here_s_some_more.qcow2
>
> Therefore, we need to also rely on the volume name itself to properly
> parse the snapshot name.

Did some testing and tried to break things.

Works as advertised and even fixes previously broken snapshots--that is,
if one tried to delete a snapshot on LVM before this patch, the snapshot
would become stuck in the "deleted" state. Once this patch was applied,
I was able to delete the stuck snapshot as well (after `qm unlock`,
naturally).

The changes also seem fine to me; gave patch 02 most of my attention
since that's where the fix is. Couldn't spot anything wrong with any of
the changes here.

So, LGTM -- consider:

Tested-by: Max R. Carrara <m.carrara@proxmox.com>
Reviewed-by: Max R. Carrara <m.carrara@proxmox.com>

>
> Fiona Ebner (3):
>   lvm plugin: snapshot info: avoid superfluous argument for closure
>   fix #6587: lvm plugin: snapshot info: fix parsing snapshot name
>   lvm plugin: volume snapshot: actually print error when renaming
>
>  src/PVE/Storage/LVMPlugin.pm | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)



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

[pve-devel] applied: [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Thomas Lamprecht]

On Wed, 30 Jul 2025 18:20:54 +0200, Fiona Ebner wrote:
> Volume names are allowed to contain underscores, so it is impossible
> to determine the snapshot name from just the volume name, e.g:
> snap_vm-100-disk_with_underscore_here_s_some_more.qcow2
> 
> Therefore, we need to also rely on the volume name itself to properly
> parse the snapshot name.
> 
> [...]

Applied, thanks!

[1/3] lvm plugin: snapshot info: avoid superfluous argument for closure
      commit: 819dafe516c1aedfef047b4ceb38dc49ebb2b850
[2/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name
      commit: db2025f5ba4140fae9830154d16ae6999384bed8
[3/3] lvm plugin: volume snapshot: actually print error when renaming
      commit: fc633887dcfae776f801a8fec26d2c58c806e1ed


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

Re: [pve-devel] applied: [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Fiona Ebner]

Am 30.07.25 um 7:36 PM schrieb Thomas Lamprecht:
> On Wed, 30 Jul 2025 18:20:54 +0200, Fiona Ebner wrote:
>> Volume names are allowed to contain underscores, so it is impossible
>> to determine the snapshot name from just the volume name, e.g:
>> snap_vm-100-disk_with_underscore_here_s_some_more.qcow2
>>
>> Therefore, we need to also rely on the volume name itself to properly
>> parse the snapshot name.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/3] lvm plugin: snapshot info: avoid superfluous argument for closure
>       commit: 819dafe516c1aedfef047b4ceb38dc49ebb2b850
> [2/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name
>       commit: db2025f5ba4140fae9830154d16ae6999384bed8
> [3/3] lvm plugin: volume snapshot: actually print error when renaming
>       commit: fc633887dcfae776f801a8fec26d2c58c806e1ed

Just wanted to thank Shannon for the bug analysis this was based on! An
Analyzed-by tag would've been in order, but I forgot to add that in the
hurry yesterday. Sorry about that!


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

Re: [pve-devel] applied: [PATCH-SERIES storage 0/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name

[Thomas Lamprecht]

Am 31.07.25 um 10:33 schrieb Fiona Ebner:
> Am 30.07.25 um 7:36 PM schrieb Thomas Lamprecht:
>> [1/3] lvm plugin: snapshot info: avoid superfluous argument for closure
>>       commit: 819dafe516c1aedfef047b4ceb38dc49ebb2b850
>> [2/3] fix #6587: lvm plugin: snapshot info: fix parsing snapshot name
>>       commit: db2025f5ba4140fae9830154d16ae6999384bed8
>> [3/3] lvm plugin: volume snapshot: actually print error when renaming
>>       commit: fc633887dcfae776f801a8fec26d2c58c806e1ed
> 
> Just wanted to thank Shannon for the bug analysis this was based on! An
> Analyzed-by tag would've been in order, but I forgot to add that in the
> hurry yesterday. Sorry about that!

FWIW, thanks to b4 and the Link tag it can add there is a chain linking
(heh) the commits to this threads and thus your shout out here, that's
one of the reasons I like having them.


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