On 21/04/22 8:20 pm, Peter Krempa wrote:
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.
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
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 ?
