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@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@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. Right. + } else { 'source' can be declared here. Ack.
+ 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. Yes, sure. I will add the validation in next patch.
+ 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. Right, should I add a boolean variable in virDomainLoaderDef to remember
On 21/04/22 8:20 pm, Peter Krempa wrote: the new style and format in the new style ? Or formatting always in the old style in case of VIR_STORAGE_TYPE_FILE is okay ?
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. Ack.
+ 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 ... Ack.
+ } } - virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false); And use just one invocation. Ack.
+ + 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. This is ignoring firmware autoselection feature in case if NVRAM image is there. Shouldn't this be here in this patch ?