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>
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;
+ } else {
+ 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;
+ }
+ return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt);
+ }
+ return 0;
+}
+
+
static int
virDomainSchedulerParseCommonAttrs(xmlNodePtr node,
virProcessSchedPolicy *policy,
@@ -18251,7 +18287,9 @@ virDomainDefParseBootFirmwareOptions(virDomainDef *def,
static int
virDomainDefParseBootLoaderOptions(virDomainDef *def,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ virDomainXMLOption *xmlopt,
+ unsigned int flags)
{
xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt);
const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE;
@@ -18268,8 +18306,10 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def,
if (virXPathNode("./os/nvram[1]", ctxt)) {
def->os.loader->nvram = g_new0(virStorageSource, 1);
- def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt);
- def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE;
+
+ if (virDomainNvramDefParseXML(def->os.loader->nvram,
+ ctxt, xmlopt, flags) < 0)
+ return -1;
}
if (!fwAutoSelect)
def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt);
@@ -18324,7 +18364,9 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def,
static int
virDomainDefParseBootOptions(virDomainDef *def,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ virDomainXMLOption *xmlopt,
+ unsigned int flags)
{
/*
* Booting options for different OS types....
@@ -18342,7 +18384,7 @@ virDomainDefParseBootOptions(virDomainDef *def,
if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0)
return -1;
- if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0)
+ if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0)
return -1;
if (virDomainDefParseBootAcpiOptions(def, ctxt) < 0)
@@ -18358,7 +18400,7 @@ virDomainDefParseBootOptions(virDomainDef *def,
case VIR_DOMAIN_OSTYPE_UML:
virDomainDefParseBootKernelOptions(def, ctxt);
- if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0)
+ if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0)
return -1;
break;
@@ -19649,7 +19691,7 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
if (virDomainDefClockParse(def, ctxt) < 0)
return NULL;
- if (virDomainDefParseBootOptions(def, ctxt) < 0)
+ if (virDomainDefParseBootOptions(def, ctxt, xmlopt, flags) < 0)
return NULL;
/* analysis of the disk devices */
@@ -26949,9 +26991,11 @@ virDomainHugepagesFormat(virBuffer *buf,
virBufferAddLit(buf, "</hugepages>\n");
}
-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) {
virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path);
+ virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
+ } else {
+ virBufferEscapeString(&nvramAttrBuf, " type='%s'", virStorageTypeToString(loader->nvram->type));
+ if (virDomainDiskSourceFormat(&nvramChildBuf, loader->nvram, "source", 0,
+ 0, false, flags, true, xmlopt) < 0) {
+ return -1;
+ }
+ virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, true);
+ }
}
- virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
+
+ return 0;
}
static void
@@ -28180,8 +28234,9 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
if (def->os.initgroup)
virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
- if (def->os.loader)
- virDomainLoaderDefFormat(buf, def->os.loader);
+ if (def->os.loader &&
+ virDomainLoaderDefFormat(buf, def->os.loader, xmlopt, flags) < 0)
+ return -1;
virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
def->os.kernel);
virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
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;
+
/* ... then we want to consult JSON FW descriptors first,
* but we don't want to fail if we haven't found a match. */
needResult = false;
--
2.25.1On 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.
> + } 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.
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@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
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 ?
>
© 2016 - 2026 Red Hat, Inc.