public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev
@ 2025-08-12 14:37 Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template Fiona Ebner
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

Changes in v2:
* new approach, attach as ide-cd rather than using older machine
  version, which would break qcow2 snapshot-as-volume-chain
* get rid of now outdated drive_is_read_only() helper

With ide-hd, the inserted block node needs to be marked as writable
too, but -blockdev will complain if it's marked as writable but the
actual backing device is read-only (e.g. read-only base LV).

IDE/SATA do not support being configured as read-only, the most
similar is using ide-cd instead of ide-hd, with most of the code and
configuration shared in QEMU.

Since a template is never actually started, the front-end device is
never accessed. The backup only accesses the inserted block node, so
it does not matter for the backup if the type is 'ide-cd' instead.

The same issue did not manifest for '-drive', because the '-snapshot'
option is used for template backups. The '-snapshot' option does not
affect '-blockdev', from 'man kvm':

> snapshot is incompatible with -blockdev

Fiona Ebner (5):
  ovmf: pass along whether the VM is a template
  code cleanup: cfg2cmd: check if configuration is for template
    centrally
  fix #6675: template backup: fix regression with IDE/SATA and blockdev
  code cleanup: drive: get rid of outdated drive_is_read_only() helper
  cfg2cmd: add reminder comments to remove template handling for -drive

 src/PVE/QemuServer.pm                      | 37 ++++++++++++++++------
 src/PVE/QemuServer/Drive.pm                | 10 ------
 src/PVE/QemuServer/OVMF.pm                 | 20 ++++++------
 src/test/cfg2cmd/simple1-template.conf.cmd |  4 +--
 4 files changed, 39 insertions(+), 32 deletions(-)

-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
@ 2025-08-12 14:37 ` Fiona Ebner
  2025-08-13  7:37   ` Fabian Grünbichler
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 2/5] code cleanup: cfg2cmd: check if configuration is for template centrally Fiona Ebner
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

This is in preparation to remove the hidden dependency from the Drive
module to QemuConfig.

Note that the drive_is_read_only() can be replaced with $is_template
for OVMF, because the helper only behaves differently for IDE and
SATA, but not for EFI disks.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm      |  3 ++-
 src/PVE/QemuServer/OVMF.pm | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cfc54568..d6657a1f 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3361,8 +3361,9 @@ sub config_to_command {
             'machine-version' => $machine_version,
             q35 => $q35,
         };
+        my $is_template = PVE::QemuConfig->is_template($conf);
         my ($ovmf_cmd, $ovmf_machine_flags) = PVE::QemuServer::OVMF::print_ovmf_commandline(
-            $conf, $storecfg, $vmid, $hw_info, $version_guard,
+            $conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template,
         );
         push $cmd->@*, $ovmf_cmd->@*;
         push $machineFlags->@*, $ovmf_machine_flags->@*;
diff --git a/src/PVE/QemuServer/OVMF.pm b/src/PVE/QemuServer/OVMF.pm
index 8948651c..72f5be0d 100644
--- a/src/PVE/QemuServer/OVMF.pm
+++ b/src/PVE/QemuServer/OVMF.pm
@@ -10,7 +10,7 @@ use PVE::Storage;
 use PVE::Tools;
 
 use PVE::QemuServer::Blockdev;
-use PVE::QemuServer::Drive qw(checked_volume_format drive_is_read_only parse_drive print_drive);
+use PVE::QemuServer::Drive qw(checked_volume_format parse_drive print_drive);
 use PVE::QemuServer::QemuImage;
 
 my $EDK2_FW_BASE = '/usr/share/pve-edk2-firmware/';
