[pve-devel] [PATCH storage 0/4] fix snapshot filename regressions

[pve-devel] [PATCH storage 0/4] fix snapshot filename regressions

[Fabian Grünbichler]

fixes for various issues

Fabian Grünbichler (4):
  plugin: fix parse_name_dir regression for custom volume names
  fix #6584: plugin: list_images: only include parseable filenames
  plugin: extend snapshot name parsing to legacy volnames
  plugin: parse_name_dir: drop deprecation warning

 src/PVE/Storage/Plugin.pm | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH storage 1/4] plugin: fix parse_name_dir regression for custom volume names

[Fabian Grünbichler]

prior to the introduction of snapshot as volume chains, volume names of
almost arbitrary form were accepted. only forbid filenames which are
part of the newly introduced namespace for snapshot files, while
deprecating other names not following our usual naming scheme, instead
of forbidding them outright.

Fixes: b63147f5dfd62e398cc206d2f7086d5db38b3c9b "plugin: fix volname parsing"

Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 2b5a016..c08f5a5 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -715,8 +715,11 @@ sub parse_name_dir {
     if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
         my $isbase = $2 eq 'base-' ? $2 : undef;
         return ($1, $4, $isbase); # (name, format, isBase)
+    } elsif (parse_snap_name($name)) {
+        die "'$name' is a snapshot filename, not a volume!\n";
     } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
-        warn "this volume name `$name` is not supported anymore\n" if !parse_snap_name($name);
+        warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
+        return ($1, $3, $2); # (name ,format, isBase)
     }
 
     die "unable to parse volume filename '$name'\n";
-- 
2.39.5



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

[pve-devel] [PATCH storage 2/4] fix #6584: plugin: list_images: only include parseable filenames

[Fabian Grünbichler]

