all lists on 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal