[libvirt] [PATCH 0/7] conf: Replace SKIP_OSTYPE with SKIP_VALIDATE
The SKIP_OSTYPE domain parse flag is largely redundant nowadays since we have SKIP_VALIDATE. This series smoothes out some of the SKIP_OSTYPE weirdness so we can drop it Cole Robinson (7): conf: Break out virDomainDefParseCaps conf: Clean up virDomainDefParseCaps tests: qemuhotplug: Fix segfault when XML loading fails conf: Drop unneccessary caps parsing logic conf: Sync caps data even when SKIP_OSTYPE_CHECKS tests: Remove redundant lxc test conf: Replace SKIP_OSTYPE_CHECKS with SKIP_VALIDATE src/conf/domain_conf.c | 175 +++++++++++---------- src/conf/domain_conf.h | 13 +- src/conf/snapshot_conf.c | 2 - src/conf/virdomainobjlist.c | 2 - tests/lxcxml2xmltest.c | 2 - tests/qemuhotplugtest.c | 2 + tests/qemuxml2argvdata/missing-machine.xml | 2 +- tests/qemuxml2argvtest.c | 5 +- tests/qemuxml2xmltest.c | 1 - tests/testutils.c | 13 +- tests/testutilsqemu.c | 18 +++ tests/vircapstest.c | 2 - 12 files changed, 132 insertions(+), 105 deletions(-) -- 2.17.1
Handles parse virtType, os.type, bootloader bits, arch, machine, emulator Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 96 +++++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 178c6d2711..7eb5ffc718 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19114,46 +19114,15 @@ virDomainCachetuneDefParse(virDomainDefPtr def, } -static virDomainDefPtr -virDomainDefParseXML(xmlDocPtr xml, - xmlNodePtr root, - xmlXPathContextPtr ctxt, - virCapsPtr caps, - virDomainXMLOptionPtr xmlopt, - unsigned int flags) +static int +virDomainDefParseCaps(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + virCapsPtr caps, + unsigned int flags) { - xmlNodePtr *nodes = NULL, node = NULL; + int ret = -1; + int virtType; char *tmp = NULL; - size_t i, j; - int n, virtType, gic_version; - long id = -1; - virDomainDefPtr def; - bool uuid_generated = false; - bool usb_none = false; - bool usb_other = false; - bool usb_master = false; - char *netprefix = NULL; - - if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) { - char *schema = virFileFindResource("domain.rng", - abs_topsrcdir "/docs/schemas", - PKGDATADIR "/schemas"); - if (!schema) - return NULL; - if (virXMLValidateAgainstSchema(schema, xml) < 0) { - VIR_FREE(schema); - return NULL; - } - VIR_FREE(schema); - } - - if (!(def = virDomainDefNew())) - return NULL; - - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) - if (virXPathLong("string(./@id)", ctxt, &id) < 0) - id = -1; - def->id = (int)id; /* Find out what type of virtualization to use */ if (!(tmp = virXMLPropString(ctxt->node, "type"))) { @@ -19239,6 +19208,57 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(capsdata); } + ret = 0; + error: + VIR_FREE(tmp); + return ret; +} + + +static virDomainDefPtr +virDomainDefParseXML(xmlDocPtr xml, + xmlNodePtr root, + xmlXPathContextPtr ctxt, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + xmlNodePtr *nodes = NULL, node = NULL; + char *tmp = NULL; + size_t i, j; + int n, gic_version; + long id = -1; + virDomainDefPtr def; + bool uuid_generated = false; + bool usb_none = false; + bool usb_other = false; + bool usb_master = false; + char *netprefix = NULL; + + if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) { + char *schema = virFileFindResource("domain.rng", + abs_topsrcdir "/docs/schemas", + PKGDATADIR "/schemas"); + if (!schema) + return NULL; + if (virXMLValidateAgainstSchema(schema, xml) < 0) { + VIR_FREE(schema); + return NULL; + } + VIR_FREE(schema); + } + + if (!(def = virDomainDefNew())) + return NULL; + + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + if (virXPathLong("string(./@id)", ctxt, &id) < 0) + id = -1; + def->id = (int)id; + + if (virDomainDefParseCaps(def, ctxt, caps, flags) < 0) + goto error; + /* Extract domain name */ if (!(def->name = virXPathString("string(./name[1])", ctxt))) { virReportError(VIR_ERR_NO_NAME, NULL); -- 2.17.1
- Convert to 'cleanup' label naming - Use more than one 'tmp' string and do all freeing at the end - Make the code easier to follow Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 76 ++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7eb5ffc718..5a90429cd6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19121,43 +19121,45 @@ virDomainDefParseCaps(virDomainDefPtr def, unsigned int flags) { int ret = -1; - int virtType; - char *tmp = NULL; + char *virttype = NULL; + char *arch = NULL; + char *ostype = NULL; + virCapsDomainDataPtr capsdata = NULL; - /* Find out what type of virtualization to use */ - if (!(tmp = virXMLPropString(ctxt->node, "type"))) { + virttype = virXPathString("string(./@type)", ctxt); + ostype = virXPathString("string(./os/type[1])", ctxt); + arch = virXPathString("string(./os/type[1]/@arch)", ctxt); + + def->os.bootloader = virXPathString("string(./bootloader)", ctxt); + def->os.bootloaderArgs = virXPathString("string(./bootloader_args)", ctxt); + def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt); + def->emulator = virXPathString("string(./devices/emulator[1])", ctxt); + + if (!virttype) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing domain type attribute")); - goto error; + goto cleanup; } - - if ((virtType = virDomainVirtTypeFromString(tmp)) < 0) { + if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid domain type %s"), tmp); - goto error; + _("invalid domain type %s"), virttype); + goto cleanup; } - def->virtType = virtType; - VIR_FREE(tmp); - - def->os.bootloader = virXPathString("string(./bootloader)", ctxt); - def->os.bootloaderArgs = virXPathString("string(./bootloader_args)", ctxt); - tmp = virXPathString("string(./os/type[1])", ctxt); - if (!tmp) { + if (!ostype) { if (def->os.bootloader) { def->os.type = VIR_DOMAIN_OSTYPE_XEN; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("an os <type> must be specified")); - goto error; + goto cleanup; } } else { - if ((def->os.type = virDomainOSTypeFromString(tmp)) < 0) { + if ((def->os.type = virDomainOSTypeFromString(ostype)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown OS type '%s'"), tmp); - goto error; + _("unknown OS type '%s'"), ostype); + goto cleanup; } - VIR_FREE(tmp); } /* @@ -19170,17 +19172,11 @@ virDomainDefParseCaps(virDomainDefPtr def, def->os.type = VIR_DOMAIN_OSTYPE_XEN; } - tmp = virXPathString("string(./os/type[1]/@arch)", ctxt); - if (tmp && !(def->os.arch = virArchFromString(tmp))) { + if (arch && !(def->os.arch = virArchFromString(arch))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown architecture %s"), - tmp); - goto error; + _("Unknown architecture %s"), arch); + goto cleanup; } - VIR_FREE(tmp); - - def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt); - def->emulator = virXPathString("string(./devices/emulator[1])", ctxt); if ((!def->os.arch || !def->os.machine) && !(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { @@ -19191,26 +19187,28 @@ virDomainDefParseCaps(virDomainDefPtr def, * in numerous minor ways. */ bool use_virttype = ((def->os.arch == VIR_ARCH_NONE) || !def->os.machine); - virCapsDomainDataPtr capsdata = NULL; - if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, - def->os.arch, use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE, + if (!(capsdata = virCapabilitiesDomainDataLookup(caps, + def->os.type, + def->os.arch, + use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE, NULL, NULL))) - goto error; + goto cleanup; if (!def->os.arch) def->os.arch = capsdata->arch; if ((!def->os.machine && VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) { - VIR_FREE(capsdata); - goto error; + goto cleanup; } - VIR_FREE(capsdata); } ret = 0; - error: - VIR_FREE(tmp); + cleanup: + VIR_FREE(virttype); + VIR_FREE(ostype); + VIR_FREE(arch); + VIR_FREE(capsdata); return ret; } -- 2.17.1
Some tests use the same VM state multiple times in a row. But if we failed loading the VM XML, subsequent tests crash on the NULL def pointer Signed-off-by: Cole Robinson <crobinso@redhat.com> --- I hit this with failing tests while writing this series tests/qemuhotplugtest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 674ba92b27..4f9e127f88 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -268,6 +268,8 @@ testQemuHotplug(const void *data) if (test->vm) { vm = test->vm; + if (!vm->def) + goto cleanup; } else { if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml, test->deviceDeletedEvent) < 0) -- 2.17.1
On 07/24/2018 11:23 PM, Cole Robinson wrote:
Some tests use the same VM state multiple times in a row. But if we failed loading the VM XML, subsequent tests crash on the NULL def pointer
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- I hit this with failing tests while writing this series
tests/qemuhotplugtest.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 674ba92b27..4f9e127f88 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -268,6 +268,8 @@ testQemuHotplug(const void *data)
if (test->vm) { vm = test->vm; + if (!vm->def) + goto cleanup; } else { if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml, test->deviceDeletedEvent) < 0)
I wonder if we should fprintf(stderr, "test skipped because of an failure in dependant test"); or something among these lines. The idea being it's easier to debug. Look at all places where we 'goto cleanup'. At least libvirt error is reported there (virAsprintf reports OOM, virTestLoadFile reports error too, etc.). Michal
On 07/26/2018 02:44 AM, Michal Privoznik wrote:
On 07/24/2018 11:23 PM, Cole Robinson wrote:
Some tests use the same VM state multiple times in a row. But if we failed loading the VM XML, subsequent tests crash on the NULL def pointer
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- I hit this with failing tests while writing this series
tests/qemuhotplugtest.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 674ba92b27..4f9e127f88 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -268,6 +268,8 @@ testQemuHotplug(const void *data)
if (test->vm) { vm = test->vm; + if (!vm->def) + goto cleanup; } else { if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml, test->deviceDeletedEvent) < 0)
I wonder if we should fprintf(stderr, "test skipped because of an failure in dependant test"); or something among these lines. The idea being it's easier to debug. Look at all places where we 'goto cleanup'. At least libvirt error is reported there (virAsprintf reports OOM, virTestLoadFile reports error too, etc.).
I'll use VIR_TEST_VERBOSE() which will give similarish behavior to virReportError in this case: output will only be shown if VIR_TEST_DEBUG=1 or similar is used Thanks, Cole
The comment says: /* If the logic here seems fairly arbitrary, that's because it is :) * This is duplicating how the code worked before * CapabilitiesDomainDataLookup was added. We can simplify this, * but it would take a bit of work because the test suite fails * in numerous minor ways. */ Nowadays the test suite changes appear quite simple, just extending test capabilities data a bit so that we aren't trying to define invalid arch/os/virtType/machine combos Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 15 ++------------- tests/testutils.c | 13 ++++++++++++- tests/testutilsqemu.c | 18 ++++++++++++++++++ tests/vircapstest.c | 2 -- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5a90429cd6..b7f6a22e20 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19178,20 +19178,9 @@ virDomainDefParseCaps(virDomainDefPtr def, goto cleanup; } - if ((!def->os.arch || !def->os.machine) && - !(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { - /* If the logic here seems fairly arbitrary, that's because it is :) - * This is duplicating how the code worked before - * CapabilitiesDomainDataLookup was added. We can simplify this, - * but it would take a bit of work because the test suite fails - * in numerous minor ways. */ - bool use_virttype = ((def->os.arch == VIR_ARCH_NONE) || - !def->os.machine); - + if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { if (!(capsdata = virCapabilitiesDomainDataLookup(caps, - def->os.type, - def->os.arch, - use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE, + def->os.type, def->os.arch, def->virtType, NULL, NULL))) goto cleanup; diff --git a/tests/testutils.c b/tests/testutils.c index 423f4bfdff..ab938c12fc 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1196,7 +1196,12 @@ virCapsPtr virTestGenericCapsInit(void) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, NULL, 0, NULL)) goto error; - + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, + NULL, NULL, 0, NULL)) + goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, "/usr/bin/acme-virt", NULL, @@ -1205,6 +1210,12 @@ virCapsPtr virTestGenericCapsInit(void) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, + NULL, NULL, 0, NULL)) + goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; if (virTestGetDebug() > 1) { diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index dc7e90b952..cc2f8a7b64 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -219,6 +219,9 @@ static int testQemuAddPPC64Guest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; @@ -246,6 +249,9 @@ static int testQemuAddPPC64LEGuest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; @@ -276,6 +282,9 @@ static int testQemuAddPPCGuest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; @@ -307,6 +316,9 @@ static int testQemuAddS390Guest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; @@ -338,6 +350,9 @@ static int testQemuAddArmGuest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; @@ -367,6 +382,9 @@ static int testQemuAddAARCH64Guest(virCapsPtr caps) if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) goto error; + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, + NULL, NULL, 0, NULL)) + goto error; return 0; diff --git a/tests/vircapstest.c b/tests/vircapstest.c index 1df3fa091f..19e3c79302 100644 --- a/tests/vircapstest.c +++ b/tests/vircapstest.c @@ -195,8 +195,6 @@ test_virCapsDomainDataLookupQEMU(const void *data ATTRIBUTE_UNUSED) CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_LINUX, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL); CAPS_EXPECT_ERR(-1, VIR_ARCH_PPC64LE, VIR_DOMAIN_VIRT_NONE, NULL, "pc"); CAPS_EXPECT_ERR(-1, VIR_ARCH_MIPS, VIR_DOMAIN_VIRT_NONE, NULL, NULL); - CAPS_EXPECT_ERR(-1, VIR_ARCH_AARCH64, VIR_DOMAIN_VIRT_KVM, - "/usr/bin/qemu-system-aarch64", NULL); CAPS_EXPECT_ERR(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, "/usr/bin/qemu-system-aarch64", "pc"); CAPS_EXPECT_ERR(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_VMWARE, NULL, "pc"); -- 2.17.1
We should still make an effort to fill in data, just not raise an error if say an ostype/virttype combo disappeared from caps. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 13 ++++++------- tests/qemuxml2argvdata/missing-machine.xml | 2 +- tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7f6a22e20..78ee000857 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def, goto cleanup; } - if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { - if (!(capsdata = virCapabilitiesDomainDataLookup(caps, - def->os.type, def->os.arch, def->virtType, - NULL, NULL))) + if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, + def->os.arch, def->virtType, NULL, NULL))) { + if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)) goto cleanup; - + virResetLastError(); + } else { if (!def->os.arch) def->os.arch = capsdata->arch; if ((!def->os.machine && - VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) { + VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) goto cleanup; - } } ret = 0; diff --git a/tests/qemuxml2argvdata/missing-machine.xml b/tests/qemuxml2argvdata/missing-machine.xml index 4ce7b377a5..2900baec90 100644 --- a/tests/qemuxml2argvdata/missing-machine.xml +++ b/tests/qemuxml2argvdata/missing-machine.xml @@ -6,7 +6,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1'>1</vcpu> <os> - <type arch='i686'>hvm</type> + <type arch='alpha'>hvm</type> <boot dev='hd'/> </os> <clock offset='utc'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1a936faef1..03b6d92912 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2773,6 +2773,9 @@ mymain(void) QEMU_CAPS_OBJECT_GPEX, QEMU_CAPS_NEC_USB_XHCI); + /* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag + * will avoid the error. Still, we expect qemu driver to complain about + * missing machine error, and not crash */ DO_TEST_PARSE_FLAGS_ERROR("missing-machine", VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, NONE); -- 2.17.1
On 07/24/2018 11:23 PM, Cole Robinson wrote:
We should still make an effort to fill in data, just not raise an error if say an ostype/virttype combo disappeared from caps.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 13 ++++++------- tests/qemuxml2argvdata/missing-machine.xml | 2 +- tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7f6a22e20..78ee000857 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def, goto cleanup; }
- if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { - if (!(capsdata = virCapabilitiesDomainDataLookup(caps, - def->os.type, def->os.arch, def->virtType, - NULL, NULL))) + if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, + def->os.arch, def->virtType, NULL, NULL))) {
This looks misaligned ;-)
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)) goto cleanup;
So you're changing the flag here even though I believe it belongs to the next patch. It helps downstream maintainers, but in the end the code will look the same.
- + virResetLastError(); + } else { if (!def->os.arch) def->os.arch = capsdata->arch; if ((!def->os.machine && - VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) { + VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) goto cleanup; - } }
ret = 0; diff --git a/tests/qemuxml2argvdata/missing-machine.xml b/tests/qemuxml2argvdata/missing-machine.xml index 4ce7b377a5..2900baec90 100644 --- a/tests/qemuxml2argvdata/missing-machine.xml +++ b/tests/qemuxml2argvdata/missing-machine.xml @@ -6,7 +6,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1'>1</vcpu> <os> - <type arch='i686'>hvm</type> + <type arch='alpha'>hvm</type>
Firstly I was wondering why is this change needed, but then I read the comment in the next hunk and it makes sense. We need to have non-existent pair of arch and os type so that the error is triggered.
<boot dev='hd'/> </os> <clock offset='utc'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1a936faef1..03b6d92912 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2773,6 +2773,9 @@ mymain(void) QEMU_CAPS_OBJECT_GPEX, QEMU_CAPS_NEC_USB_XHCI);
+ /* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag + * will avoid the error. Still, we expect qemu driver to complain about + * missing machine error, and not crash */ DO_TEST_PARSE_FLAGS_ERROR("missing-machine", VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, NONE);
Michal
On 07/26/2018 02:44 AM, Michal Privoznik wrote:
On 07/24/2018 11:23 PM, Cole Robinson wrote:
We should still make an effort to fill in data, just not raise an error if say an ostype/virttype combo disappeared from caps.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 13 ++++++------- tests/qemuxml2argvdata/missing-machine.xml | 2 +- tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7f6a22e20..78ee000857 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def, goto cleanup; }
- if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) { - if (!(capsdata = virCapabilitiesDomainDataLookup(caps, - def->os.type, def->os.arch, def->virtType, - NULL, NULL))) + if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, + def->os.arch, def->virtType, NULL, NULL))) {
This looks misaligned ;-)
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)) goto cleanup;
So you're changing the flag here even though I believe it belongs to the next patch. It helps downstream maintainers, but in the end the code will look the same.
Good catch, mistake on my part. I'll fix before pushing. Thanks for the review! - Cole
- + virResetLastError(); + } else { if (!def->os.arch) def->os.arch = capsdata->arch; if ((!def->os.machine && - VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) { + VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) goto cleanup; - } }
ret = 0; diff --git a/tests/qemuxml2argvdata/missing-machine.xml b/tests/qemuxml2argvdata/missing-machine.xml index 4ce7b377a5..2900baec90 100644 --- a/tests/qemuxml2argvdata/missing-machine.xml +++ b/tests/qemuxml2argvdata/missing-machine.xml @@ -6,7 +6,7 @@ <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1'>1</vcpu> <os> - <type arch='i686'>hvm</type> + <type arch='alpha'>hvm</type>
Firstly I was wondering why is this change needed, but then I read the comment in the next hunk and it makes sense. We need to have non-existent pair of arch and os type so that the error is triggered.
<boot dev='hd'/> </os> <clock offset='utc'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1a936faef1..03b6d92912 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2773,6 +2773,9 @@ mymain(void) QEMU_CAPS_OBJECT_GPEX, QEMU_CAPS_NEC_USB_XHCI);
+ /* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag + * will avoid the error. Still, we expect qemu driver to complain about + * missing machine error, and not crash */ DO_TEST_PARSE_FLAGS_ERROR("missing-machine", VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, NONE);
Michal
This test was added in 2d40e2da7ba to ensure LXC domains could be defined correctly when caps probing was skipped due to SKIP_OSTYPE. However we do caps probing unconditionally now, so this test case is redundant Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/lxcxml2xmltest.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c index 3b96862c62..5dbeb0b2eb 100644 --- a/tests/lxcxml2xmltest.c +++ b/tests/lxcxml2xmltest.c @@ -96,8 +96,6 @@ mymain(void) DO_TEST("sharenet"); DO_TEST("ethernet"); DO_TEST("ethernet-hostip"); - DO_TEST_FULL("filesystem-root", 0, false, - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS); DO_TEST("initenv"); DO_TEST("initdir"); DO_TEST("inituser"); -- 2.17.1
SKIP_OSTYPE_CHECKS only hides some error reporting at this point, so it can be foled into SKIP_VALIDATE Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 13 +++++-------- src/conf/snapshot_conf.c | 2 -- src/conf/virdomainobjlist.c | 2 -- tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmltest.c | 1 - 6 files changed, 7 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 78ee000857..41baac08c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28780,8 +28780,7 @@ virDomainDefCopy(virDomainDefPtr src, virDomainDefPtr ret; unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS; + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; if (migratable) format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE | VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5e2f21dea3..a804e86f6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2981,24 +2981,21 @@ typedef enum { VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 6, /* perform RNG schema validation on the passed XML document */ VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA = 1 << 7, - /* don't validate os.type and arch against capabilities. Prevents - * VMs from disappearing when qemu is removed and libvirtd is restarted */ - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8, /* allow updates in post parse callback that would break ABI otherwise */ - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9, + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 8, /* skip definition validation checks meant to be executed on define time only */ - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 10, + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 9, /* skip parsing of security labels */ - VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL = 1 << 11, + VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL = 1 << 10, /* Allows updates in post parse callback for incoming persistent migration * that would break ABI otherwise. This should be used only if it's safe * to do such change. */ - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION = 1 << 12, + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION = 1 << 11, /* Allows to ignore certain failures in the post parse callbacks, which * may happen due to missing packages and can be fixed by re-running the * post parse callbacks before starting. Failure of the post parse callback * is recorded as def->postParseFail */ - VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 13, + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 12, } virDomainDefParseFlags; typedef enum { diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 9c537ac7d1..adba149241 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -273,8 +273,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) - domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS; xmlNodePtr domainNode = virXPathNode("./domain", ctxt); VIR_FREE(tmp); diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 72064d7c66..52171594f3 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -492,7 +492,6 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms, goto error; if (!(def = virDomainDefParseFile(configFile, caps, xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) goto error; @@ -544,7 +543,6 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) goto error; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 03b6d92912..84117a3e63 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2777,7 +2777,7 @@ mymain(void) * will avoid the error. Still, we expect qemu driver to complain about * missing machine error, and not crash */ DO_TEST_PARSE_FLAGS_ERROR("missing-machine", - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE, NONE); DO_TEST("name-escape", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 795ddc7003..c6cb2dda0c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -76,7 +76,6 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) goto cleanup; -- 2.17.1
On 07/24/2018 11:23 PM, Cole Robinson wrote:
The SKIP_OSTYPE domain parse flag is largely redundant nowadays since we have SKIP_VALIDATE. This series smoothes out some of the SKIP_OSTYPE weirdness so we can drop it
Cole Robinson (7): conf: Break out virDomainDefParseCaps conf: Clean up virDomainDefParseCaps tests: qemuhotplug: Fix segfault when XML loading fails conf: Drop unneccessary caps parsing logic conf: Sync caps data even when SKIP_OSTYPE_CHECKS tests: Remove redundant lxc test conf: Replace SKIP_OSTYPE_CHECKS with SKIP_VALIDATE
src/conf/domain_conf.c | 175 +++++++++++---------- src/conf/domain_conf.h | 13 +- src/conf/snapshot_conf.c | 2 - src/conf/virdomainobjlist.c | 2 - tests/lxcxml2xmltest.c | 2 - tests/qemuhotplugtest.c | 2 + tests/qemuxml2argvdata/missing-machine.xml | 2 +- tests/qemuxml2argvtest.c | 5 +- tests/qemuxml2xmltest.c | 1 - tests/testutils.c | 13 +- tests/testutilsqemu.c | 18 +++ tests/vircapstest.c | 2 - 12 files changed, 132 insertions(+), 105 deletions(-)
ACK series. Michal
participants (2)
-
Cole Robinson -
Michal Privoznik