Introduce a small kludge in the parser to avoid unnecessarily
blocking incoming migration from a range of recent libvirt
releases.
https://bugzilla.redhat.com/show_bug.cgi?id=2184966
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/conf/domain_conf.c | 39 ++++++++++++++++++-
...are-manual-efi-features.x86_64-latest.args | 35 +++++++++++++++++
tests/qemuxml2argvtest.c | 6 ++-
...ware-manual-efi-features.x86_64-latest.xml | 36 +++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
5 files changed, 114 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args
create mode 100644 tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 70e4d52ee6..bc20acf103 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17210,11 +17210,13 @@ virDomainDefParseBootKernelOptions(virDomainDef *def,
static int
virDomainDefParseBootFirmwareOptions(virDomainDef *def,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ unsigned int flags)
{
g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
g_autofree xmlNodePtr *nodes = NULL;
g_autofree int *features = NULL;
+ bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
int fw = 0;
int n = 0;
size_t i;
@@ -17222,6 +17224,39 @@ virDomainDefParseBootFirmwareOptions(virDomainDef *def,
if ((n = virXPathNodeSet("./os/firmware/feature", ctxt, &nodes)) < 0)
return -1;
+ /* Migration compatibility kludge.
+ *
+ * Between 8.6.0 and 9.1.0 (extremes included), the migratable
+ * XML produced when feature-based firmware autoselection was
+ * enabled looked like
+ *
+ * <os>
+ * <firmware>
+ * <feature name='foo' enabled='yes'/>
+ *
+ * Notice how there's no firmware='foo' attribute for the <os>
+ * element, meaning that firmware autoselection is disabled, and
+ * yet some <feature> elements, which are used to control the
+ * firmware autoselection process, are present. We don't consider
+ * this to be a valid combination, and want such a configuration
+ * to get rejected when submitted by users.
+ *
+ * In order to achieve that, while at the same time keeping
+ * migration coming from the libvirt versions listed above
+ * working, we can simply stop parsing early and ignore the
+ * <feature> tags when firmware autoselection is not enabled,
+ * *except* if we're defining a new domain.
+ *
+ * This is safe to do because the configuration will either come
+ * from another libvirt instance, in which case it will have a
+ * properly filled in <loader> element that contains enough
+ * information to successfully define and start the domain, or it
+ * will be a random configuration that lacks such information, in
+ * which case a different failure will be reported anyway.
+ */
+ if (n > 0 && !firmware && !abiUpdate)
+ return 0;
+
if (n > 0)
features = g_new0(int, VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST);
@@ -17350,7 +17385,7 @@ virDomainDefParseBootOptions(virDomainDef *def,
case VIR_DOMAIN_OSTYPE_HVM:
virDomainDefParseBootKernelOptions(def, ctxt);
- if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0)
+ if (virDomainDefParseBootFirmwareOptions(def, ctxt, flags) < 0)
return -1;
if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0)
diff --git a/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args b/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args
new file mode 100644
index 0000000000..57c71542cd
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
+-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
+-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \
+-machine pc-i440fx-4.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,acpi=on \
+-accel tcg \
+-cpu qemu64 \
+-m 1024 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e055d372fa..1808d9fc02 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1051,7 +1051,11 @@ mymain(void)
DO_TEST_CAPS_LATEST("firmware-manual-bios-stateless");
DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-bios-not-stateless");
DO_TEST_CAPS_LATEST("firmware-manual-efi");
- DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-efi-features");
+ DO_TEST_CAPS_LATEST("firmware-manual-efi-features");
+ DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-manual-efi-features", "x86_64",
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
+ ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
+ ARG_END);
DO_TEST_CAPS_LATEST("firmware-manual-efi-rw");
DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-implicit");
DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-secure");
diff --git a/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml b/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml
new file mode 100644
index 0000000000..dc4b8bb97f
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml
@@ -0,0 +1,36 @@
+<domain type='qemu'>
+ <name>guest</name>
+ <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os firmware='efi'>
+ <type arch='x86_64' machine='pc-i440fx-4.0'>hvm</type>
+ <firmware>
+ <feature enabled='no' name='enrolled-keys'/>
+ <feature enabled='no' name='secure-boot'/>
+ </firmware>
+ <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+ <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <acpi/>
+ </features>
+ <cpu mode='custom' match='exact' check='none'>
+ <model fallback='forbid'>qemu64</model>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb' index='0' model='none'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <audio id='1' type='none'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index e19aba0878..b1bc6e9216 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -866,6 +866,7 @@ mymain(void)
DO_TEST_CAPS_LATEST("firmware-manual-bios");
DO_TEST_CAPS_LATEST("firmware-manual-bios-stateless");
DO_TEST_CAPS_LATEST("firmware-manual-efi");
+ DO_TEST_CAPS_LATEST("firmware-manual-efi-features");
DO_TEST_CAPS_LATEST("firmware-manual-efi-rw");
DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-implicit");
DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-secure");
--
2.39.2On a Tuesday in 2023, Andrea Bolognani wrote:
>Introduce a small kludge in the parser to avoid unnecessarily
>blocking incoming migration from a range of recent libvirt
>releases.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=2184966
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> src/conf/domain_conf.c | 39 ++++++++++++++++++-
> ...are-manual-efi-features.x86_64-latest.args | 35 +++++++++++++++++
> tests/qemuxml2argvtest.c | 6 ++-
> ...ware-manual-efi-features.x86_64-latest.xml | 36 +++++++++++++++++
> tests/qemuxml2xmltest.c | 1 +
> 5 files changed, 114 insertions(+), 3 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args
> create mode 100644 tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 70e4d52ee6..bc20acf103 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -17210,11 +17210,13 @@ virDomainDefParseBootKernelOptions(virDomainDef *def,
>
> static int
> virDomainDefParseBootFirmwareOptions(virDomainDef *def,
>- xmlXPathContextPtr ctxt)
>+ xmlXPathContextPtr ctxt,
>+ unsigned int flags)
> {
> g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
> g_autofree xmlNodePtr *nodes = NULL;
> g_autofree int *features = NULL;
>+ bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
The flag is documented as:
/* allow updates in post parse callback that would break ABI otherwise */
VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
and I also think that this is something that better belongs in
post-parse.
Jano
> int fw = 0;
> int n = 0;
> size_t i;
On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
> On a Tuesday in 2023, Andrea Bolognani wrote:
> > static int
> > virDomainDefParseBootFirmwareOptions(virDomainDef *def,
> > - xmlXPathContextPtr ctxt)
> > + xmlXPathContextPtr ctxt,
> > + unsigned int flags)
> > {
> > g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
> > g_autofree xmlNodePtr *nodes = NULL;
> > g_autofree int *features = NULL;
> > + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
>
> The flag is documented as:
> /* allow updates in post parse callback that would break ABI otherwise */
> VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
>
> and I also think that this is something that better belongs in
> post-parse.
Okay, so the idea would be to keep picking up the firmware features
here and possibly drop them from the DomainDef during the PostParse
phase? I think that could work too. Let me give it a try.
--
Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 12, 2023 at 04:52:16AM -0700, Andrea Bolognani wrote:
> On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
> > On a Tuesday in 2023, Andrea Bolognani wrote:
> > > static int
> > > virDomainDefParseBootFirmwareOptions(virDomainDef *def,
> > > - xmlXPathContextPtr ctxt)
> > > + xmlXPathContextPtr ctxt,
> > > + unsigned int flags)
> > > {
> > > g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
> > > g_autofree xmlNodePtr *nodes = NULL;
> > > g_autofree int *features = NULL;
> > > + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
> >
> > The flag is documented as:
> > /* allow updates in post parse callback that would break ABI otherwise */
> > VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
> >
> > and I also think that this is something that better belongs in
> > post-parse.
>
> Okay, so the idea would be to keep picking up the firmware features
> here and possibly drop them from the DomainDef during the PostParse
> phase? I think that could work too. Let me give it a try.
This doesn't seem to work as smoothly as I hoped it would :(
The problem is that, if the kludge is performed in PostParse, the
call tree ends up looking like
virDomainDefPostParse()
qemuDomainDefPostParse()
qemuFirmwareFillDomain() <- firmware selection
virDomainDefOSValidate()
virDomainDefPostParseCommon()
virDomainDefPostParseOs() <- migration kludge
and firmware selection fails, because the configuration it sees is
the original one, which doesn't pass validation.
I could take virDomainDefPostParseOs() out of
virDomainDefPostParseCommon() and ensure it gets called before the
driver-specific PostParse callback, but to be honest I'm not
convinced making the kludge even more invasive would represent an
improvement.
Other ideas?
--
Andrea Bolognani / Red Hat / Virtualization
On a Wednesday in 2023, Andrea Bolognani wrote:
>On Wed, Apr 12, 2023 at 04:52:16AM -0700, Andrea Bolognani wrote:
>> On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
>> > On a Tuesday in 2023, Andrea Bolognani wrote:
>> > > static int
>> > > virDomainDefParseBootFirmwareOptions(virDomainDef *def,
>> > > - xmlXPathContextPtr ctxt)
>> > > + xmlXPathContextPtr ctxt,
>> > > + unsigned int flags)
>> > > {
>> > > g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
>> > > g_autofree xmlNodePtr *nodes = NULL;
>> > > g_autofree int *features = NULL;
>> > > + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
>> >
>> > The flag is documented as:
>> > /* allow updates in post parse callback that would break ABI otherwise */
>> > VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
>> >
>> > and I also think that this is something that better belongs in
>> > post-parse.
>>
>> Okay, so the idea would be to keep picking up the firmware features
>> here and possibly drop them from the DomainDef during the PostParse
>> phase? I think that could work too. Let me give it a try.
>
>This doesn't seem to work as smoothly as I hoped it would :(
>
>The problem is that, if the kludge is performed in PostParse, the
>call tree ends up looking like
>
> virDomainDefPostParse()
> qemuDomainDefPostParse()
> qemuFirmwareFillDomain() <- firmware selection
> virDomainDefOSValidate()
> virDomainDefPostParseCommon()
> virDomainDefPostParseOs() <- migration kludge
>
>and firmware selection fails, because the configuration it sees is
>the original one, which doesn't pass validation.
>
>I could take virDomainDefPostParseOs() out of
>virDomainDefPostParseCommon() and ensure it gets called before the
>driver-specific PostParse callback, but to be honest I'm not
>convinced making the kludge even more invasive would represent an
>improvement.
You could call it virDomainDefPrePostParse.
>
>Other ideas?
>
Make the complete separation of XML parsing and post-parsing a problem
of future ourselves.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
© 2016 - 2026 Red Hat, Inc.