4:23 p.m.
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
4:23 p.m.
New subject: [libvirt] [PATCH 1/7] conf: Break out virDomainDefParseCaps
Handles parse virtType, os.type, bootloader bits, arch, machine,
emulator
Signed-off-by: Cole Robinson <crobinso(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 2/7] conf: Clean up virDomainDefParseCaps
- 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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 3/7] tests: qemuhotplug: Fix segfault when XML loading fails
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(a)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
1:44 a.m.
New subject: [libvirt] [PATCH 3/7] tests: qemuhotplug: Fix segfault when XML loading fails
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(a)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
9:05 a.m.
New subject: [libvirt] [PATCH 3/7] tests: qemuhotplug: Fix segfault when XML loading fails
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 4/7] conf: Drop unneccessary caps parsing logic
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS
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(a)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
1:44 a.m.
New subject: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS
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(a)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
8:23 a.m.
New subject: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 6/7] tests: Remove redundant lxc test
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH 7/7] conf: Replace SKIP_OSTYPE_CHECKS with SKIP_VALIDATE
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(a)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
1:44 a.m.
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
2311
days inactive
2313
days old
12 comments
2 participants
participants (2)
-
Cole Robinson -
Michal Privoznik