[pve-devel] [PATCH qemu-server 0/6] fix #2862: more template backup fixes

[pve-devel] [PATCH qemu-server 0/6] fix #2862: more template backup fixes

[Fabian Grünbichler]

first one is unrelated, rest should fix the remaining cases where
template backups to PBS fail.

Fabian Grünbichler (6):
  test: unbreak restore_config_test
  drive: factor out read-only helper
  template: mark efidisk as read-only
  test: add template drive read-only tests
  template: add -snapshot to KVM command
  template: start VM for VMA backup

 PVE/QemuServer.pm                             | 18 +++++----
 PVE/QemuServer/Drive.pm                       | 10 +++++
 PVE/VZDump/QemuServer.pm                      | 20 ++--------
 test/cfg2cmd/efi-raw-old.conf.cmd             |  2 +-
 test/cfg2cmd/efi-raw-template.conf            |  5 +++
 test/cfg2cmd/efi-raw-template.conf.cmd        | 28 ++++++++++++++
 test/cfg2cmd/efi-raw.conf.cmd                 |  2 +-
 test/cfg2cmd/i440fx-win10-hostpci.conf.cmd    |  2 +-
 .../q35-linux-hostpci-multifunction.conf.cmd  |  2 +-
 test/cfg2cmd/q35-linux-hostpci.conf.cmd       |  2 +-
 test/cfg2cmd/q35-win10-hostpci.conf.cmd       |  2 +-
 test/cfg2cmd/simple1-template.conf            | 17 +++++++++
 test/cfg2cmd/simple1-template.conf.cmd        | 37 +++++++++++++++++++
 test/run_qemu_restore_config_tests.pl         | 14 +++++++
 14 files changed, 132 insertions(+), 29 deletions(-)
 create mode 100644 test/cfg2cmd/efi-raw-template.conf
 create mode 100644 test/cfg2cmd/efi-raw-template.conf.cmd
 create mode 100644 test/cfg2cmd/simple1-template.conf
 create mode 100644 test/cfg2cmd/simple1-template.conf.cmd

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/6] test: unbreak restore_config_test

[Fabian Grünbichler]

for unprivileged users (and possibly some root setups). reading from
pmxcfs now results in a hard error for unprivileged users, so there
might be some more of these lurking somewhere..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 test/run_qemu_restore_config_tests.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index e5d9f2a..dc10647 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -6,6 +6,7 @@ use warnings;
 use lib qw(..);
 
 use Test::More;
+use Test::MockModule;
 
 use File::Basename;
 
@@ -18,6 +19,19 @@ my $EXPECTED_DIR = './restore-config-expected';
 # NOTE update when you add/remove tests
 plan tests => 4;
 
+my $cfs_mock = Test::MockModule->new("PVE::Cluster");
+$cfs_mock->mock(
+    cfs_read_file => sub {
+	my ($file) = @_;
+
+	if ($file eq 'datacenter.cfg') {
+	    return {};
+	} else {
+	    die "'cfs_read_file' called - missing mock?\n";
+	}
+    },
+);
+
 dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
     my ($file) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/6] drive: factor out read-only helper

[Fabian Grünbichler]

we also need it for efidisks.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/QemuServer.pm       |  8 ++------
 PVE/QemuServer/Drive.pm | 10 ++++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 25ac052..0d49415 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -48,7 +48,7 @@ use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
-use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive);
+use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory;
 use PVE::QemuServer::Monitor qw(mon_cmd);
@@ -3662,11 +3662,7 @@ sub config_to_command {
 	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $pbs_name);
 
 	# extra protection for templates, but SATA and IDE don't support it..
-	my $read_only = PVE::QemuConfig->is_template($conf)
-	    && $drive->{interface} ne 'sata'
-	    && $drive->{interface} ne 'ide';
-
-	$drive_cmd .= ',readonly=on' if $read_only;
+	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
 
 	push @$devices, '-drive',$drive_cmd;
 	push @$devices, '-device', print_drivedevice_full(
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 146a4ab..0408e32 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -12,6 +12,7 @@ our @EXPORT_OK = qw(
 is_valid_drivename
 drive_is_cloudinit
 drive_is_cdrom
+drive_is_read_only
 parse_drive
 print_drive
 );
@@ -422,6 +423,15 @@ 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';
+}
+
 # ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
 #        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
 #        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
-- 
2.30.2

[pve-devel] [PATCH qemu-server 3/6] template: mark efidisk as read-only

[Fabian Grünbichler]