@@ -79,7 +79,7 @@ my sub get_ovmf_files($$$$) {
 }
 
 my sub print_ovmf_drive_commandlines {
-    my ($conf, $storecfg, $vmid, $hw_info, $version_guard) = @_;
+    my ($conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template) = @_;
 
     my ($amd_sev_type, $arch, $q35) = $hw_info->@{qw(amd-sev-type arch q35)};
 
@@ -109,7 +109,7 @@ my sub print_ovmf_drive_commandlines {
 
         $var_drive_str .= ",size=" . (-s $ovmf_vars)
             if $format eq 'raw' && $version_guard->(4, 1, 2);
-        $var_drive_str .= ',readonly=on' if drive_is_read_only($conf, $d);
+        $var_drive_str .= ',readonly=on' if $is_template;
     } else {
         log_warn("no efidisk configured! Using temporary efivars disk.");
         my $path = "/tmp/$vmid-ovmf.fd";
@@ -145,7 +145,7 @@ sub create_efidisk($$$$$$$$) {
 }
 
 my sub generate_ovmf_blockdev {
-    my ($conf, $storecfg, $vmid, $hw_info) = @_;
+    my ($conf, $storecfg, $vmid, $hw_info, $is_template) = @_;
 
     my ($amd_sev_type, $arch, $machine_version, $q35) =
         $hw_info->@{qw(amd-sev-type arch machine-version q35)};
@@ -187,8 +187,7 @@ my sub generate_ovmf_blockdev {
     $drive->{cache} = 'writeback' if !$drive->{cache};
 
     my $extra_blockdev_options = {};
-    # extra protection for templates, but SATA and IDE don't support it..
-    $extra_blockdev_options->{'read-only'} = 1 if drive_is_read_only($conf, $drive);
+    $extra_blockdev_options->{'read-only'} = 1 if $is_template;
 
     $extra_blockdev_options->{size} = -s $ovmf_vars if $format eq 'raw';
 
@@ -202,7 +201,7 @@ my sub generate_ovmf_blockdev {
 }
 
 sub print_ovmf_commandline {
-    my ($conf, $storecfg, $vmid, $hw_info, $version_guard) = @_;
+    my ($conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template) = @_;
 
     my $amd_sev_type = $hw_info->{'amd-sev-type'};
 
@@ -217,7 +216,7 @@ sub print_ovmf_commandline {
     } else {
         if ($version_guard->(10, 0, 0)) { # for the switch to -blockdev
             my ($code_blockdev, $vars_blockdev, $throttle_group) =
-                generate_ovmf_blockdev($conf, $storecfg, $vmid, $hw_info);
+                generate_ovmf_blockdev($conf, $storecfg, $vmid, $hw_info, $is_template);
 
             push $cmd->@*, '-object', to_json($throttle_group, { canonical => 1 });
             push $cmd->@*, '-blockdev', to_json($code_blockdev, { canonical => 1 });
@@ -225,8 +224,9 @@ sub print_ovmf_commandline {
             push $machine_flags->@*, "pflash0=$code_blockdev->{'node-name'}",
                 "pflash1=$vars_blockdev->{'node-name'}";
         } else {
-            my ($code_drive_str, $var_drive_str) =
-                print_ovmf_drive_commandlines($conf, $storecfg, $vmid, $hw_info, $version_guard);
+            my ($code_drive_str, $var_drive_str) = print_ovmf_drive_commandlines(
+                $conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template,
+            );
             push $cmd->@*, '-drive', $code_drive_str;
             push $cmd->@*, '-drive', $var_drive_str;
         }
-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server v2 2/5] code cleanup: cfg2cmd: check if configuration is for template centrally
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template Fiona Ebner
@ 2025-08-12 14:37 ` Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 3/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d6657a1f..67db9bc3 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3205,9 +3205,11 @@ sub config_to_command {
     my ($forcemachine, $forcecpu, $live_restore_backing, $dry_run) =
         $options->@{qw(force-machine force-cpu live-restore-backing dry-run)};
 
+    my $is_template = PVE::QemuConfig->is_template($conf);
+
     # minimize config for templates, they can only start for backup,
     # so most options besides the disks are irrelevant
-    if (PVE::QemuConfig->is_template($conf)) {
+    if ($is_template) {
         my $newconf = {
             template => 1, # in case below code checks that
             kvm => 0, # to prevent an error on hosts without virtualization extensions
@@ -3361,7 +3363,6 @@ sub config_to_command {
             'machine-version' => $machine_version,
             q35 => $q35,
         };
-        my $is_template = PVE::QemuConfig->is_template($conf);
         my ($ovmf_cmd, $ovmf_machine_flags) = PVE::QemuServer::OVMF::print_ovmf_commandline(
             $conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template,
         );
@@ -3463,7 +3464,7 @@ sub config_to_command {
 
     # Add a TPM only if the VM is not a template,
     # to support backing up template VMs even if the TPM disk is write-protected.
-    add_tpm_device($vmid, $devices, $conf) if (!PVE::QemuConfig->is_template($conf));
+    add_tpm_device($vmid, $devices, $conf) if !$is_template;
 
     my $sockets = 1;
     $sockets = $conf->{smp} if $conf->{smp}; # old style - no longer iused
@@ -3900,7 +3901,7 @@ sub config_to_command {
         print "activating and using '$vmstate' as vmstate\n";
     }
 
-    if (PVE::QemuConfig->is_template($conf)) {
+    if ($is_template) {
         # needed to workaround base volumes being read-only
         push @$cmd, '-snapshot';
     }
-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server v2 3/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 2/5] code cleanup: cfg2cmd: check if configuration is for template centrally Fiona Ebner
@ 2025-08-12 14:37 ` Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 4/5] code cleanup: drive: get rid of outdated drive_is_read_only() helper Fiona Ebner
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

With ide-hd, the inserted block node needs to be marked as writable
too, but -blockdev will complain if it's marked as writable but the
actual backing device is read-only (e.g. read-only base LV).

IDE/SATA do not support being configured as read-only, the most
similar is using ide-cd instead of ide-hd, with most of the code and
configuration shared in QEMU.

Since a template is never actually started, the front-end device is
never accessed. The backup only accesses the inserted block node, so
it does not matter for the backup if the type is 'ide-cd' instead.

The same issue did not manifest for '-drive', because the '-snapshot'
option is used for template backups. The '-snapshot' option does not
affect '-blockdev', from 'man kvm':

> snapshot is incompatible with -blockdev

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm                      | 13 ++++++++++---
 src/test/cfg2cmd/simple1-template.conf.cmd |  4 ++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 67db9bc3..d3f0c589 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1272,6 +1272,15 @@ sub print_drivedevice_full {
 
         my $device_type = ($drive->{media} && $drive->{media} eq 'cdrom') ? "cd" : "hd";
 
+        # With ide-hd, the inserted block node needs to be marked as writable too, but -blockdev
+        # will complain if it's marked as writable but the actual backing device is read-only (e.g.
+        # read-only base LV). IDE/SATA do not support being configured as read-only, the most
+        # similar is using ide-cd instead of ide-hd, with most of the code and configuration shared
+        # in QEMU. Since a template is never actually started, the front-end device is never
+        # accessed. The backup only accesses the inserted block node, so it does not matter for the
+        # backup if the type is 'ide-cd' instead.
+        $device_type = 'cd' if $conf->{template};
+
         $device = "ide-$device_type";
         if ($drive->{interface} eq 'ide') {
             $device .= ",bus=ide.$controller,unit=$unit";
@@ -3752,9 +3761,7 @@ sub config_to_command {
 
                     my $extra_blockdev_options = {};
                     $extra_blockdev_options->{'live-restore'} = $live_restore if $live_restore;
-                    # extra protection for templates, but SATA and IDE don't support it..
-                    $extra_blockdev_options->{'read-only'} = 1
-                        if drive_is_read_only($conf, $drive);
+                    $extra_blockdev_options->{'read-only'} = 1 if $is_template;
 
                     my $blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
                         $storecfg, $drive, $machine_version, $extra_blockdev_options,
diff --git a/src/test/cfg2cmd/simple1-template.conf.cmd b/src/test/cfg2cmd/simple1-template.conf.cmd
index df51a4a5..369b4de9 100644
--- a/src/test/cfg2cmd/simple1-template.conf.cmd
+++ b/src/test/cfg2cmd/simple1-template.conf.cmd
@@ -30,7 +30,7 @@
   -blockdev '{"detect-zeroes":"unmap","discard":"unmap","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"file","filename":"/var/lib/vz/images/8006/base-8006-disk-1.qcow2","node-name":"e1085774206ae4a6b6bf8426ff08f16","read-only":true},"node-name":"f1085774206ae4a6b6bf8426ff08f16","read-only":true},"node-name":"drive-scsi0","read-only":true,"throttle-group":"throttle-drive-scsi0"}' \
   -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,device_id=drive-scsi0,write-cache=on' \
   -device 'ahci,id=ahci0,multifunction=on,bus=pci.0,addr=0x7' \
-  -blockdev '{"detect-zeroes":"unmap","discard":"unmap","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"file","filename":"/var/lib/vz/images/8006/base-8006-disk-0.qcow2","node-name":"eab334c2e07734480f33dd80d89871b","read-only":false},"node-name":"fab334c2e07734480f33dd80d89871b","read-only":false},"node-name":"drive-sata0","read-only":false,"throttle-group":"throttle-drive-sata0"}' \
-  -device 'ide-hd,bus=ahci0.0,drive=drive-sata0,id=sata0,write-cache=on' \
+  -blockdev '{"detect-zeroes":"unmap","discard":"unmap","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"unmap","discard":"unmap","driver":"file","filename":"/var/lib/vz/images/8006/base-8006-disk-0.qcow2","node-name":"eab334c2e07734480f33dd80d89871b","read-only":true},"node-name":"fab334c2e07734480f33dd80d89871b","read-only":true},"node-name":"drive-sata0","read-only":true,"throttle-group":"throttle-drive-sata0"}' \
+  -device 'ide-cd,bus=ahci0.0,drive=drive-sata0,id=sata0,write-cache=on' \
   -machine 'accel=tcg,smm=off,type=pc+pve0' \
   -snapshot
-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server v2 4/5] code cleanup: drive: get rid of outdated drive_is_read_only() helper
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
                   ` (2 preceding siblings ...)
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 3/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
@ 2025-08-12 14:37 ` Fiona Ebner
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 5/5] cfg2cmd: add reminder comments to remove template handling for -drive Fiona Ebner
  2025-08-13  7:34 ` [pve-devel] applied-series: [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fabian Grünbichler
  5 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

The drive_is_read_only() helper only applies to '-drive', but not
'-blockdev' and is only used in a single place. Inline it to avoid
accidental usages popping up in the future.

This also gets rid of a hidden dependency from Drive to QemuConfig.

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d3f0c589..aacdadad 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -67,7 +67,6 @@ use PVE::QemuServer::Drive qw(
     checked_volume_format
     drive_is_cloudinit
     drive_is_cdrom
-    drive_is_read_only
     parse_drive
     print_drive
     storage_allows_io_uring_default
@@ -3778,8 +3777,10 @@ sub config_to_command {
                 my $drive_cmd =
                     print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
 
-                # extra protection for templates, but SATA and IDE don't support it..
-                $drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
+                if ($is_template) {
+                    my $interface = $drive->{interface};
+                    $drive_cmd .= ',readonly=on' if $interface ne 'ide' && $interface ne 'sata';
+                }
 
                 push @$devices, '-drive', $drive_cmd;
             }
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index 9dc4e674..79dd22e6 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -20,7 +20,6 @@ our @EXPORT_OK = qw(
     checked_volume_format
     drive_is_cloudinit
     drive_is_cdrom
-    drive_is_read_only
     parse_drive
     print_drive
     storage_allows_io_uring_default
@@ -751,15 +750,6 @@ sub drive_is_cdrom {
     return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
 }
 
-sub drive_is_read_only {
-    my ($conf, $drive) = @_;
-
-    return 0 if !PVE::QemuConfig->is_template($conf);
-
-    # don't support being marked read-only
-    return $drive->{interface} ne 'sata' && $drive->{interface} ne 'ide';
-}
-
 sub parse_drive_interface {
     my ($key) = @_;
 
-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server v2 5/5] cfg2cmd: add reminder comments to remove template handling for -drive
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
                   ` (3 preceding siblings ...)
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 4/5] code cleanup: drive: get rid of outdated drive_is_read_only() helper Fiona Ebner
@ 2025-08-12 14:37 ` Fiona Ebner
  2025-08-13  7:34 ` [pve-devel] applied-series: [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fabian Grünbichler
  5 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2025-08-12 14:37 UTC (permalink / raw)
  To: pve-devel

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index aacdadad..d40b14de 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3778,6 +3778,9 @@ sub config_to_command {
                     print_drive_commandline_full($storecfg, $vmid, $drive, $live_blockdev_name);
 
                 if ($is_template) {
+                    # TODO PVE 10.x - since the temporary config for starting templates for backup
+                    # uses the latest machine version, this should already be dead code. It's kept
+                    # for now if for whatever reason an older QEMU build is used (e.g. bisecting).
                     my $interface = $drive->{interface};
                     $drive_cmd .= ',readonly=on' if $interface ne 'ide' && $interface ne 'sata';
                 }
@@ -3910,6 +3913,10 @@ sub config_to_command {
     }
 
     if ($is_template) {
+        # TODO PVE 10.x - since the temporary config for starting templates for backup uses the
+        # latest machine version, this should already be dead code. It's kept for now if for
+        # whatever reason an older QEMU build is used (e.g. bisecting).
+
         # needed to workaround base volumes being read-only
         push @$cmd, '-snapshot';
     }
-- 
2.47.2



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] applied-series: [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev
  2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
                   ` (4 preceding siblings ...)
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 5/5] cfg2cmd: add reminder comments to remove template handling for -drive Fiona Ebner
@ 2025-08-13  7:34 ` Fabian Grünbichler
  5 siblings, 0 replies; 8+ messages in thread
From: Fabian Grünbichler @ 2025-08-13  7:34 UTC (permalink / raw)
  To: pve-devel, Fiona Ebner


On Tue, 12 Aug 2025 16:37:38 +0200, Fiona Ebner wrote:
> Changes in v2:
> * new approach, attach as ide-cd rather than using older machine
>   version, which would break qcow2 snapshot-as-volume-chain
> * get rid of now outdated drive_is_read_only() helper
> 
> With ide-hd, the inserted block node needs to be marked as writable
> too, but -blockdev will complain if it's marked as writable but the
> actual backing device is read-only (e.g. read-only base LV).
> 
> [...]

Applied, thanks!

[1/5] ovmf: pass along whether the VM is a template
      commit: 58b929879906a69fdf4e3a9bb2180986881ca1df
[2/5] code cleanup: cfg2cmd: check if configuration is for template centrally
      commit: d45b08004ae2e4ea76e503d183763f083504c723
[3/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev
      commit: 69afe422e53cb0ad0aca74ba5ba20bd02933f878
[4/5] code cleanup: drive: get rid of outdated drive_is_read_only() helper
      commit: bc753d2bc72f103ac39cec9699d58ae71848e316
[5/5] cfg2cmd: add reminder comments to remove template handling for -drive
      commit: 9b1460220c0a8802f3d315073e4062fe38cea137

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template
  2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template Fiona Ebner
@ 2025-08-13  7:37   ` Fabian Grünbichler
  0 siblings, 0 replies; 8+ messages in thread
From: Fabian Grünbichler @ 2025-08-13  7:37 UTC (permalink / raw)
  To: Proxmox VE development discussion

should we (as follow-up) rename the parameter there to `is_readonly`?
that we currently only have the template case where we set it makes the
point a bit moot, but it also seems a bit "leaky" ;)

On August 12, 2025 4:37 pm, Fiona Ebner wrote:
> This is in preparation to remove the hidden dependency from the Drive
> module to QemuConfig.
> 
> Note that the drive_is_read_only() can be replaced with $is_template
> for OVMF, because the helper only behaves differently for IDE and
> SATA, but not for EFI disks.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer.pm      |  3 ++-
>  src/PVE/QemuServer/OVMF.pm | 20 ++++++++++----------
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index cfc54568..d6657a1f 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -3361,8 +3361,9 @@ sub config_to_command {
>              'machine-version' => $machine_version,
>              q35 => $q35,
>          };
> +        my $is_template = PVE::QemuConfig->is_template($conf);
>          my ($ovmf_cmd, $ovmf_machine_flags) = PVE::QemuServer::OVMF::print_ovmf_commandline(
> -            $conf, $storecfg, $vmid, $hw_info, $version_guard,
> +            $conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template,
>          );
>          push $cmd->@*, $ovmf_cmd->@*;
>          push $machineFlags->@*, $ovmf_machine_flags->@*;
> diff --git a/src/PVE/QemuServer/OVMF.pm b/src/PVE/QemuServer/OVMF.pm
> index 8948651c..72f5be0d 100644
> --- a/src/PVE/QemuServer/OVMF.pm
> +++ b/src/PVE/QemuServer/OVMF.pm
> @@ -10,7 +10,7 @@ use PVE::Storage;
>  use PVE::Tools;
>  
>  use PVE::QemuServer::Blockdev;
> -use PVE::QemuServer::Drive qw(checked_volume_format drive_is_read_only parse_drive print_drive);
> +use PVE::QemuServer::Drive qw(checked_volume_format parse_drive print_drive);
>  use PVE::QemuServer::QemuImage;
>  
>  my $EDK2_FW_BASE = '/usr/share/pve-edk2-firmware/';
> @@ -79,7 +79,7 @@ my sub get_ovmf_files($$$$) {
>  }
>  
>  my sub print_ovmf_drive_commandlines {
> -    my ($conf, $storecfg, $vmid, $hw_info, $version_guard) = @_;
> +    my ($conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template) = @_;
>  
>      my ($amd_sev_type, $arch, $q35) = $hw_info->@{qw(amd-sev-type arch q35)};
>  
> @@ -109,7 +109,7 @@ my sub print_ovmf_drive_commandlines {
>  
>          $var_drive_str .= ",size=" . (-s $ovmf_vars)
>              if $format eq 'raw' && $version_guard->(4, 1, 2);
> -        $var_drive_str .= ',readonly=on' if drive_is_read_only($conf, $d);
> +        $var_drive_str .= ',readonly=on' if $is_template;
>      } else {
>          log_warn("no efidisk configured! Using temporary efivars disk.");
>          my $path = "/tmp/$vmid-ovmf.fd";
> @@ -145,7 +145,7 @@ sub create_efidisk($$$$$$$$) {
>  }
>  
>  my sub generate_ovmf_blockdev {
> -    my ($conf, $storecfg, $vmid, $hw_info) = @_;
> +    my ($conf, $storecfg, $vmid, $hw_info, $is_template) = @_;
>  
>      my ($amd_sev_type, $arch, $machine_version, $q35) =
>          $hw_info->@{qw(amd-sev-type arch machine-version q35)};
> @@ -187,8 +187,7 @@ my sub generate_ovmf_blockdev {
>      $drive->{cache} = 'writeback' if !$drive->{cache};
>  
>      my $extra_blockdev_options = {};
> -    # extra protection for templates, but SATA and IDE don't support it..
> -    $extra_blockdev_options->{'read-only'} = 1 if drive_is_read_only($conf, $drive);
> +    $extra_blockdev_options->{'read-only'} = 1 if $is_template;
>  
>      $extra_blockdev_options->{size} = -s $ovmf_vars if $format eq 'raw';
>  
> @@ -202,7 +201,7 @@ my sub generate_ovmf_blockdev {
>  }
>  
>  sub print_ovmf_commandline {
> -    my ($conf, $storecfg, $vmid, $hw_info, $version_guard) = @_;
> +    my ($conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template) = @_;
>  
>      my $amd_sev_type = $hw_info->{'amd-sev-type'};
>  
> @@ -217,7 +216,7 @@ sub print_ovmf_commandline {
>      } else {
>          if ($version_guard->(10, 0, 0)) { # for the switch to -blockdev
>              my ($code_blockdev, $vars_blockdev, $throttle_group) =
> -                generate_ovmf_blockdev($conf, $storecfg, $vmid, $hw_info);
> +                generate_ovmf_blockdev($conf, $storecfg, $vmid, $hw_info, $is_template);
>  
>              push $cmd->@*, '-object', to_json($throttle_group, { canonical => 1 });
>              push $cmd->@*, '-blockdev', to_json($code_blockdev, { canonical => 1 });
> @@ -225,8 +224,9 @@ sub print_ovmf_commandline {
>              push $machine_flags->@*, "pflash0=$code_blockdev->{'node-name'}",
>                  "pflash1=$vars_blockdev->{'node-name'}";
>          } else {
> -            my ($code_drive_str, $var_drive_str) =
> -                print_ovmf_drive_commandlines($conf, $storecfg, $vmid, $hw_info, $version_guard);
> +            my ($code_drive_str, $var_drive_str) = print_ovmf_drive_commandlines(
> +                $conf, $storecfg, $vmid, $hw_info, $version_guard, $is_template,
> +            );
>              push $cmd->@*, '-drive', $code_drive_str;
>              push $cmd->@*, '-drive', $var_drive_str;
>          }
> -- 
> 2.47.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-08-13  7:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-12 14:37 [pve-devel] [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 1/5] ovmf: pass along whether the VM is a template Fiona Ebner
2025-08-13  7:37   ` Fabian Grünbichler
2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 2/5] code cleanup: cfg2cmd: check if configuration is for template centrally Fiona Ebner
2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 3/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fiona Ebner
2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 4/5] code cleanup: drive: get rid of outdated drive_is_read_only() helper Fiona Ebner
2025-08-12 14:37 ` [pve-devel] [PATCH qemu-server v2 5/5] cfg2cmd: add reminder comments to remove template handling for -drive Fiona Ebner
2025-08-13  7:34 ` [pve-devel] applied-series: [PATCH-SERIES qemu-server v2 0/5] fix #6675: template backup: fix regression with IDE/SATA and blockdev Fabian Grünbichler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal