Apologies for this feedback coming very late - not just post-merge
but also extremely close to release.
On Fri, Jul 22, 2022 at 05:23:16PM +0100, Daniel P. Berrangé wrote:
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 3ea094e64c..4199abfd1a 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -242,7 +242,11 @@ harddisk, cdrom, network) determining where to obtain/find the boot
image.
firmwares may implement the Secure boot feature. Attribute ``secure`` can be
used to tell the hypervisor that the firmware is capable of Secure Boot feature.
It cannot be used to enable or disable the feature itself in the firmware.
- :since:`Since 2.1.0`
+ :since:`Since 2.1.0`. If the loader is marked as read-only, then with UEFI it
+ is assumed that there will be a writable NVRAM available. In some cases,
+ however, it may be desirable for the loader to run without any NVRAM, discarding
+ any config changes on shutdown. The ``stateless`` flag can be used to control
+ this behaviour, when set to ``no`` NVRAM will never be created.
Isn't the actual behavior the opposite of what you're describing
here? That is, stateless=yes is what causes the NVRAM file to not be
created.
+++ b/src/conf/domain_validate.c
@@ -1672,6 +1672,32 @@ virDomainDefOSValidate(const virDomainDef *def,
}
}
+ if (loader->stateless == VIR_TRISTATE_BOOL_YES) {
+ if (loader->nvramTemplate) {
+ virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("NVRAM template is not permitted when loader is
stateless"));
+ return -1;
+ }
+
+ if (loader->nvram) {
+ virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("NVRAM is not permitted when loader is
stateless"));
+ return -1;
+ }
+ } else if (loader->stateless == VIR_TRISTATE_BOOL_NO) {
+ if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
+ if (def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) {
+ virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("Only pflash loader type permits NVRAM"));
+ return -1;
+ }
+ } else if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
+ virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("Only EFI firmware permits NVRAM"));
+ return -1;
+ }
These last two error messages could be improved IMO.
Consider the firmware-auto-bios-not-stateless test case, where the
input configuration looks like
<os firmware='bios'>
<type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
<loader stateless='no'/>
</os>
In this case, printing out
Only EFI firmware permits NVRAM
is a bit confusing, since the user has not directly mentioned NVRAM
anywhere. Something along the lines of
virReportError(VIR_ERR_XML_DETAIL,
_("Firmware type '%s' only supports stateless
operations"),
virDomainOsDefFirmwareTypeToString(def->os.firmware));
would be more understandable and actionable, I think.
--
Andrea Bolognani / Red Hat / Virtualization