On Sun, Mar 20, 2022 at 21:28:13 -0700, Rohit Kumar wrote:
> Libvirt domain XML allows only local filepaths to specify a NVRAM
> element. Since VMs can move across different hosts, it should be
> possibe to allocate NVRAM disks on network storage for uninturrupted
> access.
>
> This Patch extends the NVRAM disk elements to be described as
> virStorageSource* elements.
>
> Sample XML with new annotation:
>
> <nvram>
This should have a 'type' attribute rather than blindly checking the
fields.
> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'>
> <host name='example.com' port='6000'/>
> </source>
> </nvram>
>
> or
>
> <nvram>
> <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/>
> </nvram>
>
> Signed-off-by: Prerna Saxena <prerna.saxena@xxxxxxxxxxx>
> Signed-off-by: Florian Schmidt <flosch@xxxxxxxxxxx>
> Signed-off-by: Rohit Kumar <rohit.kumar3@xxxxxxxxxxx>
> ---
> docs/schemas/domaincommon.rng | 72 +++++++++------
> src/conf/domain_conf.c | 90 +++++++++++++++----
> src/conf/domain_conf.h | 2 +-
> src/qemu/qemu_cgroup.c | 3 +-
> src/qemu/qemu_command.c | 5 +-
> src/qemu/qemu_domain.c | 23 +++--
> src/qemu/qemu_driver.c | 5 +-
> src/qemu/qemu_firmware.c | 17 ++--
> src/qemu/qemu_namespace.c | 5 +-
> src/qemu/qemu_process.c | 5 +-
> src/security/security_dac.c | 6 +-
> src/security/security_selinux.c | 6 +-
> src/security/virt-aa-helper.c | 5 +-
> src/vbox/vbox_common.c | 8 +-
> .../qemuxml2argvdata/bios-nvram-network.args | 35 ++++++++
> tests/qemuxml2argvdata/bios-nvram-network.xml | 40 +++++++++
> tests/qemuxml2argvtest.c | 1 +
So this patch is doing too much. You'll have to split it. I'll note how
below.
You are completely missing documentation for the new syntax in
docs/formatdomain.rst.
> 17 files changed, 256 insertions(+), 72 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
> create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 9c1b64a644..a25c84a0b7 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -330,7 +330,17 @@
> </attribute>
> </optional>
> <optional>
> - <ref name="absFilePath"/>
> + <choice>
> + <group>
> + <ref name="absFilePath"/>
> + </group>
> + <group>
> + <ref name="diskSourceFileElement"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkElement"/>
> + </group>
> + </choice>
> </optional>
> </element>
> </optional>
> @@ -1714,6 +1724,31 @@
> </choice>
> </define>
>
> + <define name="diskSourceFileElement">
> + <element name="source">
> + <interleave>
> + <optional>
> + <attribute name="file">
> + <choice>
> + <ref name="absFilePath"/>
> + <ref name="vmwarePath"/>
> + </choice>
> + </attribute>
> + </optional>
> + <ref name="diskSourceCommon"/>
> + <optional>
> + <ref name="storageStartupPolicy"/>
> + </optional>
> + <optional>
> + <ref name="encryption"/>
> + </optional>
> + <zeroOrMore>
> + <ref name="devSeclabel"/>
> + </zeroOrMore>
> + </interleave>
> + </element>
> + </define>
> +
> <define name="diskSourceFile">
> <optional>
> <attribute name="type">
> @@ -1721,28 +1756,7 @@
> </attribute>
> </optional>
> <optional>
> - <element name="source">
> - <interleave>
> - <optional>
> - <attribute name="file">
> - <choice>
> - <ref name="absFilePath"/>
> - <ref name="vmwarePath"/>
> - </choice>
> - </attribute>
> - </optional>
> - <ref name="diskSourceCommon"/>
> - <optional>
> - <ref name="storageStartupPolicy"/>
> - </optional>
> - <optional>
> - <ref name="encryption"/>
> - </optional>
> - <zeroOrMore>
> - <ref name="devSeclabel"/>
> - </zeroOrMore>
> - </interleave>
> - </element>
> + <ref name="diskSourceFileElement"/>
> </optional>
> </define>
>
> @@ -2137,10 +2151,7 @@
> </element>
> </define>
>
> - <define name="diskSourceNetwork">
> - <attribute name="type">
> - <value>network</value>
> - </attribute>
> + <define name="diskSourceNetworkElement">
> <choice>
> <ref name="diskSourceNetworkProtocolNBD"/>
> <ref name="diskSourceNetworkProtocolGluster"/>
> @@ -2155,6 +2166,13 @@
> </choice>
> </define>
>
> + <define name="diskSourceNetwork">
> + <attribute name="type">
> + <value>network</value>
> + </attribute>
> + <ref name="diskSourceNetworkElement"/>
> + </define>
> +
> <define name="diskSourceVolume">
> <attribute name="type">
> <value>volume</value>
Please separate the cleanups (moving of definitions under the 'define'
element into a separate patch.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e0dfc9e45f..5fcc09db05 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3535,7 +3535,7 @@ virDomainLoaderDefFree(virDomainLoaderDef *loader)
> return;
>
> g_free(loader->path);
> - g_free(loader->nvram);
> + virObjectUnref(loader->nvram);
> g_free(loader->nvramTemplate);
> g_free(loader);
> }
> @@ -17834,6 +17834,37 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> }
>
>
> +static int
> +virDomainNvramDefParseXML(virStorageSource *nvram,
> + xmlXPathContextPtr ctxt,
> + virDomainXMLOption *opt,
> + unsigned int flags)
> +{
> + g_autofree char *srcTypeFile = NULL;
> + g_autofree char *srcTypeNetwork = NULL;
> + xmlNodePtr source;
> +
> + srcTypeFile = virXPathString("string(./os/nvram/source/@file)", ctxt);
> + srcTypeNetwork = virXPathString("string(./os/nvram/source/@protocol)", ctxt);
> +
> + if (!srcTypeFile && !srcTypeNetwork) {
> + nvram->type = VIR_STORAGE_TYPE_FILE;
> + nvram->path = virXPathString("string(./os/nvram[1])", ctxt);
> + return 0;
> + } else {
> + if (srcTypeFile) {
> + nvram->type = VIR_STORAGE_TYPE_FILE;
> + } else {
> + nvram->type = VIR_STORAGE_TYPE_NETWORK;
> + }
> + source = virXPathNode("./os/nvram/source[1]", ctxt);
> + if (!source)
> + return -1;
virXPathNode does not report an error but you return it, thus the code
would not report anything.
> + return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt);
> + }
> +
> +}
> +
> static int
> virDomainSchedulerParseCommonAttrs(xmlNodePtr node,
> virProcessSchedPolicy *policy,
> @@ -18219,7 +18250,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;
> @@ -18234,7 +18267,14 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def,
> fwAutoSelect) < 0)
> return -1;
>
> - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
> + if (virXPathNode("./os/nvram[1]", ctxt)) {
> + def->os.loader->nvram = g_new0(virStorageSource, 1);
> +
> + 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);
>
> @@ -18288,7 +18328,9 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def,
>
> static int
> virDomainDefParseBootOptions(virDomainDef *def,
> - xmlXPathContextPtr ctxt)
> + xmlXPathContextPtr ctxt,
> + virDomainXMLOption *xmlopt,
> + unsigned int flags)
> {
> /*
> * Booting options for different OS types....
> @@ -18306,7 +18348,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)
> @@ -18322,7 +18364,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;
> @@ -19606,7 +19648,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 */
> @@ -26899,7 +26941,9 @@ virDomainHugepagesFormat(virBuffer *buf,
>
> static void
> virDomainLoaderDefFormat(virBuffer *buf,
> - virDomainLoaderDef *loader)
> + virDomainLoaderDef *loader,
> + virDomainXMLOption *xmlopt,
> + unsigned int flags)
> {
> const char *readonly = virTristateBoolTypeToString(loader->readonly);
> const char *secure = virTristateBoolTypeToString(loader->secure);
> @@ -26921,13 +26965,27 @@ virDomainLoaderDefFormat(virBuffer *buf,
> else
> virBufferAddLit(buf, "/>\n");
>
> - if (loader->nvram || loader->nvramTemplate) {
> - virBufferAddLit(buf, "<nvram");
> - virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate);
> - if (loader->nvram)
> - virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram);
> - else
> - virBufferAddLit(buf, "/>\n");
> + if (loader->nvram) {
> + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) {
> + virBufferAddLit(buf, "<nvram");
> + virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate);
> + if (loader->nvram->path)
> + virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram->path);
> + else
> + virBufferAddLit(buf, "/>\n");
> + } else {
> + virBufferAddLit(buf, "<nvram");
> + virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate);
> + virBufferAdjustIndent(buf, 2);
> + if (virDomainDiskSourceFormat(buf, loader->nvram, "source", 0,
> + 0, false, flags, true, xmlopt) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot format NVRAM source"));
'virDomainDiskSourceFormat' is already reporting an error so you must
not overwrite it ...
> + return;
... also you must propagate it appropriately.
> + }
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</nvram>\n");
I'll post a patch refactoring this function to use virXMLFormatElement
which will greatly clean up your addition. Make sure to rebase it on top
of it.
> + }
> }
> }
>
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c836799888..649a6f5b55 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9583,6 +9583,7 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd,
> virQEMUCaps *qemuCaps)
> {
> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> + g_autofree char *nvramPath = NULL;
> int unit = 0;
>
> if (loader->secure == VIR_TRISTATE_BOOL_YES) {
> @@ -9610,8 +9611,10 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd,
> virCommandAddArgBuffer(cmd, &buf);
>
> if (loader->nvram) {
> + if (qemuGetDriveSourceString(loader->nvram, NULL, &nvramPath) < 0)
> + return;
NACK to this bit. We should only allow the new feature when qemu
supports VIR_QEMU_CAPS_BLOCKDEV, thus no change to this function should
be needed.
Add the check into qemu_validate.c into the appropriate place.
Nobody is reallistically going to keep maintaining support for old qemu.
> virBufferAddLit(&buf, "file=");
> - virQEMUBuildBufferEscapeComma(&buf, loader->nvram);
> + virQEMUBuildBufferEscapeComma(&buf, nvramPath);
> virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit);
>
> virCommandAddArg(cmd, "-drive");
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7180ae616b..303d9661a4 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4452,8 +4452,13 @@ qemuDomainDefPostParse(virDomainDef *def,
> }
>
> if (virDomainDefHasOldStyleROUEFI(def) &&
> - !def->os.loader->nvram)
> - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram);
> + (!def->os.loader->nvram || !def->os.loader->nvram->path)) {
> +
> + if (!def->os.loader->nvram)
> + def->os.loader->nvram = g_new0(virStorageSource, 1);
> + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path);
> + }
>
> if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0)
> return -1;
> @@ -11133,15 +11138,23 @@ qemuDomainInitializePflashStorageSource(virDomainObj *vm)
> pflash0->nodeformat = g_strdup("libvirt-pflash0-format");
> pflash0->nodestorage = g_strdup("libvirt-pflash0-storage");
>
> -
> if (def->os.loader->nvram) {
> pflash1 = virStorageSourceNew();
> - pflash1->type = VIR_STORAGE_TYPE_FILE;
> pflash1->format = VIR_STORAGE_FILE_RAW;
> - pflash1->path = g_strdup(def->os.loader->nvram);
> + pflash1->path = g_strdup(def->os.loader->nvram->path);
> pflash1->readonly = false;
> pflash1->nodeformat = g_strdup("libvirt-pflash1-format");
> pflash1->nodestorage = g_strdup("libvirt-pflash1-storage");
> +
> + if (def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) {
> + pflash1->type = VIR_STORAGE_TYPE_FILE;
> + } else if (def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK) {
> + pflash1->protocol = def->os.loader->nvram->protocol;
> + pflash1->hosts = g_new0(virStorageNetHostDef, 1);
> + pflash1->nhosts = 1;
> + pflash1->hosts[0].name = def->os.loader->nvram->hosts[0].name;
> + pflash1->hosts[0].port = def->os.loader->nvram->hosts[0].port;
since both 'def->os.loader->nvram' and pflash1 are 'virStorageSource'
you should use virStorageSourceCopy instead of this.
> + }
> }
>
> priv->pflash0 = g_steal_pointer(&pflash0);
[...]
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 51223faadf..fa99c7d217 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -1192,13 +1192,16 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver,
> VIR_FREE(def->os.loader->nvramTemplate);
> def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename);
>
> - if (!def->os.loader->nvram)
> - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram);
> + if (!def->os.loader->nvram) {
> + def->os.loader->nvram = g_new0(virStorageSource, 1);
> + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path);
> + }
>
> VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'",
> def->os.loader->path,
> def->os.loader->nvramTemplate,
> - def->os.loader->nvram);
> + def->os.loader->nvram->path);
> break;
>
> case QEMU_FIRMWARE_DEVICE_KERNEL:
> @@ -1364,8 +1367,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
> * its path in domain XML) but no template for NVRAM was
> * specified and the varstore doesn't exist ... */
> if (!virDomainDefHasOldStyleROUEFI(def) ||
> - def->os.loader->nvramTemplate ||
> - (!reset_nvram && virFileExists(def->os.loader->nvram)))
> + def->os.loader->nvramTemplate ||
> + (def->os.loader->nvram &&
> + !reset_nvram && ((def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE
> + && virFileExists(def->os.loader->nvram->path)) ||
> + (def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK &&
> + def->os.loader->nvram->path))))
This condition got a bit too crazy ... please factor out the inner
clause you've added into a sub-condition.
> return 0;
>
> /* ... then we want to consult JSON FW descriptors first,
[...]
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index acd18494d3..d6a482d28b 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -994,10 +994,10 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data,
> VIR_DEBUG("def->os.cmdline %s", def->os.cmdline);
> VIR_DEBUG("def->os.root %s", def->os.root);
> if (def->os.loader) {
> - VIR_DEBUG("def->os.loader->path %s", def->os.loader->path);
> - VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly);
> - VIR_DEBUG("def->os.loader->type %d", def->os.loader->type);
> - VIR_DEBUG("def->os.loader->nvram %s", def->os.loader->nvram);
> + VIR_DEBUG("def->os.loader->path %s", def->os.loader->path);
> + VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly);
> + VIR_DEBUG("def->os.loader->type %d", def->os.loader->type);
> + VIR_DEBUG("def->os.loader->nvram->path %s", def->os.loader->nvram->path);
These seem to be aligning with the block above as well, but IMO we
should not do any alignment here. I'll post a patch removing the
whitespace, so please rebase to it.
> }
> VIR_DEBUG("def->os.bootloader %s", def->os.bootloader);
> VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs);
> diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args b/tests/qemuxml2argvdata/bios-nvram-network.args
> new file mode 100644
> index 0000000000..05075b45de
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/bios-nvram-network.args
> @@ -0,0 +1,35 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/tmp/lib/domain--1-test-bios \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \
> +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \
> +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=test-bios,debug-threads=on \
> +-S \
> +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-test-bios/master-key.aes \
> +-machine pc,usb=off,dump-guest-core=off \
> +-accel tcg \
> +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \
> +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example,if=pflash,format=raw,unit=1 \
As noted below we need at least one test case which uses -blockdev for
this which should be used by modern qemu.
> +-m 1024 \
> +-realtime mlock=off \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-test-bios/monitor.sock,server=on,wait=off \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-boot menu=on,strict=on \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
> +-device usb-tablet,id=input0,bus=usb.0,port=1 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
> +-msg timestamp=on
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e7fecb24d3..7ec45ec74b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1189,6 +1189,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_ISA_SERIAL);
> DO_TEST_NOCAPS("bios-nvram");
> DO_TEST_PARSE_ERROR_NOCAPS("bios-nvram-no-path");
> + DO_TEST_NOCAPS("bios-nvram-network");
For new tests, please always use 'DO_TEST_CAPS_LATEST' or
DO_TEST_CAPS_VER for older versions. New use of DO_TEST_NOCAPS is not
desirable.
> DO_TEST_CAPS_LATEST("bios-nvram-rw");
> DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit");
> DO_TEST("bios-nvram-secure",
> --
> 2.25.1
>