by only including filenames that are also valid when actually parsing them,
things like snapshot files or files not following our naming scheme are no
longer candidates for rescanning or included in other output.

Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index c08f5a5..affe7b0 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1555,6 +1555,10 @@ sub list_images {
 
         next if !$vollist && defined($vmid) && ($owner ne $vmid);
 
+        # skip files that are snapshots or have invalid names
+        my ($parsed_name) = eval { parse_name_dir(basename($fn)) };
+        next if !defined($parsed_name);
+
         my ($size, undef, $used, $parent, $ctime) = eval { file_size_info($fn, undef, $format); };
         if (my $err = $@) {
             die $err if $err !~ m/Image is not in \S+ format$/;
-- 
2.39.5



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

[pve-devel] [PATCH storage 3/4] plugin: extend snapshot name parsing to legacy volnames

[Fabian Grünbichler]

otherwise a volume like `100/oldstyle-100-disk-0.qcow2` can be snapshotted, but
the snapshot file is treated as a volume instead of a snapshot afterwards.

this also avoids issues with volnames with `vm-` in their names, similar to the
LVM fix for underscores.

Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index affe7b0..db05e0e 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -702,9 +702,9 @@ sub cluster_lock_storage {
 }
 
 my sub parse_snap_name {
-    my ($name) = @_;
+    my ($filename, $volname) = @_;
 
-    if ($name =~ m/^snap-(.*)-vm(.*)$/) {
+    if ($filename =~ m/^snap-(.*)-\Q$volname\E$/) {
         return $1;
     }
 }
@@ -715,7 +715,7 @@ sub parse_name_dir {
     if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
         my $isbase = $2 eq 'base-' ? $2 : undef;
         return ($1, $4, $isbase); # (name, format, isBase)
-    } elsif (parse_snap_name($name)) {
+    } elsif ($name =~ m!^snap-.*\.qcow2$!) {
         die "'$name' is a snapshot filename, not a volume!\n";
     } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
         warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
@@ -1753,7 +1753,7 @@ sub volume_snapshot_info {
 
         my $name = basename($path);
 
-        if (my $snapname = parse_snap_name($name)) {
+        if (my $snapname = parse_snap_name($name, basename($volname))) {
             return $snapname;
         } elsif ($name eq basename($volname)) {
             return 'current';
-- 
2.39.5



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

[pve-devel] [RFC storage 4/4] plugin: parse_name_dir: drop deprecation warning

[Fabian Grünbichler]

this gets printed very often if such a volume exists - e.g. adding such a
volume to a config with `qm set` prints it 10 times..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
we could maybe just warn in the list_images code path? or add a check to
pve8to9?

 src/PVE/Storage/Plugin.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index db05e0e..2291d72 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -718,7 +718,6 @@ sub parse_name_dir {
     } elsif ($name =~ m!^snap-.*\.qcow2$!) {
         die "'$name' is a snapshot filename, not a volume!\n";
     } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
-        warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
         return ($1, $3, $2); # (name ,format, isBase)
     }
 
-- 
2.39.5



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

Re: [pve-devel] [PATCH storage 0/4] fix snapshot filename regressions

[Shannon Sterz]

On Thu Jul 31, 2025 at 1:15 PM CEST, Fabian Grünbichler wrote:
> fixes for various issues
>
> Fabian Grünbichler (4):
>   plugin: fix parse_name_dir regression for custom volume names
>   fix #6584: plugin: list_images: only include parseable filenames
>   plugin: extend snapshot name parsing to legacy volnames
>   plugin: parse_name_dir: drop deprecation warning
>
>  src/PVE/Storage/Plugin.pm | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

tested all of these by reproducing the steps outlined in 6584. plus, i
added a volume with a deprecated name to the vm and took a snapshots with
"vm-" as well as "-vm" in the name, everything worked fine for me. so
consider this:

Tested-by: Shannon Sterz <s.sterz@proxmox.com>


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

Re: [pve-devel] [PATCH storage 3/4] plugin: extend snapshot name parsing to legacy volnames

[Fiona Ebner]

Am 31.07.25 um 1:15 PM schrieb Fabian Grünbichler:
> otherwise a volume like `100/oldstyle-100-disk-0.qcow2` can be snapshotted, but
> the snapshot file is treated as a volume instead of a snapshot afterwards.
> 
> this also avoids issues with volnames with `vm-` in their names, similar to the
> LVM fix for underscores.
> 
> Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/PVE/Storage/Plugin.pm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index affe7b0..db05e0e 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -702,9 +702,9 @@ sub cluster_lock_storage {
>  }
>  
>  my sub parse_snap_name {
> -    my ($name) = @_;
> +    my ($filename, $volname) = @_;
>  
> -    if ($name =~ m/^snap-(.*)-vm(.*)$/) {
> +    if ($filename =~ m/^snap-(.*)-\Q$volname\E$/) {
>          return $1;
>      }
>  }
> @@ -715,7 +715,7 @@ sub parse_name_dir {
>      if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>          my $isbase = $2 eq 'base-' ? $2 : undef;
>          return ($1, $4, $isbase); # (name, format, isBase)
> -    } elsif (parse_snap_name($name)) {
> +    } elsif ($name =~ m!^snap-.*\.qcow2$!) {

Should we reserve this schema for all formats and not just qcow2 while
we're at it? For example, would keep open the possibility with TPM state
snapshots as separate files or something similar that might pop up in
the future.

>          die "'$name' is a snapshot filename, not a volume!\n";
>      } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>          warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
> @@ -1753,7 +1753,7 @@ sub volume_snapshot_info {
>  
>          my $name = basename($path);
>  
> -        if (my $snapname = parse_snap_name($name)) {
> +        if (my $snapname = parse_snap_name($name, basename($volname))) {
>              return $snapname;
>          } elsif ($name eq basename($volname)) {
>              return 'current';



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

Re: [pve-devel] [RFC storage 4/4] plugin: parse_name_dir: drop deprecation warning

[Fiona Ebner]

Am 31.07.25 um 1:15 PM schrieb Fabian Grünbichler:
> this gets printed very often if such a volume exists - e.g. adding such a
> volume to a config with `qm set` prints it 10 times..

Agreed with the change, that is too noisy.

> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> we could maybe just warn in the list_images code path? or add a check to
> pve8to9?

A warning pve8to9 would be good in any case. Not sure if we already want
to warn in list_images() this early if we're only gonna drop it in PVE
10? A warning during image allocation might be nice though.

>  src/PVE/Storage/Plugin.pm | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index db05e0e..2291d72 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -718,7 +718,6 @@ sub parse_name_dir {
>      } elsif ($name =~ m!^snap-.*\.qcow2$!) {
>          die "'$name' is a snapshot filename, not a volume!\n";
>      } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
> -        warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
>          return ($1, $3, $2); # (name ,format, isBase)
>      }
>  



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

[pve-devel] applied: [PATCH storage 0/4] fix snapshot filename regressions

[Thomas Lamprecht]

On Thu, 31 Jul 2025 13:15:15 +0200, Fabian Grünbichler wrote:
> fixes for various issues
> 
> Fabian Grünbichler (4):
>   plugin: fix parse_name_dir regression for custom volume names
>   fix #6584: plugin: list_images: only include parseable filenames
>   plugin: extend snapshot name parsing to legacy volnames
>   plugin: parse_name_dir: drop deprecation warning
> 
> [...]

While it might be indeed OK to broadly reserve that schema for all formats, as
Fiona mentioned, but that can be done in a dedicated patch in anycase.

Applied, thanks!

[1/4] plugin: fix parse_name_dir regression for custom volume names
      commit: a4771895759edb1f000ccc105b3c1b32b6c4f644
[2/4] fix #6584: plugin: list_images: only include parseable filenames
      commit: 59a54b3d5ff5dc205616fff2774d4e276b1f1f1e
[3/4] plugin: extend snapshot name parsing to legacy volnames
      commit: 6dbeba59da8282376f4d1a589e7787cf497f158c
[4/4] plugin: parse_name_dir: drop deprecation warning
      commit: 7513e21d741189511a20e1664f6a3d3f69b5b95b


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

Re: [pve-devel] [PATCH storage 3/4] plugin: extend snapshot name parsing to legacy volnames

[Fabian Grünbichler]

On July 31, 2025 2:09 pm, Fiona Ebner wrote:
> Am 31.07.25 um 1:15 PM schrieb Fabian Grünbichler:
>> otherwise a volume like `100/oldstyle-100-disk-0.qcow2` can be snapshotted, but
>> the snapshot file is treated as a volume instead of a snapshot afterwards.
>> 
>> this also avoids issues with volnames with `vm-` in their names, similar to the
>> LVM fix for underscores.
>> 
>> Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  src/PVE/Storage/Plugin.pm | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index affe7b0..db05e0e 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -702,9 +702,9 @@ sub cluster_lock_storage {
>>  }
>>  
>>  my sub parse_snap_name {
>> -    my ($name) = @_;
>> +    my ($filename, $volname) = @_;
>>  
>> -    if ($name =~ m/^snap-(.*)-vm(.*)$/) {
>> +    if ($filename =~ m/^snap-(.*)-\Q$volname\E$/) {
>>          return $1;
>>      }
>>  }
>> @@ -715,7 +715,7 @@ sub parse_name_dir {
>>      if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>>          my $isbase = $2 eq 'base-' ? $2 : undef;
>>          return ($1, $4, $isbase); # (name, format, isBase)
>> -    } elsif (parse_snap_name($name)) {
>> +    } elsif ($name =~ m!^snap-.*\.qcow2$!) {
> 
> Should we reserve this schema for all formats and not just qcow2 while
> we're at it? For example, would keep open the possibility with TPM state
> snapshots as separate files or something similar that might pop up in
> the future.

I mean, we don't create such volumes anyway, it's only custom ones and
for those we collide with existing ones in any case. the vsplit patch
series should provide us with clean namespaces for the new vtypes, and
then we can simply only ever allocate using those..

> 
>>          die "'$name' is a snapshot filename, not a volume!\n";
>>      } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>>          warn "this volume name `$name` is deprecated, please use (base-/vm-/subvol-)-NNN- as prefix\n";
>> @@ -1753,7 +1753,7 @@ sub volume_snapshot_info {
>>  
>>          my $name = basename($path);
>>  
>> -        if (my $snapname = parse_snap_name($name)) {
>> +        if (my $snapname = parse_snap_name($name, basename($volname))) {
>>              return $snapname;
>>          } elsif ($name eq basename($volname)) {
>>              return 'current';
> 
> 


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

Re: [pve-devel] [PATCH storage 0/4] fix snapshot filename regressions

[Fiona Ebner]

Am 31.07.25 um 1:15 PM schrieb Fabian Grünbichler:
> fixes for various issues
> 
> Fabian Grünbichler (4):
>   plugin: fix parse_name_dir regression for custom volume names
>   fix #6584: plugin: list_images: only include parseable filenames
>   plugin: extend snapshot name parsing to legacy volnames
>   plugin: parse_name_dir: drop deprecation warning
> 
>  src/PVE/Storage/Plugin.pm | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 


Already got applied before I could respond here after finishing my
testing, but for the record:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>

O:)


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

Re: [pve-devel] [PATCH storage 3/4] plugin: extend snapshot name parsing to legacy volnames

[Fiona Ebner]

Am 31.07.25 um 2:20 PM schrieb Fabian Grünbichler:
> On July 31, 2025 2:09 pm, Fiona Ebner wrote:
>> Am 31.07.25 um 1:15 PM schrieb Fabian Grünbichler:
>>> otherwise a volume like `100/oldstyle-100-disk-0.qcow2` can be snapshotted, but
>>> the snapshot file is treated as a volume instead of a snapshot afterwards.
>>>
>>> this also avoids issues with volnames with `vm-` in their names, similar to the
>>> LVM fix for underscores.
>>>
>>> Co-authored-by: Shannon Sterz <s.sterz@proxmox.com>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>>  src/PVE/Storage/Plugin.pm | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>>> index affe7b0..db05e0e 100644
>>> --- a/src/PVE/Storage/Plugin.pm
>>> +++ b/src/PVE/Storage/Plugin.pm
>>> @@ -702,9 +702,9 @@ sub cluster_lock_storage {
>>>  }
>>>  
>>>  my sub parse_snap_name {
>>> -    my ($name) = @_;
>>> +    my ($filename, $volname) = @_;
>>>  
>>> -    if ($name =~ m/^snap-(.*)-vm(.*)$/) {
>>> +    if ($filename =~ m/^snap-(.*)-\Q$volname\E$/) {
>>>          return $1;
>>>      }
>>>  }
>>> @@ -715,7 +715,7 @@ sub parse_name_dir {
>>>      if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>>>          my $isbase = $2 eq 'base-' ? $2 : undef;
>>>          return ($1, $4, $isbase); # (name, format, isBase)
>>> -    } elsif (parse_snap_name($name)) {
>>> +    } elsif ($name =~ m!^snap-.*\.qcow2$!) {
>>
>> Should we reserve this schema for all formats and not just qcow2 while
>> we're at it? For example, would keep open the possibility with TPM state
>> snapshots as separate files or something similar that might pop up in
>> the future.
> 
> I mean, we don't create such volumes anyway, it's only custom ones and
> for those we collide with existing ones in any case. the vsplit patch
> series should provide us with clean namespaces for the new vtypes, and
> then we can simply only ever allocate using those..

Yes, makes sense to not restrict further than necessary now if we
already have a path forward :)


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