[PATCH-SERIES qemu-server 0/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling

[PATCH-SERIES qemu-server 0/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling

[Fiona Ebner]

When removing a volume chain snapshot, the blockdev properties are
re-generated in blockdev_{commit,replace,stream}() to operate upon.
This must happen with the current drive information, not the one from
the time the snapshot was taken.

A similar bug was present before volume chain snapshots existed, and
fixed by commit c6058683 ("fix #2258: select correct device when
removing drive snapshot via QEMU"). This is where the handling for
$attached_deviceid comes from. With snapshot-as-volume-chain, this
needs to be extended to the full drive information, not just the
device ID.

Includes some extra preparation which allows moving the functions to a
separate module and getting rid of two calls from QemuConfig back to
QemuServer making progress towards getting rid of that cyclic
dependency.

Also does some further unrelated cleanup in the VolumeChain module,
that was noticed while looking over the code.

qemu-server:

Fiona Ebner (7):
  volume snapshot (delete): avoid using deprecated check_running()
    helper
  volume snapshot delete: move getting attached device ID to caller
  fix #8071: volume snapshot delete: use current drive information to
    fix SAVC handling
  snapshot: add module for snapshot-related functionality
  snapshot: use v5.36 and subroutine signatures
  volume chain: blockdev delete: remove superfluous parse_volume_id()
    call
  volume chain: blockdev delete: pass volume ID instead of drive and
    mark private

 src/PVE/QemuConfig.pm             |  27 ++++-
 src/PVE/QemuServer.pm             | 163 +-----------------------------
 src/PVE/QemuServer/Makefile       |   1 +
 src/PVE/QemuServer/Snapshot.pm    | 155 ++++++++++++++++++++++++++++
 src/PVE/QemuServer/VolumeChain.pm |  18 ++--
 src/test/snapshot-test.pm         |   7 +-
 6 files changed, 194 insertions(+), 177 deletions(-)
 create mode 100644 src/PVE/QemuServer/Snapshot.pm


Summary over all repositories:
  6 files changed, 194 insertions(+), 177 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server 1/7] volume snapshot (delete): avoid using deprecated check_running() helper

[Fiona Ebner]

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index f199371d..036dc4c1 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4336,7 +4336,7 @@ sub qemu_volume_snapshot {
     my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
 
     my $volid = $drive->{file};
-    my $running = check_running($vmid);
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
 
     my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $deviceid, $running);
 
@@ -4369,7 +4369,7 @@ sub qemu_volume_snapshot_delete {
     my ($vmid, $storecfg, $drive, $snap) = @_;
 
     my $volid = $drive->{file};
-    my $running = check_running($vmid);
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
     my $attached_deviceid;
 
     if ($running) {
-- 
2.47.3

[PATCH qemu-server 2/7] volume snapshot delete: move getting attached device ID to caller

[Fiona Ebner]

In preparation to move the volume snapshot helpers to a separate
module that will not depend on QemuConfig.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuConfig.pm | 18 +++++++++++++++++-
 src/PVE/QemuServer.pm | 15 +--------------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 2ef6c075..3dd69ce8 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -438,7 +438,23 @@ sub __snapshot_delete_vol_snapshot {
     my $storecfg = PVE::Storage::config();
     my $volid = $drive->{file};
 
-    PVE::QemuServer::qemu_volume_snapshot_delete($vmid, $storecfg, $drive, $snapname);
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
+    my $attached_deviceid;
+
+    if ($running) {
+        my $conf = $class->load_config($vmid);
+        $class->foreach_volume(
+            $conf,
+            sub {
+                my ($ds, $drive) = @_;
+                $attached_deviceid = "drive-$ds" if $drive->{file} eq $volid;
+            },
+        );
+    }
+
+    PVE::QemuServer::qemu_volume_snapshot_delete(
+        $vmid, $storecfg, $drive, $snapname, $attached_deviceid, $running,
+    );
 
     push @$unused, $volid;
 }
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 036dc4c1..e76a6ec4 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4366,22 +4366,9 @@ sub qemu_volume_snapshot {
 }
 
 sub qemu_volume_snapshot_delete {
-    my ($vmid, $storecfg, $drive, $snap) = @_;
+    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
 
     my $volid = $drive->{file};
-    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
-    my $attached_deviceid;
-
-    if ($running) {
-        my $conf = PVE::QemuConfig->load_config($vmid);
-        PVE::QemuConfig->foreach_volume(
-            $conf,
-            sub {
-                my ($ds, $drive) = @_;
-                $attached_deviceid = "drive-$ds" if $drive->{file} eq $volid;
-            },
-        );
-    }
 
     my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
 
-- 
2.47.3

[PATCH qemu-server 3/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling

[Fiona Ebner]

When removing a volume chain snapshot, the blockdev properties are
re-generated in blockdev_{commit,replace,stream}() to operate upon.
This must happen with the current drive information, not the one from
the time the snapshot was taken.

A similar bug was present before volume chain snapshots existed, and
fixed by commit c6058683 ("fix #2258: select correct device when
removing drive snapshot via QEMU"). This is where the handling for
$attached_deviceid comes from. With snapshot-as-volume-chain, this
needs to be extended to the full drive information, not just the
device ID.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuConfig.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 3dd69ce8..39e88366 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -441,13 +441,19 @@ sub __snapshot_delete_vol_snapshot {
     my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
     my $attached_deviceid;
 
+    # Need to operate on the currently attached device if the VM is running for QMP handling to
+    # work correctly. For blockdev generation, it's also important that the same drive information
+    # as currently attached is used, because otherwise node names and other settings might differ.
     if ($running) {
         my $conf = $class->load_config($vmid);
         $class->foreach_volume(
             $conf,
             sub {
-                my ($ds, $drive) = @_;
-                $attached_deviceid = "drive-$ds" if $drive->{file} eq $volid;
+                my ($ds, $current_drive) = @_;
+                if ($current_drive->{file} eq $volid) {
+                    $attached_deviceid = "drive-$ds";
+                    $drive = $current_drive;
+                }
             },
         );
     }
-- 
2.47.3

[PATCH qemu-server 4/7] snapshot: add module for snapshot-related functionality

[Fiona Ebner]

Initially, the 'Snapshot' module contains the functions for creating
and removing a snapshot from a volume.

Drops two more calls from QemuConfig back to QemuServer, making
progress towards getting rid of that cyclic dependency.

This was the only usage of the qmp_cmd() helper in QemuServer, so
remove the import.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuConfig.pm          |   5 +-
 src/PVE/QemuServer.pm          | 150 +-----------------------------
 src/PVE/QemuServer/Makefile    |   1 +
 src/PVE/QemuServer/Snapshot.pm | 162 +++++++++++++++++++++++++++++++++
 src/test/snapshot-test.pm      |   7 +-
 5 files changed, 170 insertions(+), 155 deletions(-)
 create mode 100644 src/PVE/QemuServer/Snapshot.pm

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 39e88366..c84f10f8 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -18,6 +18,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer;
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
+use PVE::QemuServer::Snapshot;
 use PVE::RESTEnvironment qw(log_warn);
 use PVE::Storage;
 use PVE::Tools;
@@ -401,7 +402,7 @@ sub __snapshot_create_vol_snapshot {
 
     print "snapshotting '$device' ($drive->{file})\n";
 
-    PVE::QemuServer::qemu_volume_snapshot($vmid, $device, $storecfg, $drive, $snapname);
+    PVE::QemuServer::Snapshot::qemu_volume_snapshot($vmid, $device, $storecfg, $drive, $snapname);
 }
 
 sub __snapshot_delete_remove_drive {
@@ -458,7 +459,7 @@ sub __snapshot_delete_vol_snapshot {
         );
     }
 
-    PVE::QemuServer::qemu_volume_snapshot_delete(
+    PVE::QemuServer::Snapshot::qemu_volume_snapshot_delete(
         $vmid, $storecfg, $drive, $snapname, $attached_deviceid, $running,
     );
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index e76a6ec4..322ad7fa 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -83,7 +83,7 @@ use PVE::QemuServer::DriveDevice qw(print_drivedevice_full scsihw_infos);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
 use PVE::QemuServer::MetaInfo;
-use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd vm_qmp_peer);
+use PVE::QemuServer::Monitor qw(mon_cmd vm_qmp_peer);
 use PVE::QemuServer::Network;
 use PVE::QemuServer::OVMF;
 use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr parse_hostpci get_pci_bridges);
@@ -4332,134 +4332,6 @@ sub qemu_cpu_hotplug {
     }
 }
 
-sub qemu_volume_snapshot {
-    my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
-
-    my $volid = $drive->{file};
-    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
-
-    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $deviceid, $running);
-
-    if ($do_snapshots_type eq 'internal') {
-        print "internal qemu snapshot\n";
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        qmp_cmd($qmp_peer, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
-    } elsif ($do_snapshots_type eq 'external') {
-        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
-        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
-            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
-                . " QEMU machine version >= 10.0. See"
-                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
-        }
-        my $storeid = (PVE::Storage::parse_volume_id($volid))[0];
-        my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-        print "external qemu snapshot\n";
-        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
-        my $parent_snap = $snapshots->{'current'}->{parent};
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        PVE::QemuServer::VolumeChain::blockdev_external_snapshot(
-            $storecfg, $qmp_peer, $machine_version, $deviceid, $drive, $snap, $parent_snap,
-        );
-    } elsif ($do_snapshots_type eq 'storage') {
-        PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
-    }
-}
-
-sub qemu_volume_snapshot_delete {
-    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
-
-    my $volid = $drive->{file};
-
-    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
-
-    if ($do_snapshots_type eq 'internal') {
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        qmp_cmd(
-            $qmp_peer,
-            'blockdev-snapshot-delete-internal-sync',
-            device => $attached_deviceid,
-            name => $snap,
-        );
-    } elsif ($do_snapshots_type eq 'external') {
-        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
-        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
-            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
-                . " QEMU machine version >= 10.0. See"
-                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
-        }
-
-        print "delete qemu external snapshot\n";
-
-        my $path = PVE::Storage::path($storecfg, $volid);
-        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
-
-        die "could not find snapshot '$snap' for volume '$volid'\n"
-            if !defined($snapshots->{$snap});
-
-        my $parentsnap = $snapshots->{$snap}->{parent};
-        my $childsnap = $snapshots->{$snap}->{child};
-
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-
-        # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
-        # improve-me: if firstsnap > child : commit, if firstsnap < child do a stream.
-        if (!$parentsnap) {
-            print "delete first snapshot $snap\n";
-
-            my $snap_size = $snapshots->{$snap}->{'virtual-size'};
-            my $child_size = $snapshots->{$childsnap}->{'virtual-size'};
-            if (defined($child_size) && defined($snap_size) && $child_size > $snap_size) {
-                print
-                    "resize '$snap' ($snap_size bytes) to match '$childsnap' ($child_size bytes)\n";
-                PVE::Storage::volume_resize($storecfg, $volid, $child_size, $running, $snap);
-            }
-
-            PVE::QemuServer::VolumeChain::blockdev_commit(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $childsnap,
-                $snap,
-            );
-
-            PVE::Storage::rename_snapshot($storecfg, $volid, $snap, $childsnap);
-
-            PVE::QemuServer::VolumeChain::blockdev_replace(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $snap,
-                $childsnap,
-                $snapshots->{$childsnap}->{child},
-            );
-        } else {
-            #intermediate snapshot, we always stream the snapshot to child snapshot
-            print "stream intermediate snapshot $snap to $childsnap\n";
-            PVE::QemuServer::VolumeChain::blockdev_stream(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $snap,
-                $parentsnap,
-                $childsnap,
-            );
-        }
-    } elsif ($do_snapshots_type eq 'storage') {
-        PVE::Storage::volume_snapshot_delete(
-            $storecfg,
-            $volid,
-            $snap,
-            $attached_deviceid ? 1 : undef,
-        );
-    }
-}
-
 sub foreach_volid {
     my ($conf, $func, @param) = @_;
 
@@ -7810,26 +7682,6 @@ sub restore_tar_archive {
     warn $@ if $@;
 }
 
-sub do_snapshots_type {
-    my ($storecfg, $drive, $deviceid, $running) = @_;
-
-    #we use storage snapshot if vm is not running or if disk is unused;
-    return 'storage' if !$running || !$deviceid;
-
-    if (
-        $deviceid eq 'drive-tpmstate0'
-        && !PVE::QemuServer::Drive::drive_uses_qsd_fuse($storecfg, $drive)
-    ) {
-        return 'storage';
-    }
-
-    if (my $method = PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file})) {
-        return 'internal' if $method eq 'qemu';
-        return 'external' if $method eq 'mixed';
-    }
-    return 'storage';
-}
-
 =head3 template_create($vmid, $conf [, $disk])
 
 Converts all used disk volumes for the VM with the identifier C<$vmid> and
diff --git a/src/PVE/QemuServer/Makefile b/src/PVE/QemuServer/Makefile
index 060fac23..61ed1a4e 100644
--- a/src/PVE/QemuServer/Makefile
+++ b/src/PVE/QemuServer/Makefile
@@ -27,6 +27,7 @@ SOURCES=Agent.pm	\
 	QSD.pm		\
 	RNG.pm		\
 	RunState.pm	\
+	Snapshot.pm	\
 	StateFile.pm	\
 	USB.pm		\
 	Virtiofs.pm	\
diff --git a/src/PVE/QemuServer/Snapshot.pm b/src/PVE/QemuServer/Snapshot.pm
new file mode 100644
index 00000000..a0d4c239
--- /dev/null
+++ b/src/PVE/QemuServer/Snapshot.pm
@@ -0,0 +1,162 @@
+package PVE::QemuServer::Snapshot;
+
+use strict;
+use warnings;
+
+use PVE::Storage;
+
+use PVE::QemuServer::Drive;
+use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Machine;
+use PVE::QemuServer::Monitor qw(qmp_cmd);
+use PVE::QemuServer::VolumeChain;
+
+sub do_snapshots_type {
+    my ($storecfg, $drive, $deviceid, $running) = @_;
+
+    #we use storage snapshot if vm is not running or if disk is unused;
+    return 'storage' if !$running || !$deviceid;
+
+    if (
+        $deviceid eq 'drive-tpmstate0'
+        && !PVE::QemuServer::Drive::drive_uses_qsd_fuse($storecfg, $drive)
+    ) {
+        return 'storage';
+    }
+
+    if (my $method = PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file})) {
+        return 'internal' if $method eq 'qemu';
+        return 'external' if $method eq 'mixed';
+    }
+    return 'storage';
+}
+
+sub qemu_volume_snapshot {
+    my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
+
+    my $volid = $drive->{file};
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
+
+    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $deviceid, $running);
+
+    if ($do_snapshots_type eq 'internal') {
+        print "internal qemu snapshot\n";
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        qmp_cmd($qmp_peer, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
+    } elsif ($do_snapshots_type eq 'external') {
+        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
+            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
+                . " QEMU machine version >= 10.0. See"
+                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
+        }
+        my $storeid = (PVE::Storage::parse_volume_id($volid))[0];
+        my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+        print "external qemu snapshot\n";
+        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+        my $parent_snap = $snapshots->{'current'}->{parent};
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        PVE::QemuServer::VolumeChain::blockdev_external_snapshot(
+            $storecfg, $qmp_peer, $machine_version, $deviceid, $drive, $snap, $parent_snap,
+        );
+    } elsif ($do_snapshots_type eq 'storage') {
+        PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
+    }
+}
+
+sub qemu_volume_snapshot_delete {
+    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
+
+    my $volid = $drive->{file};
+
+    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
+
+    if ($do_snapshots_type eq 'internal') {
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        qmp_cmd(
+            $qmp_peer,
+            'blockdev-snapshot-delete-internal-sync',
+            device => $attached_deviceid,
+            name => $snap,
+        );
+    } elsif ($do_snapshots_type eq 'external') {
+        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
+            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
+                . " QEMU machine version >= 10.0. See"
+                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
+        }
+
+        print "delete qemu external snapshot\n";
+
+        my $path = PVE::Storage::path($storecfg, $volid);
+        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+
+        die "could not find snapshot '$snap' for volume '$volid'\n"
+            if !defined($snapshots->{$snap});
+
+        my $parentsnap = $snapshots->{$snap}->{parent};
+        my $childsnap = $snapshots->{$snap}->{child};
+
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+
+        # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
+        # improve-me: if firstsnap > child : commit, if firstsnap < child do a stream.
+        if (!$parentsnap) {
+            print "delete first snapshot $snap\n";
+
+            my $snap_size = $snapshots->{$snap}->{'virtual-size'};
+            my $child_size = $snapshots->{$childsnap}->{'virtual-size'};
+            if (defined($child_size) && defined($snap_size) && $child_size > $snap_size) {
+                print
+                    "resize '$snap' ($snap_size bytes) to match '$childsnap' ($child_size bytes)\n";
+                PVE::Storage::volume_resize($storecfg, $volid, $child_size, $running, $snap);
+            }
+
+            PVE::QemuServer::VolumeChain::blockdev_commit(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $childsnap,
+                $snap,
+            );
+
+            PVE::Storage::rename_snapshot($storecfg, $volid, $snap, $childsnap);
+
+            PVE::QemuServer::VolumeChain::blockdev_replace(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $snap,
+                $childsnap,
+                $snapshots->{$childsnap}->{child},
+            );
+        } else {
+            #intermediate snapshot, we always stream the snapshot to child snapshot
+            print "stream intermediate snapshot $snap to $childsnap\n";
+            PVE::QemuServer::VolumeChain::blockdev_stream(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $snap,
+                $parentsnap,
+                $childsnap,
+            );
+        }
+    } elsif ($do_snapshots_type eq 'storage') {
+        PVE::Storage::volume_snapshot_delete(
+            $storecfg,
+            $volid,
+            $snap,
+            $attached_deviceid ? 1 : undef,
+        );
+    }
+}
+
+1;
diff --git a/src/test/snapshot-test.pm b/src/test/snapshot-test.pm
index 0c5715c5..44e55d6a 100644
--- a/src/test/snapshot-test.pm
+++ b/src/test/snapshot-test.pm
@@ -404,10 +404,6 @@ sub set_migration_caps { } # ignored
 
 # BEGIN redefine PVE::QemuServer methods
 
