* [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.