Re: [PATCH v2 2/8] Add support to parse/format virStorageSource type NVRAM

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(a)nutanix.com> Signed-off-by: Florian Schmidt <flosch(a)nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com> --- > 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.