On 2/15/22 19:54, Daniel P. Berrangé wrote:
> When using <os firmware='...'/> we still parse the <nvram> path,
> but completely ignore it, replacing any user provided content with
> a custom generated path. This makes sense since when undefining the
> guest, the code to cleanup NVRAM also uses the same generated path.
>
> Instead of silently ignoring user config, we should report an
> explicit error message. This shows that some of our tests had the
> bogus config scenario present.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 8 +++
> tests/qemuxml2argvdata/os-firmware-bios.xml | 1 -
> .../os-firmware-efi-bad-nvram-path.err | 1 +
> .../os-firmware-efi-bad-nvram-path.xml | 68 +++++++++++++++++++
> .../os-firmware-efi-secboot.xml | 1 -
> tests/qemuxml2argvdata/os-firmware-efi.xml | 1 -
> tests/qemuxml2argvtest.c | 1 +
> .../os-firmware-bios.x86_64-latest.xml | 1 -
> .../os-firmware-efi-secboot.x86_64-latest.xml | 1 -
> .../os-firmware-efi.x86_64-latest.xml | 1 -
> 10 files changed, 78 insertions(+), 6 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err
> create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 31b49c4ec9..946a80c239 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4827,6 +4827,14 @@ virDomainDefPostParseOs(virDomainDef *def)
> return -1;
> }
>
> + if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
> + if (def->os.loader->nvram) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("NVRAM path is not permitted with firmware attribute"));
> + return -1;
> + }
> + }
> +
> return 0;
Again, virDomainDefOSValidate(), ...
> }
>
> diff --git a/tests/qemuxml2argvdata/os-firmware-bios.xml b/tests/qemuxml2argvdata/os-firmware-bios.xml
> index 63886666dd..d89fcb6c58 100644
> --- a/tests/qemuxml2argvdata/os-firmware-bios.xml
> +++ b/tests/qemuxml2argvdata/os-firmware-bios.xml
> @@ -7,7 +7,6 @@
> <os firmware='bios'>
> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
> <loader secure='no'/>
> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram>
> <boot dev='hd'/>
> <bootmenu enable='yes'/>
> </os>
> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err
> new file mode 100644
> index 0000000000..2ba8135ad4
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err
> @@ -0,0 +1 @@
> +XML error: NVRAM path is not permitted with firmware attribute
... which will have unfortunate result that this error message will look
different, because one of earlier validators reports an error.
> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml
> new file mode 100644
> index 0000000000..a4afdb6d0b
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml
> @@ -0,0 +1,68 @@
> +<domain type='kvm'>
> + <name>fedora</name>
> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
> + <memory unit='KiB'>8192</memory>
> + <currentMemory unit='KiB'>8192</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os firmware='efi'>
> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
> + <loader secure='no'/>
> + <nvram>/some/path</nvram>
> + <boot dev='hd'/>
> + <bootmenu enable='yes'/>
> + </os>
> + <features>
> + <acpi/>
> + <apic/>
> + <pae/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> + <pm>
> + <suspend-to-mem enabled='yes'/>
> + <suspend-to-disk enabled='no'/>
> + </pm>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <controller type='usb' index='0' model='ich9-ehci1'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/>
> + </controller>
> + <controller type='usb' index='0' model='ich9-uhci1'>
> + <master startport='0'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/>
> + </controller>
> + <controller type='usb' index='0' model='ich9-uhci2'>
> + <master startport='2'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/>
> + </controller>
> + <controller type='usb' index='0' model='ich9-uhci3'>
> + <master startport='4'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/>
> + </controller>
> + <controller type='sata' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
> + </controller>
> + <controller type='pci' index='0' model='pcie-root'/>
> + <controller type='pci' index='1' model='dmi-to-pci-bridge'>
> + <model name='i82801b11-bridge'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model name='pci-bridge'/>
> + <target chassisNr='2'/>
> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
> + </controller>
> + <controller type='pci' index='3' model='pcie-root-port'>
> + <model name='ioh3420'/>
> + <target chassis='3' port='0x8'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
> + </memballoon>
Nit pick, I've gotten so far that machine type is plain 'q35' and
removed all these devices. They are not needed since this test is bound
to fail anyway.
> + </devices>
> +</domain>
Michal
