[libvirt] [PATCHv2 0/5] Fix machine type parsing regressions
Patch 2/5 is the important one, fixing persistent non-QEMU domains. Ján Tomko (5): testCompareDomXML2XMLFiles: add parseFlags parameter Revert "Error out on missing machine type in machine configs" tests: add a test for persistent LXC XML parsing tests: add parseFlags to qemuxml2argvtest qemu: error out on missing machine type in configs src/conf/domain_conf.c | 6 --- src/qemu/qemu_domain.c | 6 +++ tests/bhyvexml2xmltest.c | 2 +- tests/genericxml2xmltest.c | 2 +- tests/lxcxml2xmltest.c | 14 ++++--- .../qemuxml2argv-missing-machine.xml | 30 +++++++++++++++ tests/qemuxml2argvtest.c | 43 ++++++++++++++-------- tests/qemuxml2xmltest.c | 4 +- tests/testutils.c | 5 ++- tests/testutils.h | 3 +- 10 files changed, 83 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml -- 2.4.10
Allow testing XML parsing with different flags. --- tests/bhyvexml2xmltest.c | 2 +- tests/genericxml2xmltest.c | 2 +- tests/lxcxml2xmltest.c | 2 +- tests/qemuxml2xmltest.c | 4 ++-- tests/testutils.c | 5 ++++- tests/testutils.h | 3 ++- 6 files changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c index 2d34a00..8f556ee 100644 --- a/tests/bhyvexml2xmltest.c +++ b/tests/bhyvexml2xmltest.c @@ -33,7 +33,7 @@ testCompareXMLToXMLHelper(const void *data) ret = testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, xml_in, info->different ? xml_out : xml_in, false, - NULL, NULL); + NULL, NULL, 0); cleanup: VIR_FREE(xml_in); diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index aa3a570..bf9b11d 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -40,7 +40,7 @@ testCompareXMLToXMLHelper(const void *data) ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in, info->different ? xml_out : xml_in, !info->inactive_only, - NULL, NULL); + NULL, NULL, 0); cleanup: VIR_FREE(xml_in); VIR_FREE(xml_out); diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c index aafebb4..8174a54 100644 --- a/tests/lxcxml2xmltest.c +++ b/tests/lxcxml2xmltest.c @@ -45,7 +45,7 @@ testCompareXMLToXMLHelper(const void *data) ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in, info->different ? xml_out : xml_in, !info->inactive_only, - NULL, NULL); + NULL, NULL, 0); cleanup: VIR_FREE(xml_in); VIR_FREE(xml_out); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ef6e35a..80c80f6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -54,7 +54,7 @@ testXML2XMLActive(const void *opaque) return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName, info->outActiveName, true, - qemuXML2XMLPreFormatCallback, opaque); + qemuXML2XMLPreFormatCallback, opaque, 0); } @@ -65,7 +65,7 @@ testXML2XMLInactive(const void *opaque) return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName, info->outInactiveName, false, - qemuXML2XMLPreFormatCallback, opaque); + qemuXML2XMLPreFormatCallback, opaque, 0); } diff --git a/tests/testutils.c b/tests/testutils.c index c1ca656..b1bd4e8 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1054,13 +1054,16 @@ int testCompareDomXML2XMLFiles(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, const char *infile, const char *outfile, bool live, testCompareDomXML2XMLPreFormatCallback cb, - const void *opaque) + const void *opaque, unsigned int parseFlags) { char *actual = NULL; int ret = -1; virDomainDefPtr def = NULL; unsigned int parse_flags = live ? 0 : VIR_DOMAIN_DEF_PARSE_INACTIVE; unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE; + + parse_flags |= parseFlags; + if (!live) format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; diff --git a/tests/testutils.h b/tests/testutils.h index bb58148..752fa52 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -142,6 +142,7 @@ int testCompareDomXML2XMLFiles(virCapsPtr caps, const char *outfile, bool live, testCompareDomXML2XMLPreFormatCallback cb, - const void *opaque); + const void *opaque, + unsigned int parseFlags); #endif /* __VIT_TEST_UTILS_H__ */ -- 2.4.10
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5. This fix for https://bugzilla.redhat.com/show_bug.cgi?id=1267256 unconditionally required a machine type for all machine types even though qemu is the only emulator using them. Revert it to fix persistent configs for drivers with no machine type: https://www.redhat.com/archives/libvir-list/2016-February/msg01228.html --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } } /* Extract domain name */ -- 2.4.10
Check if we correctly parse the persistent config even with the VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS flag. --- tests/lxcxml2xmltest.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c index 8174a54..0b51340 100644 --- a/tests/lxcxml2xmltest.c +++ b/tests/lxcxml2xmltest.c @@ -26,6 +26,7 @@ struct testInfo { const char *name; int different; bool inactive_only; + unsigned int parse_flags; }; static int @@ -45,7 +46,7 @@ testCompareXMLToXMLHelper(const void *data) ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in, info->different ? xml_out : xml_in, !info->inactive_only, - NULL, NULL, 0); + NULL, NULL, info->parse_flags); cleanup: VIR_FREE(xml_in); VIR_FREE(xml_out); @@ -64,19 +65,20 @@ mymain(void) if (!(xmlopt = lxcDomainXMLConfInit())) return EXIT_FAILURE; -# define DO_TEST_FULL(name, is_different, inactive) \ +# define DO_TEST_FULL(name, is_different, inactive, parse_flags) \ do { \ - const struct testInfo info = {name, is_different, inactive}; \ + const struct testInfo info = {name, is_different, inactive, \ + parse_flags}; \ if (virtTestRun("LXC XML-2-XML " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ } while (0) # define DO_TEST(name) \ - DO_TEST_FULL(name, 0, false) + DO_TEST_FULL(name, 0, false, 0) # define DO_TEST_DIFFERENT(name) \ - DO_TEST_FULL(name, 1, false) + DO_TEST_FULL(name, 1, false, 0) /* Unset or set all envvars here that are copied in lxcdBuildCommandLine * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected @@ -91,6 +93,8 @@ mymain(void) DO_TEST("idmap"); DO_TEST("capabilities"); DO_TEST("sharenet"); + DO_TEST_FULL("filesystem-root", 0, false, + VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS); virObjectUnref(caps); virObjectUnref(xmlopt); -- 2.4.10
--- tests/qemuxml2argvtest.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 854623d..517f996 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -253,7 +253,8 @@ static int testCompareXMLToArgvFiles(const char *xml, const char *cmdline, virQEMUCapsPtr extraFlags, const char *migrateURI, - virQemuXML2ArgvTestFlags flags) + virQemuXML2ArgvTestFlags flags, + unsigned int parseFlags) { char *actualargv = NULL; int ret = -1; @@ -275,7 +276,8 @@ static int testCompareXMLToArgvFiles(const char *xml, goto out; if (!(vmdef = virDomainDefParseFile(xml, driver.caps, driver.xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE))) { + (VIR_DOMAIN_DEF_PARSE_INACTIVE | + parseFlags)))) { if (!virtTestOOMActive() && (flags & FLAG_EXPECT_PARSE_ERROR)) goto ok; @@ -408,6 +410,7 @@ struct testInfo { const char *migrateFrom; int migrateFd; unsigned int flags; + unsigned int parseFlags; }; static int @@ -443,7 +446,7 @@ testCompareXMLToArgvHelper(const void *data) goto cleanup; result = testCompareXMLToArgvFiles(xml, args, info->extraFlags, - migrateURI, flags); + migrateURI, flags, info->parseFlags); cleanup: VIR_FREE(migrateURI); @@ -537,10 +540,11 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->channelTargetDir, "/tmp") < 0) return EXIT_FAILURE; -# define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, ...) \ +# define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ + parseFlags, ...) \ do { \ static struct testInfo info = { \ - name, NULL, migrateFrom, migrateFd, (flags) \ + name, NULL, migrateFrom, migrateFd, (flags), parseFlags \ }; \ if (!(info.extraFlags = virQEMUCapsNew())) \ return EXIT_FAILURE; \ @@ -554,21 +558,22 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, __VA_ARGS__) + DO_TEST_FULL(name, NULL, -1, 0, 0, __VA_ARGS__) # define DO_TEST_ERROR(name, ...) \ - DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_ERROR, __VA_ARGS__) + DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_ERROR, 0, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ - DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, __VA_ARGS__) + DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, 0, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, NULL, -1, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ - __VA_ARGS__) + 0, __VA_ARGS__) + # define DO_TEST_LINUX(name, ...) \ - DO_TEST_LINUX_FULL(name, NULL, -1, 0, __VA_ARGS__) + DO_TEST_LINUX_FULL(name, NULL, -1, 0, 0, __VA_ARGS__) # ifdef __linux__ /* This is a macro that invokes test only on Linux. It's @@ -1262,12 +1267,12 @@ mymain(void) QEMU_CAPS_PCIDEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PCI_ROMBAR); - DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, NONE); - DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, NONE); - DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, NONE); - DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, NONE); + DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, NONE); + DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, NONE); + DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, NONE); + DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, NONE); - DO_TEST_LINUX_FULL("migrate-numa-unaligned", "stdio", 7, 0, + DO_TEST_LINUX_FULL("migrate-numa-unaligned", "stdio", 7, 0, 0, QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM); -- 2.4.10
Commit f1a89a8 allowed parsing configs from /etc/libvirt without validating the emulator capabilities. Check for the presence of a machine type in the qemu driver's post parse function instead of crashing. https://bugzilla.redhat.com/show_bug.cgi?id=1267256 --- src/qemu/qemu_domain.c | 6 +++++ .../qemuxml2argv-missing-machine.xml | 30 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++++++ 3 files changed, 44 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56f9f1..8c96a33 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1307,6 +1307,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, return ret; } + if (!def->os.machine) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing machine type")); + return ret; + } + /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml b/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml new file mode 100644 index 0000000..12f7d2f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>A description of the test machine.</title> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1'>1</vcpu> + <os> + <type arch='i686'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 517f996..ff12077 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -571,6 +571,10 @@ mymain(void) FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ 0, __VA_ARGS__) +# define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ + DO_TEST_FULL(name, NULL, -1, \ + FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ + parseFlags, __VA_ARGS__) # define DO_TEST_LINUX(name, ...) \ DO_TEST_LINUX_FULL(name, NULL, -1, 0, 0, __VA_ARGS__) @@ -1868,6 +1872,10 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST_PARSE_FLAGS_ERROR("missing-machine", + VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, + NONE); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.4.10
On 02/25/2016 10:40 AM, Ján Tomko wrote:
Commit f1a89a8 allowed parsing configs from /etc/libvirt without validating the emulator capabilities.
Check for the presence of a machine type in the qemu driver's post parse function instead of crashing.
https://bugzilla.redhat.com/show_bug.cgi?id=1267256 --- src/qemu/qemu_domain.c | 6 +++++ .../qemuxml2argv-missing-machine.xml | 30 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++++++ 3 files changed, 44 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56f9f1..8c96a33 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1307,6 +1307,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, return ret; }
+ if (!def->os.machine) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing machine type"));
A "nit" of sorts... Sometimes we start error messages with Capital letters and sometimes we don't... I guess I've trained my fingers to go with lower case letters (although it really messes with my eyes/brain). Since a majority in qemu_domain go with lower case, could we take that path here too. But really what I'd like to point out - would it be possible to add "def->name" to the error message to indicate which domain had the issue. When it happened to me all I got were two messages during startup - I had to generate a local patch which added the def->name to the output to figure out which of my domains had the issue. This actually is a much bigger problem overall... We have a *lot* of messages spewed in domain_conf.c and perhaps a couple that spit out *which* domain. Now if I'm defining/creating a single domain, it's easy to figure out. Figuring it out which domain had an issue during libvirtd start/restart/reload is a problem of magnitude! I have to figure out who's missing ;-) Tks - John
+ return ret; + } + /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml b/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml new file mode 100644 index 0000000..12f7d2f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>A description of the test machine.</title> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1'>1</vcpu> + <os> + <type arch='i686'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 517f996..ff12077 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -571,6 +571,10 @@ mymain(void) FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ 0, __VA_ARGS__)
+# define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ + DO_TEST_FULL(name, NULL, -1, \ + FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ + parseFlags, __VA_ARGS__)
# define DO_TEST_LINUX(name, ...) \ DO_TEST_LINUX_FULL(name, NULL, -1, 0, 0, __VA_ARGS__) @@ -1868,6 +1872,10 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI);
+ DO_TEST_PARSE_FLAGS_ERROR("missing-machine", + VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, + NONE); + qemuTestDriverFree(&driver);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
On 02/25/2016 10:40 AM, Ján Tomko wrote:
Patch 2/5 is the important one, fixing persistent non-QEMU domains.
Ján Tomko (5): testCompareDomXML2XMLFiles: add parseFlags parameter Revert "Error out on missing machine type in machine configs" tests: add a test for persistent LXC XML parsing tests: add parseFlags to qemuxml2argvtest qemu: error out on missing machine type in configs
src/conf/domain_conf.c | 6 --- src/qemu/qemu_domain.c | 6 +++ tests/bhyvexml2xmltest.c | 2 +- tests/genericxml2xmltest.c | 2 +- tests/lxcxml2xmltest.c | 14 ++++--- .../qemuxml2argv-missing-machine.xml | 30 +++++++++++++++ tests/qemuxml2argvtest.c | 43 ++++++++++++++-------- tests/qemuxml2xmltest.c | 4 +- tests/testutils.c | 5 ++- tests/testutils.h | 3 +- 10 files changed, 83 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-missing-machine.xml
ACK series Thanks, Cole
participants (3)
-
Cole Robinson -
John Ferlan -
Ján Tomko