otherwise backups of templates using UEFI fail with storages like LVM
thin, where the volumes are not writable. disk controllers like IDE and
SATA that don't support being read-only are still broken for UEFI.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/QemuServer.pm                                     | 5 ++++-
 test/cfg2cmd/efi-raw-old.conf.cmd                     | 2 +-
 test/cfg2cmd/efi-raw.conf.cmd                         | 2 +-
 test/cfg2cmd/i440fx-win10-hostpci.conf.cmd            | 2 +-
 test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd | 2 +-
 test/cfg2cmd/q35-linux-hostpci.conf.cmd               | 2 +-
 test/cfg2cmd/q35-win10-hostpci.conf.cmd               | 2 +-
 7 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0d49415..3d996af 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3279,6 +3279,7 @@ sub config_to_command {
 	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
 
 	my ($path, $format);
+	my $read_only_str = 'off';
 	if (my $efidisk = $conf->{efidisk0}) {
 	    my $d = parse_drive('efidisk0', $efidisk);
 	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
@@ -3294,6 +3295,8 @@ sub config_to_command {
 		die "efidisk format must be specified\n"
 		    if !defined($format);
 	    }
+
+	    $read_only_str = 'on' if drive_is_read_only($conf, $d);
 	} else {
 	    warn "no efidisk configured! Using temporary efivars disk.\n";
 	    $path = "/tmp/$vmid-ovmf.fd";
@@ -3308,7 +3311,7 @@ sub config_to_command {
 	}
 
 	push @$cmd, '-drive', "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
-	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path";
+	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path,readonly=$read_only_str";
     }
 
     # load q35 config
diff --git a/test/cfg2cmd/efi-raw-old.conf.cmd b/test/cfg2cmd/efi-raw-old.conf.cmd
index 0f8b7c7..622ade3 100644
--- a/test/cfg2cmd/efi-raw-old.conf.cmd
+++ b/test/cfg2cmd/efi-raw-old.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
+  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
   -smp '1,sockets=1,cores=1,maxcpus=1' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
diff --git a/test/cfg2cmd/efi-raw.conf.cmd b/test/cfg2cmd/efi-raw.conf.cmd
index 8af615c..a7a7373 100644
--- a/test/cfg2cmd/efi-raw.conf.cmd
+++ b/test/cfg2cmd/efi-raw.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
+  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
   -smp '1,sockets=1,cores=1,maxcpus=1' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
diff --git a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
index c79576f..1e11710 100644
--- a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
+++ b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
   -smp '2,sockets=2,cores=1,maxcpus=2' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
index 1f9beda..d140501 100644
--- a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
   -smp '2,sockets=2,cores=1,maxcpus=2' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
diff --git a/test/cfg2cmd/q35-linux-hostpci.conf.cmd b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
index dd1bece..7d69134 100644
--- a/test/cfg2cmd/q35-linux-hostpci.conf.cmd
+++ b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
   -smp '2,sockets=2,cores=1,maxcpus=2' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
diff --git a/test/cfg2cmd/q35-win10-hostpci.conf.cmd b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
index 37ef8f7..87a847d 100644
--- a/test/cfg2cmd/q35-win10-hostpci.conf.cmd
+++ b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
@@ -10,7 +10,7 @@
   -daemonize \
   -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
   -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
-  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
   -smp '2,sockets=2,cores=1,maxcpus=2' \
   -nodefaults \
   -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
-- 
2.30.2

[pve-devel] [PATCH qemu-server 4/6] test: add template drive read-only tests

[Fabian Grünbichler]

ensuring the current behaviour:

templates will pass readonly=on to Qemu, except for SATA and IDE drives
which don't support that flag.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 test/cfg2cmd/efi-raw-template.conf     |  5 ++++
 test/cfg2cmd/efi-raw-template.conf.cmd | 27 +++++++++++++++++++
 test/cfg2cmd/simple1-template.conf     | 17 ++++++++++++
 test/cfg2cmd/simple1-template.conf.cmd | 36 ++++++++++++++++++++++++++
 4 files changed, 85 insertions(+)
 create mode 100644 test/cfg2cmd/efi-raw-template.conf
 create mode 100644 test/cfg2cmd/efi-raw-template.conf.cmd
 create mode 100644 test/cfg2cmd/simple1-template.conf
 create mode 100644 test/cfg2cmd/simple1-template.conf.cmd

diff --git a/test/cfg2cmd/efi-raw-template.conf b/test/cfg2cmd/efi-raw-template.conf
new file mode 100644
index 0000000..a181522
--- /dev/null
+++ b/test/cfg2cmd/efi-raw-template.conf
@@ -0,0 +1,5 @@
+# TEST: Test raw efidisk size parameter
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+bios: ovmf
+efidisk0: local:100/base-disk-100-0.raw
+template: 1
diff --git a/test/cfg2cmd/efi-raw-template.conf.cmd b/test/cfg2cmd/efi-raw-template.conf.cmd
new file mode 100644
index 0000000..636f03e
--- /dev/null
+++ b/test/cfg2cmd/efi-raw-template.conf.cmd
@@ -0,0 +1,27 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name vm8006 \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
+  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
+  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/base-disk-100-0.raw,readonly=on' \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/simple1-template.conf b/test/cfg2cmd/simple1-template.conf
new file mode 100644
index 0000000..c26de40
--- /dev/null
+++ b/test/cfg2cmd/simple1-template.conf
@@ -0,0 +1,17 @@
+# TEST: Simple test for a basic template configuration
+# QEMU_VERSION: 3.0
+bootdisk: scsi0
+cores: 3
+ide2: none,media=cdrom
+memory: 768
+name: simple
+net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
+numa: 0
+ostype: l26
+sata0: local:8006/base-8006-disk-0.qcow2,discard=on,size=104858K
+scsi0: local:8006/base-8006-disk-1.qcow2,discard=on,size=104858K
+scsihw: virtio-scsi-pci
+smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
+sockets: 1
+template: 1
+vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
diff --git a/test/cfg2cmd/simple1-template.conf.cmd b/test/cfg2cmd/simple1-template.conf.cmd
new file mode 100644
index 0000000..20a6dcf
--- /dev/null
+++ b/test/cfg2cmd/simple1-template.conf.cmd
@@ -0,0 +1,36 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name simple \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
+  -smp '3,sockets=1,cores=3,maxcpus=3' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 768 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=c773c261-d800-4348-9f5d-167fadd53cf8' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide2,media=cdrom,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=200' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/8006/base-8006-disk-1.qcow2,if=none,id=drive-scsi0,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap,readonly=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -device 'ahci,id=ahci0,multifunction=on,bus=pci.0,addr=0x7'
+  -drive 'file=/var/lib/vz/images/8006/base-8006-disk-0.qcow2,if=none,id=drive-sata0,discard=on,format=qcow2,cache=none,aio=native,detect-zeroes=unmap'
+  -device 'ide-hd,bus=ahci0.0,drive=drive-sata0,id=sata0'
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=A2:C0:43:77:08:A0,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
+  -machine 'type=pc'
-- 
2.30.2

[pve-devel] [PATCH qemu-server 5/6] template: add -snapshot to KVM command

[Fabian Grünbichler]

this allows effectively setting ALL volumes as read-only, even if the
disk controller does not support it. without it, IDE and SATA disks
with (base) volumes which are marked read-only/immutable on the storage
level prevent the template VM from starting for backup purposes.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/QemuServer.pm                      | 5 +++++
 test/cfg2cmd/efi-raw-template.conf.cmd | 3 ++-
 test/cfg2cmd/simple1-template.conf.cmd | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 3d996af..ce392f8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3765,6 +3765,11 @@ sub config_to_command {
 	print "activating and using '$vmstate' as vmstate\n";
     }
 
+    if (PVE::QemuConfig->is_template($conf)) {
+	# needed to workaround base volumes being read-only
+	push @$cmd, '-snapshot';
+    }
+
     # add custom args
     if ($conf->{args}) {
 	my $aa = PVE::Tools::split_args($conf->{args});
diff --git a/test/cfg2cmd/efi-raw-template.conf.cmd b/test/cfg2cmd/efi-raw-template.conf.cmd
index 636f03e..d99862b 100644
--- a/test/cfg2cmd/efi-raw-template.conf.cmd
+++ b/test/cfg2cmd/efi-raw-template.conf.cmd
@@ -24,4 +24,5 @@
   -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
   -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
-  -machine 'type=pc+pve0'
+  -machine 'type=pc+pve0' \
+  -snapshot
diff --git a/test/cfg2cmd/simple1-template.conf.cmd b/test/cfg2cmd/simple1-template.conf.cmd
index 20a6dcf..7fb6fd1 100644
--- a/test/cfg2cmd/simple1-template.conf.cmd
+++ b/test/cfg2cmd/simple1-template.conf.cmd
@@ -33,4 +33,5 @@
   -device 'ide-hd,bus=ahci0.0,drive=drive-sata0,id=sata0'
   -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
   -device 'virtio-net-pci,mac=A2:C0:43:77:08:A0,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
-  -machine 'type=pc'
+  -machine 'type=pc' \
+  -snapshot
-- 
2.30.2

[pve-devel] [RFC qemu-server 6/6] template: start VM for VMA backup

[Fabian Grünbichler]

since using 'vma create ..' no longer works with immutable/read-only
base volumes.

first hunk drops a leftover variable from when we did the same change to
the PBS code path.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
this mimics what we already did for PBS, we still might want to think
about limiting memory for the started VM for template backups which
can't be resumed into operation anyway..

 PVE/VZDump/QemuServer.pm | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 44b705f..9ac77c1 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -446,7 +446,6 @@ sub archive_pbs {
     # proxmox-backup-client can only handle raw files and block devs
     # only use it (directly) for disk-less VMs
     if (!$diskcount) {
-	my @pathlist;
 	$self->loginfo("backup contains no disks");
 
 	local $ENV{PBS_PASSWORD} = $password;
@@ -621,19 +620,8 @@ sub archive_vma {
     }
 
     my $diskcount = scalar(@{$task->{disks}});
-    if (PVE::QemuConfig->is_template($self->{vmlist}->{$vmid}) || !$diskcount) {
-	my @pathlist;
-	foreach my $di (@{$task->{disks}}) {
-	    if ($di->{type} eq 'block' || $di->{type} eq 'file') {
-		push @pathlist, "$di->{qmdevice}=$di->{path}";
-	    } else {
-		die "implement me";
-	    }
-	}
-
-	if (!$diskcount) {
-	    $self->loginfo("backup contains no disks");
-	}
+    if (!$diskcount) {
+	$self->loginfo("backup doesn't contain any disks");
 
 	my $outcmd;
 	if ($comp) {
@@ -646,9 +634,9 @@ sub archive_vma {
 
 	my $cmd = ['/usr/bin/vma', 'create', '-v', '-c', $conffile];
 	push @$cmd, '-c', $firewall if -e $firewall;
-	push @$cmd, $outcmd, @pathlist;
+	push @$cmd, $outcmd;
 
-	$self->loginfo("starting template backup");
+	$self->loginfo("starting backup");
 	$self->loginfo(join(' ', @$cmd));
 
 	if ($opts->{stdout}) {
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 2/6] drive: factor out read-only helper

[Stefan Reiter]

On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
> we also need it for efidisks.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   PVE/QemuServer.pm       |  8 ++------
>   PVE/QemuServer/Drive.pm | 10 ++++++++++
>   2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 25ac052..0d49415 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -48,7 +48,7 @@ use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
>   use PVE::QemuServer::Cloudinit;
>   use PVE::QemuServer::CGroup;
>   use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
> -use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive);
> +use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
>   use PVE::QemuServer::Machine;
>   use PVE::QemuServer::Memory;
>   use PVE::QemuServer::Monitor qw(mon_cmd);
> @@ -3662,11 +3662,7 @@ sub config_to_command {
>   	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $pbs_name);
>   
>   	# extra protection for templates, but SATA and IDE don't support it..
> -	my $read_only = PVE::QemuConfig->is_template($conf)
> -	    && $drive->{interface} ne 'sata'
> -	    && $drive->{interface} ne 'ide';
> -
> -	$drive_cmd .= ',readonly=on' if $read_only;
> +	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
>   
>   	push @$devices, '-drive',$drive_cmd;
>   	push @$devices, '-device', print_drivedevice_full(
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 146a4ab..0408e32 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -12,6 +12,7 @@ our @EXPORT_OK = qw(
>   is_valid_drivename
>   drive_is_cloudinit
>   drive_is_cdrom
> +drive_is_read_only
>   parse_drive
>   print_drive
>   );
> @@ -422,6 +423,15 @@ sub drive_is_cdrom {
>       return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>   }
>   
> +sub drive_is_read_only {

I really don't like this name, this checks if the drive *should* be 
read-only, and only related to template backups, not in general.

Maybe 'drive_template_read_only'?

The function does two pretty unrelated things in general IMO, so maybe 
it would be clearer to do the is_template check at call site and make 
this 'drive_supports_read_only', even if it causes a little bit more 
duplication.

> +    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';
> +}
> +
>   # ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
>   #        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
>   #        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
> 

Re: [pve-devel] [PATCH qemu-server 3/6] template: mark efidisk as read-only

[Stefan Reiter]

High-level: If you make the $read_only_str contain the entire 
",readonly=on" stanza, you can just leave it blank by default and avoid 
updating all the test cases.

On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
> otherwise backups of templates using UEFI fail with storages like LVM
> thin, where the volumes are not writable. disk controllers like IDE and
> SATA that don't support being read-only are still broken for UEFI.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   PVE/QemuServer.pm                                     | 5 ++++-
>   test/cfg2cmd/efi-raw-old.conf.cmd                     | 2 +-
>   test/cfg2cmd/efi-raw.conf.cmd                         | 2 +-
>   test/cfg2cmd/i440fx-win10-hostpci.conf.cmd            | 2 +-
>   test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd | 2 +-
>   test/cfg2cmd/q35-linux-hostpci.conf.cmd               | 2 +-
>   test/cfg2cmd/q35-win10-hostpci.conf.cmd               | 2 +-
>   7 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 0d49415..3d996af 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3279,6 +3279,7 @@ sub config_to_command {
>   	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
>   
>   	my ($path, $format);
> +	my $read_only_str = 'off'; >   	if (my $efidisk = $conf->{efidisk0}) {
>   	    my $d = parse_drive('efidisk0', $efidisk);
>   	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
> @@ -3294,6 +3295,8 @@ sub config_to_command {
>   		die "efidisk format must be specified\n"
>   		    if !defined($format);
>   	    }
> +
> +	    $read_only_str = 'on' if drive_is_read_only($conf, $d);
>   	} else {
>   	    warn "no efidisk configured! Using temporary efivars disk.\n";
>   	    $path = "/tmp/$vmid-ovmf.fd";
> @@ -3308,7 +3311,7 @@ sub config_to_command {
>   	}
>   
>   	push @$cmd, '-drive', "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
> -	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path";
> +	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path,readonly=$read_only_str";
>       }
>   
>       # load q35 config
> diff --git a/test/cfg2cmd/efi-raw-old.conf.cmd b/test/cfg2cmd/efi-raw-old.conf.cmd
> index 0f8b7c7..622ade3 100644
> --- a/test/cfg2cmd/efi-raw-old.conf.cmd
> +++ b/test/cfg2cmd/efi-raw-old.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
> +  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
>     -smp '1,sockets=1,cores=1,maxcpus=1' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> diff --git a/test/cfg2cmd/efi-raw.conf.cmd b/test/cfg2cmd/efi-raw.conf.cmd
> index 8af615c..a7a7373 100644
> --- a/test/cfg2cmd/efi-raw.conf.cmd
> +++ b/test/cfg2cmd/efi-raw.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
> +  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
>     -smp '1,sockets=1,cores=1,maxcpus=1' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> diff --git a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
> index c79576f..1e11710 100644
> --- a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
> +++ b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
> index 1f9beda..d140501 100644
> --- a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
> +++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> diff --git a/test/cfg2cmd/q35-linux-hostpci.conf.cmd b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
> index dd1bece..7d69134 100644
> --- a/test/cfg2cmd/q35-linux-hostpci.conf.cmd
> +++ b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> diff --git a/test/cfg2cmd/q35-win10-hostpci.conf.cmd b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
> index 37ef8f7..87a847d 100644
> --- a/test/cfg2cmd/q35-win10-hostpci.conf.cmd
> +++ b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
> @@ -10,7 +10,7 @@
>     -daemonize \
>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>     -nodefaults \
>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> 

Re: [pve-devel] [RFC qemu-server 6/6] template: start VM for VMA backup

[Stefan Reiter]

On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
> since using 'vma create ..' no longer works with immutable/read-only
> base volumes.

Why that? It shouldn't matter to vma if the base is read-only? Though I 
do see that in vma.c we don't tell QEMU that it should open the block 
backend read-only...

Anyway, I do personally think that just starting a VM is also a good way 
of doing this, though as you say, for templates it would make a lot of 
sense to restrict the VM to use less resources.

> 
> first hunk drops a leftover variable from when we did the same change to
> the PBS code path.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> this mimics what we already did for PBS, we still might want to think
> about limiting memory for the started VM for template backups which
> can't be resumed into operation anyway..
> 
>   PVE/VZDump/QemuServer.pm | 20 ++++----------------
>   1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> index 44b705f..9ac77c1 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -446,7 +446,6 @@ sub archive_pbs {
>       # proxmox-backup-client can only handle raw files and block devs
>       # only use it (directly) for disk-less VMs
>       if (!$diskcount) {
> -	my @pathlist;
>   	$self->loginfo("backup contains no disks");
>   
>   	local $ENV{PBS_PASSWORD} = $password;
> @@ -621,19 +620,8 @@ sub archive_vma {
>       }
>   
>       my $diskcount = scalar(@{$task->{disks}});
> -    if (PVE::QemuConfig->is_template($self->{vmlist}->{$vmid}) || !$diskcount) {
> -	my @pathlist;
> -	foreach my $di (@{$task->{disks}}) {
> -	    if ($di->{type} eq 'block' || $di->{type} eq 'file') {
> -		push @pathlist, "$di->{qmdevice}=$di->{path}";
> -	    } else {
> -		die "implement me";
> -	    }
> -	}
> -
> -	if (!$diskcount) {
> -	    $self->loginfo("backup contains no disks");
> -	}
> +    if (!$diskcount) {
> +	$self->loginfo("backup doesn't contain any disks");

nit: I'd leave the "backup contains no disks" message the same

>   
>   	my $outcmd;
>   	if ($comp) {
> @@ -646,9 +634,9 @@ sub archive_vma {
>   
>   	my $cmd = ['/usr/bin/vma', 'create', '-v', '-c', $conffile];
>   	push @$cmd, '-c', $firewall if -e $firewall;
> -	push @$cmd, $outcmd, @pathlist;
> +	push @$cmd, $outcmd;
>   
> -	$self->loginfo("starting template backup");
> +	$self->loginfo("starting backup");
>   	$self->loginfo(join(' ', @$cmd));
>   
>   	if ($opts->{stdout}) {
> 

Re: [pve-devel] [RFC qemu-server 6/6] template: start VM for VMA backup

[Fabian Grünbichler]

On June 7, 2021 11:29 am, Stefan Reiter wrote:
> On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
>> since using 'vma create ..' no longer works with immutable/read-only
>> base volumes.
> 
> Why that? It shouldn't matter to vma if the base is read-only? Though I 
> do see that in vma.c we don't tell QEMU that it should open the block 
> backend read-only...

I have not checked in-depth on the Qemu side what changed, but 
alternatively we could fix the vma binary to mark as read-only like the 
rest of the series and past patches did for the regular startup for 
backup purposes.

> Anyway, I do personally think that just starting a VM is also a good way 
> of doing this, though as you say, for templates it would make a lot of 
> sense to restrict the VM to use less resources.

yeah, I am on the fence (hence the RFC) - on the one hand, it would be 
more consistent with PBSp-backups to just start the VM, OTOH we'd save 
on memory usage/.. if we fix vma.

> 
>> 
>> first hunk drops a leftover variable from when we did the same change to
>> the PBS code path.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> this mimics what we already did for PBS, we still might want to think
>> about limiting memory for the started VM for template backups which
>> can't be resumed into operation anyway..
>> 
>>   PVE/VZDump/QemuServer.pm | 20 ++++----------------
>>   1 file changed, 4 insertions(+), 16 deletions(-)
>> 
>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>> index 44b705f..9ac77c1 100644
>> --- a/PVE/VZDump/QemuServer.pm
>> +++ b/PVE/VZDump/QemuServer.pm
>> @@ -446,7 +446,6 @@ sub archive_pbs {
>>       # proxmox-backup-client can only handle raw files and block devs
>>       # only use it (directly) for disk-less VMs
>>       if (!$diskcount) {
>> -	my @pathlist;
>>   	$self->loginfo("backup contains no disks");
>>   
>>   	local $ENV{PBS_PASSWORD} = $password;
>> @@ -621,19 +620,8 @@ sub archive_vma {
>>       }
>>   
>>       my $diskcount = scalar(@{$task->{disks}});
>> -    if (PVE::QemuConfig->is_template($self->{vmlist}->{$vmid}) || !$diskcount) {
>> -	my @pathlist;
>> -	foreach my $di (@{$task->{disks}}) {
>> -	    if ($di->{type} eq 'block' || $di->{type} eq 'file') {
>> -		push @pathlist, "$di->{qmdevice}=$di->{path}";
>> -	    } else {
>> -		die "implement me";
>> -	    }
>> -	}
>> -
>> -	if (!$diskcount) {
>> -	    $self->loginfo("backup contains no disks");
>> -	}
>> +    if (!$diskcount) {
>> +	$self->loginfo("backup doesn't contain any disks");
> 
> nit: I'd leave the "backup contains no disks" message the same
> 
>>   
>>   	my $outcmd;
>>   	if ($comp) {
>> @@ -646,9 +634,9 @@ sub archive_vma {
>>   
>>   	my $cmd = ['/usr/bin/vma', 'create', '-v', '-c', $conffile];
>>   	push @$cmd, '-c', $firewall if -e $firewall;
>> -	push @$cmd, $outcmd, @pathlist;
>> +	push @$cmd, $outcmd;
>>   
>> -	$self->loginfo("starting template backup");
>> +	$self->loginfo("starting backup");
>>   	$self->loginfo(join(' ', @$cmd));
>>   
>>   	if ($opts->{stdout}) {
>> 
> 

Re: [pve-devel] [PATCH qemu-server 3/6] template: mark efidisk as read-only

[Fabian Grünbichler]

On June 7, 2021 11:29 am, Stefan Reiter wrote:
> High-level: If you make the $read_only_str contain the entire 
> ",readonly=on" stanza, you can just leave it blank by default and avoid 
> updating all the test cases.

yeah, but once you add a few of those the resulting code gets rather 
messy/ugly. except for a bit of test churn it does not hurt to spell it 
out (and if we want to leave it out, I'd rather factor out the combined 
string and conditionally add to it than have multiple variables that 
might be empty concatenated).

> 
> On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
>> otherwise backups of templates using UEFI fail with storages like LVM
>> thin, where the volumes are not writable. disk controllers like IDE and
>> SATA that don't support being read-only are still broken for UEFI.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>   PVE/QemuServer.pm                                     | 5 ++++-
>>   test/cfg2cmd/efi-raw-old.conf.cmd                     | 2 +-
>>   test/cfg2cmd/efi-raw.conf.cmd                         | 2 +-
>>   test/cfg2cmd/i440fx-win10-hostpci.conf.cmd            | 2 +-
>>   test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd | 2 +-
>>   test/cfg2cmd/q35-linux-hostpci.conf.cmd               | 2 +-
>>   test/cfg2cmd/q35-win10-hostpci.conf.cmd               | 2 +-
>>   7 files changed, 10 insertions(+), 7 deletions(-)
>> 
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 0d49415..3d996af 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -3279,6 +3279,7 @@ sub config_to_command {
>>   	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
>>   
>>   	my ($path, $format);
>> +	my $read_only_str = 'off'; >   	if (my $efidisk = $conf->{efidisk0}) {
>>   	    my $d = parse_drive('efidisk0', $efidisk);
>>   	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
>> @@ -3294,6 +3295,8 @@ sub config_to_command {
>>   		die "efidisk format must be specified\n"
>>   		    if !defined($format);
>>   	    }
>> +
>> +	    $read_only_str = 'on' if drive_is_read_only($conf, $d);
>>   	} else {
>>   	    warn "no efidisk configured! Using temporary efivars disk.\n";
>>   	    $path = "/tmp/$vmid-ovmf.fd";
>> @@ -3308,7 +3311,7 @@ sub config_to_command {
>>   	}
>>   
>>   	push @$cmd, '-drive', "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
>> -	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path";
>> +	push @$cmd, '-drive', "if=pflash,unit=1,format=$format,id=drive-efidisk0$size_str,file=$path,readonly=$read_only_str";
>>       }
>>   
>>       # load q35 config
>> diff --git a/test/cfg2cmd/efi-raw-old.conf.cmd b/test/cfg2cmd/efi-raw-old.conf.cmd
>> index 0f8b7c7..622ade3 100644
>> --- a/test/cfg2cmd/efi-raw-old.conf.cmd
>> +++ b/test/cfg2cmd/efi-raw-old.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
>> +  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
>>     -smp '1,sockets=1,cores=1,maxcpus=1' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> diff --git a/test/cfg2cmd/efi-raw.conf.cmd b/test/cfg2cmd/efi-raw.conf.cmd
>> index 8af615c..a7a7373 100644
>> --- a/test/cfg2cmd/efi-raw.conf.cmd
>> +++ b/test/cfg2cmd/efi-raw.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=7b10d7af-b932-4c66-b2c3-3996152ec465' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw' \
>> +  -drive 'if=pflash,unit=1,format=raw,id=drive-efidisk0,size=131072,file=/var/lib/vz/images/100/vm-disk-100-0.raw,readonly=off' \
>>     -smp '1,sockets=1,cores=1,maxcpus=1' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> diff --git a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
>> index c79576f..1e11710 100644
>> --- a/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
>> +++ b/test/cfg2cmd/i440fx-win10-hostpci.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
>> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>> index 1f9beda..d140501 100644
>> --- a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>> +++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
>> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> diff --git a/test/cfg2cmd/q35-linux-hostpci.conf.cmd b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
>> index dd1bece..7d69134 100644
>> --- a/test/cfg2cmd/q35-linux-hostpci.conf.cmd
>> +++ b/test/cfg2cmd/q35-linux-hostpci.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
>> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> diff --git a/test/cfg2cmd/q35-win10-hostpci.conf.cmd b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
>> index 37ef8f7..87a847d 100644
>> --- a/test/cfg2cmd/q35-win10-hostpci.conf.cmd
>> +++ b/test/cfg2cmd/q35-win10-hostpci.conf.cmd
>> @@ -10,7 +10,7 @@
>>     -daemonize \
>>     -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>>     -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> -  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
>> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2,readonly=off' \
>>     -smp '2,sockets=2,cores=1,maxcpus=2' \
>>     -nodefaults \
>>     -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> 
> 

Re: [pve-devel] [PATCH qemu-server 2/6] drive: factor out read-only helper

[Fabian Grünbichler]

On June 7, 2021 11:29 am, Stefan Reiter wrote:
> On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
>> we also need it for efidisks.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>   PVE/QemuServer.pm       |  8 ++------
>>   PVE/QemuServer/Drive.pm | 10 ++++++++++
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>> 
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 25ac052..0d49415 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -48,7 +48,7 @@ use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
>>   use PVE::QemuServer::Cloudinit;
>>   use PVE::QemuServer::CGroup;
>>   use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
>> -use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive);
>> +use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
>>   use PVE::QemuServer::Machine;
>>   use PVE::QemuServer::Memory;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>> @@ -3662,11 +3662,7 @@ sub config_to_command {
>>   	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $pbs_name);
>>   
>>   	# extra protection for templates, but SATA and IDE don't support it..
>> -	my $read_only = PVE::QemuConfig->is_template($conf)
>> -	    && $drive->{interface} ne 'sata'
>> -	    && $drive->{interface} ne 'ide';
>> -
>> -	$drive_cmd .= ',readonly=on' if $read_only;
>> +	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
>>   
>>   	push @$devices, '-drive',$drive_cmd;
>>   	push @$devices, '-device', print_drivedevice_full(
>> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
>> index 146a4ab..0408e32 100644
>> --- a/PVE/QemuServer/Drive.pm
>> +++ b/PVE/QemuServer/Drive.pm
>> @@ -12,6 +12,7 @@ our @EXPORT_OK = qw(
>>   is_valid_drivename
>>   drive_is_cloudinit
>>   drive_is_cdrom
>> +drive_is_read_only
>>   parse_drive
>>   print_drive
>>   );
>> @@ -422,6 +423,15 @@ sub drive_is_cdrom {
>>       return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>>   }
>>   
>> +sub drive_is_read_only {
> 
> I really don't like this name, this checks if the drive *should* be 
> read-only, and only related to template backups, not in general.

yeah, `drive_should_be_read_only` would be more apt, but also sounds 
wrong. I did have the non-template case in mind as well (e.g., adding a 
'ro' flag to the drive in our VM config as a future addon, like we have 
for container mountpoints).

> 
> Maybe 'drive_template_read_only'?
> 
> The function does two pretty unrelated things in general IMO, so maybe 
> it would be clearer to do the is_template check at call site and make 
> this 'drive_supports_read_only', even if it causes a little bit more 
> duplication.

would work as well. or we drop all of it and no longer mark any drives 
as read-only, if we use the patch that adds '-snapshot' for 
'start-template-for-backup'? at the risk of re-doing it if we ever add a 
'ro' property for individual regular disks/drives..

> 
>> +    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';
>> +}
>> +
>>   # ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
>>   #        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
>>   #        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
>> 
> 

Re: [pve-devel] [PATCH qemu-server 2/6] drive: factor out read-only helper

[Stefan Reiter]

On 6/7/21 12:23 PM, Fabian Grünbichler wrote:
> On June 7, 2021 11:29 am, Stefan Reiter wrote:
>> On 6/4/21 11:47 AM, Fabian Grünbichler wrote:
>>> we also need it for efidisks.
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>>    PVE/QemuServer.pm       |  8 ++------
>>>    PVE/QemuServer/Drive.pm | 10 ++++++++++
>>>    2 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index 25ac052..0d49415 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -48,7 +48,7 @@ use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
>>>    use PVE::QemuServer::Cloudinit;
>>>    use PVE::QemuServer::CGroup;
>>>    use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
>>> -use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive);
>>> +use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
>>>    use PVE::QemuServer::Machine;
>>>    use PVE::QemuServer::Memory;
>>>    use PVE::QemuServer::Monitor qw(mon_cmd);
>>> @@ -3662,11 +3662,7 @@ sub config_to_command {
>>>    	my $drive_cmd = print_drive_commandline_full($storecfg, $vmid, $drive, $pbs_name);
>>>    
>>>    	# extra protection for templates, but SATA and IDE don't support it..
>>> -	my $read_only = PVE::QemuConfig->is_template($conf)
>>> -	    && $drive->{interface} ne 'sata'
>>> -	    && $drive->{interface} ne 'ide';
>>> -
>>> -	$drive_cmd .= ',readonly=on' if $read_only;
>>> +	$drive_cmd .= ',readonly=on' if drive_is_read_only($conf, $drive);
>>>    
>>>    	push @$devices, '-drive',$drive_cmd;
>>>    	push @$devices, '-device', print_drivedevice_full(
>>> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
>>> index 146a4ab..0408e32 100644
>>> --- a/PVE/QemuServer/Drive.pm
>>> +++ b/PVE/QemuServer/Drive.pm
>>> @@ -12,6 +12,7 @@ our @EXPORT_OK = qw(
>>>    is_valid_drivename
>>>    drive_is_cloudinit
>>>    drive_is_cdrom
>>> +drive_is_read_only
>>>    parse_drive
>>>    print_drive
>>>    );
>>> @@ -422,6 +423,15 @@ sub drive_is_cdrom {
>>>        return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>>>    }
>>>    
>>> +sub drive_is_read_only {
>>
>> I really don't like this name, this checks if the drive *should* be
>> read-only, and only related to template backups, not in general.
> 
> yeah, `drive_should_be_read_only` would be more apt, but also sounds
> wrong. I did have the non-template case in mind as well (e.g., adding a
> 'ro' flag to the drive in our VM config as a future addon, like we have
> for container mountpoints).
> 

Yes, I actually assumed we could already do that before I looked more 
closely when reviewing your patches :)

>>
>> Maybe 'drive_template_read_only'?
>>
>> The function does two pretty unrelated things in general IMO, so maybe
>> it would be clearer to do the is_template check at call site and make
>> this 'drive_supports_read_only', even if it causes a little bit more
>> duplication.
> 
> would work as well. or we drop all of it and no longer mark any drives
> as read-only, if we use the patch that adds '-snapshot' for
> 'start-template-for-backup'? at the risk of re-doing it if we ever add a
> 'ro' property for individual regular disks/drives..
> 

Hm, I do like the idea of marking them read-only if possible, even if we 
pass '-snapshot' - all of this is just preventative anyway, as the guest 
is stopped and should never write anything, so at this point might as 
well make it in-depth if it's cheap like here. And potentially reuseable 
for a ro flag.

>>
>>> +    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';
>>> +}
>>> +
>>>    # ideX = [volume=]volume-id[,media=d][,cyls=c,heads=h,secs=s[,trans=t]]
>>>    #        [,snapshot=on|off][,cache=on|off][,format=f][,backup=yes|no]
>>>    #        [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]
>>>
>>

[pve-devel] partially-applied: [PATCH qemu-server 0/6] fix #2862: more template backup fixes

[Thomas Lamprecht]

On 04.06.21 11:47, Fabian Grünbichler wrote:
> first one is unrelated, rest should fix the remaining cases where
> template backups to PBS fail.
> 
> Fabian Grünbichler (6):
>   test: unbreak restore_config_test
>   drive: factor out read-only helper
>   template: mark efidisk as read-only
>   test: add template drive read-only tests
>   template: add -snapshot to KVM command

applied above, i.e., all but the RFC below. I adapted the EFI-disk one to what
Stefan proposed, i.e., just drop the readonly if off. There also was the need to
solve a merge conflict, so I just squashed it all into that one.

>   template: start VM for VMA backup