* [PATCH qemu-server 4/7] snapshot: add module for snapshot-related functionality
2026-09-25 12:26 [PATCH-SERIES qemu-server 0/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling Fiona Ebner
` (2 preceding siblings ...)
2026-09-25 12:26 ` [PATCH qemu-server 3/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling Fiona Ebner
@ 2026-09-25 12:26 ` Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 5/7] snapshot: use v5.36 and subroutine signatures Fiona Ebner
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2026-09-25 12:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH qemu-server 7/7] volume chain: blockdev delete: pass volume ID instead of drive and mark private
2026-09-25 12:26 [PATCH-SERIES qemu-server 0/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling Fiona Ebner
` (5 preceding siblings ...)
2026-09-25 12:26 ` [PATCH qemu-server 6/7] volume chain: blockdev delete: remove superfluous parse_volume_id() call Fiona Ebner
@ 2026-09-25 12:26 ` Fiona Ebner
6 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2026-09-25 12:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread