From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40])
	by lore.proxmox.com (Postfix) with ESMTPS id A29021FF0E5
	for <inbox@lore.proxmox.com>; Wed, 15 Jul 2026 16:26:47 +0200 (CEST)
Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1])
	by gate001.proxmox.com (Proxmox) with ESMTP id BE4FC21441;
	Wed, 15 Jul 2026 16:26:46 +0200 (CEST)
From: Fiona Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [PATCH qemu-server] clone disk: refactor code to avoid goto
Date: Wed, 15 Jul 2026 16:26:00 +0200
Message-ID: <20260715142608.152953-1-f.ebner@proxmox.com>
X-Mailer: git-send-email 2.47.3
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2
X-Bm-Transport-Timestamp: 1784125553886
X-SPAM-LEVEL: Spam detection results:  0
	AWL                     0.243 Adjusted score from AWL reputation of From:
 address
	DMARC_MISSING             0.1 Missing DMARC policy
	KAM_DMARC_STATUS         0.01 Test Rule for DKIM or SPF Failure with Strict
 Alignment (newer systems)
	RCVD_IN_DNSWL_LOW        -0.7 Sender listed at https://www.dnswl.org/,
 low trust
	SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
	SPF_PASS               -0.001 SPF: sender matches SPF record
Message-ID-Hash: 2KPY2NPHQ5XGYE3LOVD4TJGWNVOQFZCS
X-Message-ID-Hash: 2KPY2NPHQ5XGYE3LOVD4TJGWNVOQFZCS
X-MailFrom: f.ebner@proxmox.com
X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop;
 banned-address; emergency; member-moderation; nonmember-moderation;
 administrivia; implicit-dest; max-recipients; max-size; news-moderation;
 no-subject; digests; suspicious-header
X-Mailman-Version: 3.3.10
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Owner: <mailto:pve-devel-owner@lists.proxmox.com>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Subscribe: <mailto:pve-devel-join@lists.proxmox.com>
List-Unsubscribe: <mailto:pve-devel-leave@lists.proxmox.com>

No functional change intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

The new proxmox-perltidy=1.3 version wants to indent the label:

* do not outdent labels: in the Proxmox codebases labels are used for
labeled blocks and loops (next <label>), not as global goto targets,
so outdenting them is unhelpful; where it is wanted, guard the label
with '#<<<'/'#>>>' markers.

Instead of adding markers to prevent this, just get rid of the goto.
This is the only goto in the codebase except one for AUTOLOAD in
RESTHandler.pm.

 src/PVE/QemuServer.pm | 121 ++++++++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 53 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 191ae549..774adcc7 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7923,6 +7923,67 @@ my sub clone_disk_check_io_uring {
         if $src_uses_io_uring && !storage_allows_io_uring_default($dst_scfg, $cache_direct);
 }
 
+my sub clone_disk_copy_data {
+    my ($storecfg, $source, $dest, $use_drive_mirror, $jobs, $completion, $qga, $bwlimit) = @_;
+
+    my ($vmid, $drive, $size, $snapname) = $source->@{qw(vmid drive size snapname)};
+    my ($newvmid, $newvolid, $dst_drivename, $dst_format) =
+        $dest->@{qw(vmid volid drivename format)};
+
+    my $sparseinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $newvolid);
+    if ($use_drive_mirror) {
+        my $source_info = { vmid => $vmid, drive => $drive };
+        my $dest_info = { volid => $newvolid };
+        $dest_info->{'zero-initialized'} = 1 if $sparseinit;
+        $dest_info->{vmid} = $newvmid if defined($newvmid);
+        my $mirror_opts = {};
+        $mirror_opts->{'guest-agent'} = $qga;
+        $mirror_opts->{bwlimit} = $bwlimit if defined($bwlimit);
+        PVE::QemuServer::BlockJob::mirror(
+            $source_info,
+            $dest_info,
+            $jobs,
+            $completion,
+            $mirror_opts,
+        );
+    } else {
+        if ($dst_drivename eq 'efidisk0') {
+            # the relevant data on the efidisk may be smaller than the source
+            # e.g. on RBD/ZFS, so we use dd to copy only the amount
+            # that is given by the OVMF_VARS.fd
+            my $src_path = PVE::Storage::path($storecfg, $drive->{file}, $snapname);
+            my $dst_path = PVE::Storage::path($storecfg, $newvolid);
+
+            my $src_format = (PVE::Storage::parse_volname($storecfg, $drive->{file}))[6];
+
+            # better for Ceph if block size is not too small, see bug #3324
+            my $bs = 1024 * 1024;
+
+            my $cmd = ['qemu-img', 'dd', '-n', '-f', $src_format, '-O', $dst_format];
+
+            if ($src_format eq 'qcow2' && $snapname) {
+                die "cannot clone qcow2 EFI disk snapshot - requires QEMU >= 6.2\n"
+                    if !min_version(kvm_user_version(), 6, 2);
+
+                my $method = PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file});
+                # in case of snapshot-as-volume-chain, $src_path points to the snapshot volume
+                push $cmd->@*, '-l', $snapname if $method eq 'qemu';
+            }
+            push $cmd->@*, "bs=$bs", "osize=$size", "if=$src_path", "of=$dst_path";
+            run_command($cmd);
+        } else {
+            my $opts = {
+                bwlimit => $bwlimit,
+                'is-zero-initialized' => $sparseinit,
+                snapname => $snapname,
+            };
+            PVE::QemuServer::QemuImage::convert($drive->{file}, $newvolid, $size, $opts);
+        }
+    }
+
+    return;
+}
+
 sub clone_disk {
     my ($storecfg, $source, $dest, $full, $newvollist, $jobs, $completion, $qga, $bwlimit) = @_;
 
@@ -7967,6 +8028,7 @@ sub clone_disk {
             warn "format '$format' is not supported by the target storage '$storeid' - using"
                 . " '$dst_format' instead\n";
         }
+        $dest->{format} = $dst_format;
 
         my $name = undef;
         my $size = undef;
@@ -7989,6 +8051,7 @@ sub clone_disk {
 
             $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
         }
+        $source->{size} = $size;
         $newvolid = PVE::Storage::vdisk_alloc(
             $storecfg,
             $storeid,
@@ -7998,6 +8061,7 @@ sub clone_disk {
             ($size / 1024),
         );
         push @$newvollist, $newvolid;
+        $dest->{volid} = $newvolid;
 
         print("allocated target volume '$newvolid'\n");
 
@@ -8012,63 +8076,14 @@ sub clone_disk {
                     vm_qmp_peer($vmid), $newvmid, $jobs, $completion, $qga,
                 );
             }
-            goto no_data_clone;
-        }
-
-        my $sparseinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $newvolid);
-        if ($use_drive_mirror) {
-            my $source_info = { vmid => $vmid, drive => $drive };
-            my $dest_info = { volid => $newvolid };
-            $dest_info->{'zero-initialized'} = 1 if $sparseinit;
-            $dest_info->{vmid} = $newvmid if defined($newvmid);
-            my $mirror_opts = {};
-            $mirror_opts->{'guest-agent'} = $qga;
-            $mirror_opts->{bwlimit} = $bwlimit if defined($bwlimit);
-            PVE::QemuServer::BlockJob::mirror(
-                $source_info,
-                $dest_info,
-                $jobs,
-                $completion,
-                $mirror_opts,
-            );
+            # No data needs to be copied for the cloudinit drive.
         } else {
-            if ($dst_drivename eq 'efidisk0') {
-                # the relevant data on the efidisk may be smaller than the source
-                # e.g. on RBD/ZFS, so we use dd to copy only the amount
-                # that is given by the OVMF_VARS.fd
-                my $src_path = PVE::Storage::path($storecfg, $drive->{file}, $snapname);
-                my $dst_path = PVE::Storage::path($storecfg, $newvolid);
-
-                my $src_format = (PVE::Storage::parse_volname($storecfg, $drive->{file}))[6];
-
-                # better for Ceph if block size is not too small, see bug #3324
-                my $bs = 1024 * 1024;
-
-                my $cmd = ['qemu-img', 'dd', '-n', '-f', $src_format, '-O', $dst_format];
-
-                if ($src_format eq 'qcow2' && $snapname) {
-                    die "cannot clone qcow2 EFI disk snapshot - requires QEMU >= 6.2\n"
-                        if !min_version(kvm_user_version(), 6, 2);
-
-                    my $method =
-                        PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file});
-                    # in case of snapshot-as-volume-chain, $src_path points to the snapshot volume
-                    push $cmd->@*, '-l', $snapname if $method eq 'qemu';
-                }
-                push $cmd->@*, "bs=$bs", "osize=$size", "if=$src_path", "of=$dst_path";
-                run_command($cmd);
-            } else {
-                my $opts = {
-                    bwlimit => $bwlimit,
-                    'is-zero-initialized' => $sparseinit,
-                    snapname => $snapname,
-                };
-                PVE::QemuServer::QemuImage::convert($drive->{file}, $newvolid, $size, $opts);
-            }
+            clone_disk_copy_data(
+                $storecfg, $source, $dest, $use_drive_mirror, $jobs, $completion, $qga, $bwlimit,
+            );
         }
     }
 
-no_data_clone:
     my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
 
     my $disk = dclone($drive);
-- 
2.47.3