-sub do_snapshots_type {
-    return 'storage';
-}
-
 sub vm_start {
     my ($storecfg, $vmid, $params, $migrate_opts) = @_;
 
@@ -463,6 +459,9 @@ $qemu_config_module->mock('has_feature', \&has_feature);
 $qemu_config_module->mock('__snapshot_save_vmstate', \&__snapshot_save_vmstate);
 $qemu_config_module->mock('assert_config_exists_on_node', \&assert_config_exists_on_node);
 
+my $qemu_snapshot_module = Test::MockModule->new('PVE::QemuServer::Snapshot');
+$qemu_snapshot_module->mock('do_snapshots_type' => sub { return 'storage'; });
+
 # ignore existing replication config
 my $repl_config_module = Test::MockModule->new('PVE::ReplicationConfig');
 $repl_config_module->mock('new' => sub { return bless {}, "PVE::ReplicationConfig" });
-- 
2.47.3

[PATCH qemu-server 5/7] snapshot: use v5.36 and subroutine signatures

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/Snapshot.pm | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/src/PVE/QemuServer/Snapshot.pm b/src/PVE/QemuServer/Snapshot.pm
index a0d4c239..50f6ac67 100644
--- a/src/PVE/QemuServer/Snapshot.pm
+++ b/src/PVE/QemuServer/Snapshot.pm
@@ -1,7 +1,6 @@
 package PVE::QemuServer::Snapshot;
 
-use strict;
-use warnings;
+use v5.36;
 
 use PVE::Storage;
 
@@ -11,9 +10,7 @@ use PVE::QemuServer::Machine;
 use PVE::QemuServer::Monitor qw(qmp_cmd);
 use PVE::QemuServer::VolumeChain;
 
-sub do_snapshots_type {
-    my ($storecfg, $drive, $deviceid, $running) = @_;
-
+sub do_snapshots_type($storecfg, $drive, $deviceid, $running) {
     #we use storage snapshot if vm is not running or if disk is unused;
     return 'storage' if !$running || !$deviceid;
 
@@ -31,9 +28,7 @@ sub do_snapshots_type {
     return 'storage';
 }
 
-sub qemu_volume_snapshot {
-    my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
-
+sub qemu_volume_snapshot($vmid, $deviceid, $storecfg, $drive, $snap) {
     my $volid = $drive->{file};
     my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
 
@@ -64,9 +59,7 @@ sub qemu_volume_snapshot {
     }
 }
 
-sub qemu_volume_snapshot_delete {
-    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
-
+sub qemu_volume_snapshot_delete($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) {
     my $volid = $drive->{file};
 
     my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
-- 
2.47.3

[PATCH qemu-server 6/7] volume chain: blockdev delete: remove superfluous parse_volume_id() call

[Fiona Ebner]

The result is not used and volume_snapshot_delete() already dies for
invalid volume IDs.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/VolumeChain.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/PVE/QemuServer/VolumeChain.pm b/src/PVE/QemuServer/VolumeChain.pm
index 1572450b..1ad137ed 100644
--- a/src/PVE/QemuServer/VolumeChain.pm
+++ b/src/PVE/QemuServer/VolumeChain.pm
@@ -72,8 +72,6 @@ sub blockdev_delete {
     #delete the file (don't use vdisk_free as we don't want to delete all snapshot chain)
     print "delete old $file_blockdev->{filename}\n";
 
-    my $storage_name = PVE::Storage::parse_volume_id($drive->{file});
-
     my $volid = $drive->{file};
     PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snap, 1);
 }
-- 
2.47.3

[PATCH qemu-server 7/7] volume chain: blockdev delete: pass volume ID instead of drive and mark private

[Fiona Ebner]

The only part of the drive that's used by blockdev_delete() is the
volume ID.

Remove the local volid variables in blockdev_{commit,stream}() which
are in-scope for the whole function but never used.

The blockdev_delete() function is not used outside of the VolumeChain
module and it has a quite specific signature tailored towards its two
existing callers.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/VolumeChain.pm | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/PVE/QemuServer/VolumeChain.pm b/src/PVE/QemuServer/VolumeChain.pm
index 1ad137ed..f52deead 100644
--- a/src/PVE/QemuServer/VolumeChain.pm
+++ b/src/PVE/QemuServer/VolumeChain.pm
@@ -63,8 +63,8 @@ sub blockdev_external_snapshot {
     );
 }
 
-sub blockdev_delete {
-    my ($storecfg, $qmp_peer, $drive, $file_blockdev, $fmt_blockdev, $snap) = @_;
+my sub blockdev_delete {
+    my ($storecfg, $qmp_peer, $volid, $file_blockdev, $fmt_blockdev, $snap) = @_;
 
     eval { PVE::QemuServer::Blockdev::detach($qmp_peer, $fmt_blockdev->{'node-name'}); };
     warn "detaching block node for $file_blockdev->{filename} failed - $@" if $@;
@@ -72,7 +72,6 @@ sub blockdev_delete {
     #delete the file (don't use vdisk_free as we don't want to delete all snapshot chain)
     print "delete old $file_blockdev->{filename}\n";
 
-    my $volid = $drive->{file};
     PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snap, 1);
 }
 
@@ -192,7 +191,6 @@ sub blockdev_replace {
 sub blockdev_commit {
     my ($storecfg, $qmp_peer, $machine_version, $deviceid, $drive, $src_snap, $target_snap) = @_;
 
-    my $volid = $drive->{file};
     my $target_was_read_only;
 
     print "block-commit $src_snap to base:$target_snap\n";
@@ -256,7 +254,12 @@ sub blockdev_commit {
         PVE::QemuServer::BlockJob::monitor($qmp_peer, undef, $jobs, $complete, 0, 'commit');
 
         blockdev_delete(
-            $storecfg, $qmp_peer, $drive, $src_file_blockdev, $src_fmt_blockdev, $src_snap,
+            $storecfg,
+            $qmp_peer,
+            $drive->{file},
+            $src_file_blockdev,
+            $src_fmt_blockdev,
+            $src_snap,
         );
     };
     my $err = $@;
@@ -286,7 +289,6 @@ sub blockdev_stream {
         $target_snap,
     ) = @_;
 
-    my $volid = $drive->{file};
     $target_snap = undef if $target_snap eq 'current';
 
     my $parent_file_blockdev = generate_file_blockdev(
@@ -338,7 +340,7 @@ sub blockdev_stream {
     PVE::QemuServer::BlockJob::monitor($qmp_peer, undef, $jobs, 'auto', 0, 'stream');
 
     blockdev_delete(
-        $storecfg, $qmp_peer, $drive, $snap_file_blockdev, $snap_fmt_blockdev, $snap,
+        $storecfg, $qmp_peer, $drive->{file}, $snap_file_blockdev, $snap_fmt_blockdev, $snap,
     );
 }
 
-- 
2.47.3