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
