On Fri, Apr 08, 2022 at 10:48:45 -0700, Rohit Kumar wrote:
> This patch introduces the logic to support remote NVRAM and
> adds 'type' attribute to nvram element.
>
> Sample XML with new annotation:
>
> <nvram type='network'>
> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'>
> <host name='example.com' port='6000'/>
> </source>
> </nvram>
>
> or
>
> <nvram type='file'>
> <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/>
> </nvram>
[1]
>
> Signed-off-by: Prerna Saxena <prerna.saxena@xxxxxxxxxxx>
> Signed-off-by: Florian Schmidt <flosch@xxxxxxxxxxx>
> Signed-off-by: Rohit Kumar <rohit.kumar3@xxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++-------
> src/qemu/qemu_firmware.c | 6 +++
> 2 files changed, 74 insertions(+), 13 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b83c2f0e6a..bc8c7e0d6f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17866,6 +17866,42 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> }
>
>
> +static int
> +virDomainNvramDefParseXML(virStorageSource *nvram,
> + xmlXPathContextPtr ctxt,
> + virDomainXMLOption *opt,
> + unsigned int flags)
> +{
> + g_autofree char *nvramType = NULL;
> + xmlNodePtr source;
> +
> + nvramType = virXPathString("string(./os/nvram/@type)", ctxt);
> +
> + /* if nvramType==NULL read old-style, else
> + * if there's a type, read new style */
> + if (!nvramType) {
> + nvram->type = VIR_STORAGE_TYPE_FILE;
> + nvram->path = virXPathString("string(./os/nvram[1])", ctxt);
> + return 0;
Note that this code path doesn't remember that the old-style config was
used.
> + } else {
'source' can be declared here.
> + if ((nvram->type = virStorageTypeFromString(nvramType)) <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown disk type '%s'"), nvramType);
> + return -1;
> + }
> + source = virXPathNode("./os/nvram/source[1]", ctxt);
> + if (!source) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Missing source element with nvram type '%s'"),
> + nvramType);
> + return -1;
> + }
You'll also need to add a form of validation either in a separate commit
prior to this commit or into this commit which will reject unsupported
source types, such as VIR_STORAGE_TYPE_DIR, or VIR_STORAGE_TYPE_VOLUME
(and others) ... even VIR_STORAGE_TYPE_NETWORK at this point.
> + return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt);
> + }
> + return 0;
> +}
> +
> +
> static int
> virDomainSchedulerParseCommonAttrs(xmlNodePtr node,
> virProcessSchedPolicy *policy,
[...]
> -static void
> +static int
> virDomainLoaderDefFormat(virBuffer *buf,
> - virDomainLoaderDef *loader)
> + virDomainLoaderDef *loader,
> + virDomainXMLOption *xmlopt,
> + unsigned int flags)
> {
> g_auto(virBuffer) loaderAttrBuf = VIR_BUFFER_INITIALIZER;
> g_auto(virBuffer) loaderChildBuf = VIR_BUFFER_INITIALIZER;
> @@ -26976,10 +27020,20 @@ virDomainLoaderDefFormat(virBuffer *buf,
>
> virBufferEscapeString(&nvramAttrBuf, " template='%s'", loader->nvramTemplate);
> if (loader->nvram) {
> - if (loader->nvram->type == VIR_STORAGE_TYPE_FILE)
> + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) {
Since you don't remember that the old-style config was used and you
force it for all VIR_STORAGE_TYPE_FILE configs the example [1] you added
in the commit message will work but will be reformatted.
> virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path);
> + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
> + } else {
> + virBufferEscapeString(&nvramAttrBuf, " type='%s'", virStorageTypeToString(loader->nvram->type));
Use virBufferAsprintf as we are not dealing with user-supplied data.
> + if (virDomainDiskSourceFormat(&nvramChildBuf, loader->nvram, "source", 0,
> + 0, false, flags, true, xmlopt) < 0) {
> + return -1;
> + }
> + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, true);
Use the 'virXMLFormatElement' instead, or add a boolean for the last
argument ...
> + }
> }
> - virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
And use just one invocation.
> +
> + return 0;
> }
>
> static void
[...]
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 6556a613a8..22ad7d42a1 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -1375,6 +1375,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
> && virFileExists(def->os.loader->nvram->path))
> return 0;
>
> +
> + if (!reset_nvram && def->os.loader->nvram &&
> + def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK &&
> + def->os.loader->nvram->path)
> + return 0;
This bit doesn't seem to belong to this patch.
