[pve-devel] [PATCH qemu-server 2/3] image convert: re-use generate_drive_blockdev()

From: Fiona Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH qemu-server 2/3] image convert: re-use generate_drive_blockdev()
Date: Wed, 30 Jul 2025 17:03:16 +0200	[thread overview]
Message-ID: <20250730150325.138087-3-f.ebner@proxmox.com> (raw)
In-Reply-To: <20250730150325.138087-1-f.ebner@proxmox.com>

This avoids having the handling for 'discard-no-unref' in two places.

In the tests, rename the relevant target images with a '-target'
suffix to test for them in the mocked volume_snapshot_info() helper.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes since last iteration:
* rebase + make tidy

 src/PVE/QemuServer/QemuImage.pm        | 65 +++++++++++++++++++++++---
 src/test/run_qemu_img_convert_tests.pl | 24 +++++++---
 2 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
index 8fe75e92..f2cadd69 100644
--- a/src/PVE/QemuServer/QemuImage.pm
+++ b/src/PVE/QemuServer/QemuImage.pm
@@ -5,11 +5,13 @@ use warnings;
 
 use Fcntl qw(S_ISBLK);
 use File::stat;
+use JSON;
 
 use PVE::Format qw(render_bytes);
 use PVE::Storage;
 use PVE::Tools;
 
+use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::Drive qw(checked_volume_format);
 use PVE::QemuServer::Helpers;
 
@@ -31,16 +33,62 @@ sub convert_iscsi_path {
 }
 
 my sub qcow2_target_image_opts {
-    my ($path, @qcow2_opts) = @_;
+    my ($storecfg, $drive, @qcow2_opts) = @_;
 
-    # FIXME this duplicates logic from qemu_blockdev_options
-    my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
+    # There is no machine version, the qemu-img binary version is what's important.
+    my $version = PVE::QemuServer::Helpers::kvm_user_version();
 
-    my $driver = S_ISBLK($st->mode) ? 'host_device' : 'file';
+    my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
+        $storecfg,
+        $drive,
+        $version,
+        { 'no-throttle' => 1 },
+    );
 
-    my $qcow2_opts_str = ',' . join(',', @qcow2_opts);
+    my $opts = [];
+    my $opt_prefix = '';
+    my $next_child = $blockdev;
+    while ($next_child) {
+        my $current = $next_child;
+        $next_child = delete($current->{file});
 
-    return "driver=qcow2$qcow2_opts_str,file.driver=$driver,file.filename=$path";
+        # TODO should cache settings be configured here (via appropriate drive configuration) rather
+        # than via dedicated qemu-img options?
+        delete($current->{cache});
+        # TODO e.g. can't use aio 'native' without cache.direct, just use QEMU default like for
+        # other targets for now
+        delete($current->{aio});
+
+        # no need for node names
+        delete($current->{'node-name'});
+
+        # it's the write target, while the flag should be 'false' anyways, remove to be sure
+        delete($current->{'read-only'});
+
+        # TODO should those be set (via appropriate drive configuration)?
+        delete($current->{'detect-zeroes'});
+        delete($current->{'discard'});
+
+        for my $key (sort keys $current->%*) {
+            my $value;
+            if (ref($current->{$key})) {
+                if ($current->{$key} eq JSON::false) {
+                    $value = 'false';
+                } elsif ($current->{$key} eq JSON::true) {
+                    $value = 'true';
+                } else {
+                    die "target image options: unhandled structured key: $key\n";
+                }
+            } else {
+                $value = $current->{$key};
+            }
+            push $opts->@*, "$opt_prefix$key=$value";
+        }
+
+        $opt_prefix .= 'file.';
+    }
+
+    return join(',', $opts->@*);
 }
 
 # The possible options are:
@@ -115,7 +163,10 @@ sub convert {
     if ($dst_is_iscsi) {
         $dst_path = convert_iscsi_path($dst_path);
     } elsif ($dst_needs_discard_no_unref) {
-        $dst_path = qcow2_target_image_opts($dst_path, 'discard-no-unref=true');
+        # don't use any other drive options, those are intended for use with a running VM and just
+        # use scsi0 as a dummy interface+index for now
+        my $dst_drive = { file => $dst_volid, interface => 'scsi', index => 0 };
+        $dst_path = qcow2_target_image_opts($storecfg, $dst_drive, 'discard-no-unref=true');
     } else {
         push @$cmd, '-O', $dst_format;
     }
diff --git a/src/test/run_qemu_img_convert_tests.pl b/src/test/run_qemu_img_convert_tests.pl
index 3c8f09f0..393fc4a8 100755
--- a/src/test/run_qemu_img_convert_tests.pl
+++ b/src/test/run_qemu_img_convert_tests.pl
@@ -532,7 +532,7 @@ my $tests = [
         name => "qcow2_external_snapshot_target",
         parameters => [
             "local:$vmid/vm-$vmid-disk-0.raw",
-            "localsnapext:$vmid/vm-$vmid-disk-0.qcow2",
+            "localsnapext:$vmid/vm-$vmid-disk-target.qcow2",
             1024 * 10,
         ],
         expected => [
@@ -544,14 +544,15 @@ my $tests = [
             "raw",
             "--target-image-opts",
             "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
-            "driver=qcow2,discard-no-unref=true,file.driver=file,"
-                . "file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-0.qcow2",
+            "discard-no-unref=true,driver=qcow2,file.driver=file"
+                . ",file.filename=/var/lib/vzsnapext/images/$vmid/vm-$vmid-disk-target.qcow2",
         ],
     },
     {
         name => "lvmqcow2_external_snapshot_target",
         parameters => [
-            "local:$vmid/vm-$vmid-disk-0.raw", "lvm-store:vm-$vmid-disk-0.qcow2", 1024 * 10,
+            "local:$vmid/vm-$vmid-disk-0.raw", "lvm-store:vm-$vmid-disk-target.qcow2",
+            1024 * 10,
         ],
         expected => [
             "/usr/bin/qemu-img",
@@ -562,8 +563,8 @@ my $tests = [
             "raw",
             "--target-image-opts",
             "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
-            "driver=qcow2,discard-no-unref=true,file.driver=host_device,"
-                . "file.filename=/dev/pve/vm-$vmid-disk-0.qcow2",
+            "discard-no-unref=true,driver=qcow2,file.driver=host_device"
+                . ",file.filename=/dev/pve/vm-$vmid-disk-target.qcow2",
         ],
     },
 ];
@@ -588,6 +589,17 @@ $storage_module->mock(
     activate_volumes => sub {
         return 1;
     },
+    volume_snapshot_info => sub {
+        my ($cfg, $volid) = @_;
+        if (
+            $volid eq "lvm-store:vm-$vmid-disk-target.qcow2"
+            || $volid eq "localsnapext:$vmid/vm-$vmid-disk-target.qcow2"
+        ) {
+            # target volumes don't have snapshots
+            return {};
+        }
+        die "mocked volume_snapshot_info called with unexpected volid $volid\n";
+    },
 );
 
 my $lio_module = Test::MockModule->new("PVE::Storage::LunCmd::LIO");
-- 
2.47.2



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