[pve-devel] [PATCH pve-storage 1/1] fix #6587: parse snapshot names beginning from the first underscore

[pve-devel] [PATCH pve-storage 1/1] fix #6587: parse snapshot names beginning from the first underscore

[Shannon Sterz]

Since regular expressions are typically greedy the old regex here
matched as many underscores as it could (`\S` matches anything that is
not a whitespace, so also `_`). This lead to an issue where snapshots
that contained an underscore in their name would be truncated to only
contain the part of the name after the last underscore.

Use `[^\s_]` to match everything that is not a whitespace and not the
underscore character.

Signed-off-by: Shannon Sterz <s.sterz@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 67fcfbd..c6586ac 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -472,7 +472,7 @@ my sub get_snap_name {
 my sub parse_snap_name {
     my ($name) = @_;
 
-    if ($name =~ m/^snap_\S+_(.*)\.qcow2$/) {
+    if ($name =~ m/^snap_[^\s_]+_(.*)\.qcow2$/) {
         return $1;
     }
 }
-- 
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 qemu-server 1/1] qemu-server: exit delete early if we cannot find a snapshot

[Shannon Sterz]

Die if the list of snapshots returned by `volume_snapshot_info` does
not contain the snapshot we are trying to delete.

Previously it was just assumed that the snapshot would be present,
leading to the entry in the list being autoviviefied by the following
lines of code. Hence, we tried to commit nonexistent snapshot states
here, as both `$parentsnap` and `$childsnap` would be undefined.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
consider this more of a suggestion, but the autovivification here made
this bug somewhat more annoying to debug and i don't really see a reason
not to check that here (after all, what behaviour should be expected in
this case?)

 src/PVE/QemuServer.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 5a4f8120..ef8f2fc0 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4418,6 +4418,9 @@ sub qemu_volume_snapshot_delete {

         my $path = PVE::Storage::path($storecfg, $volid);
         my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+
+        die "could not find snapshot '$snap'\n" if !defined($snapshots->{$snap});
+
         my $parentsnap = $snapshots->{$snap}->{parent};
         my $childsnap = $snapshots->{$snap}->{child};
         my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
--
2.47.2



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

[pve-devel] applied: [PATCH qemu-server 1/1] qemu-server: exit delete early if we cannot find a snapshot

[Fiona Ebner]

On Wed, 30 Jul 2025 12:37:06 +0200, Shannon Sterz wrote:
> Die if the list of snapshots returned by `volume_snapshot_info` does
> not contain the snapshot we are trying to delete.
> 
> Previously it was just assumed that the snapshot would be present,
> leading to the entry in the list being autoviviefied by the following
> lines of code. Hence, we tried to commit nonexistent snapshot states
> here, as both `$parentsnap` and `$childsnap` would be undefined.
> 
> [...]

Applied, this one thanks! I also included the volid in the error
message for more context.

[1/1] qemu-server: exit delete early if we cannot find a snapshot
      commit: 6b7598a643a2137c8ce7c73bb928b6d390a20f97


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

Re: [pve-devel] [PATCH pve-storage 1/1] fix #6587: parse snapshot names beginning from the first underscore

[Fiona Ebner]

Am 30.07.25 um 12:37 PM schrieb Shannon Sterz:
> Since regular expressions are typically greedy the old regex here
> matched as many underscores as it could (`\S` matches anything that is
> not a whitespace, so also `_`). This lead to an issue where snapshots
> that contained an underscore in their name would be truncated to only
> contain the part of the name after the last underscore.
> 
> Use `[^\s_]` to match everything that is not a whitespace and not the
> underscore character.
> 

Problem is that right now, disk names may contain an underscore as well,
i.e.

pvesm alloc sharedlvm 100 vm-100-disk_with_underscore.qcow2 1M --format
qcow2

which in particular means from a volume name alone like

snap_vm-100-disk_with_underscore_here_s_some_more.qcow2

it is impossible to know how the disk is called and how the volume is.

We either need to change the naming schema for snapshot and align it
with what we do for the dir plugins or restrict the volume names for
qcow2 on LVM (i.e. disallow _ as part of the volume name).


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