From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Stefan Reiter <s.reiter@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 3/6] template: mark efidisk as read-only
Date: Mon, 07 Jun 2021 12:23:00 +0200 [thread overview]
Message-ID: <1623061033.usnvayvh3c.astroid@nora.none> (raw)
In-Reply-To: <c7515804-fefe-232f-d501-47e6ea24eb9e@proxmox.com>
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' \
>>
>
next prev parent reply other threads:[~2021-06-07 10:23 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-04 9:47 [pve-devel] [PATCH qemu-server 0/6] fix #2862: more template backup fixes Fabian Grünbichler
2021-06-04 9:47 ` [pve-devel] [PATCH qemu-server 1/6] test: unbreak restore_config_test Fabian Grünbichler
2021-06-04 9:47 ` [pve-devel] [PATCH qemu-server 2/6] drive: factor out read-only helper Fabian Grünbichler
2021-06-07 9:29 ` Stefan Reiter
2021-06-07 10:23 ` Fabian Grünbichler
2021-06-07 10:35 ` Stefan Reiter
2021-06-04 9:47 ` [pve-devel] [PATCH qemu-server 3/6] template: mark efidisk as read-only Fabian Grünbichler
2021-06-07 9:29 ` Stefan Reiter
2021-06-07 10:23 ` Fabian Grünbichler [this message]
2021-06-04 9:47 ` [pve-devel] [PATCH qemu-server 4/6] test: add template drive read-only tests Fabian Grünbichler
2021-06-04 9:47 ` [pve-devel] [PATCH qemu-server 5/6] template: add -snapshot to KVM command Fabian Grünbichler
2021-06-04 9:47 ` [pve-devel] [RFC qemu-server 6/6] template: start VM for VMA backup Fabian Grünbichler
2021-06-07 9:29 ` Stefan Reiter
2021-06-07 10:22 ` Fabian Grünbichler
2021-06-23 10:50 ` [pve-devel] partially-applied: [PATCH qemu-server 0/6] fix #2862: more template backup fixes Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1623061033.usnvayvh3c.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=s.reiter@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox