[libvirt] [PATCHv2 0/7] qemuxml2argvtest cleanups
Patch 1 and patch 3 were not acked in v1. The changes to the ACKed ones are minimal. Complete diff to v2: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 867e0d569f..0e43320b96 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -460,20 +460,20 @@ testCompareXMLToStartupXML(const void *data) } -# define TEST_EXCLUSIVE_FLAGS(FLAG1, FLAG2) \ - if ((testFlags & FLAG1) && (testFlags & FLAG2)) { \ - VIR_TEST_DEBUG("Flags %s and %s are mutually exclusive\n", \ - #FLAG1, #FLAG2); \ - return -1; \ - } - - static int -testCheckExclusiveFlags(int testFlags ATTRIBUTE_UNUSED) +testCheckExclusiveFlags(int flags) { - TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE); - TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR); - TEST_EXCLUSIVE_FLAGS(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS); + virCheckFlags(FLAG_EXPECT_FAILURE | + FLAG_EXPECT_PARSE_ERROR | + FLAG_FIPS | + FLAG_STEAL_VM | + FLAG_REAL_CAPS | + FLAG_SKIP_LEGACY_CPUS | + 0, -1); + + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1); return 0; } Ján Tomko (7): tests: add a function for checking exclusive flags tests: introduce macro for qemu XML->startup XML tests: only run startup XML tests if requested tests: report errors in QEMU XML->startup XML tests tests: do not mangle real qemu caps in xml2argvtest tests: turn skipLegacyCPUs into a flag qemu: remove unnecessary virQEMUCapsFreeHostCPUModel src/qemu/qemu_capabilities.c | 25 ++------------- src/qemu/qemu_capspriv.h | 5 --- tests/qemuxml2argvtest.c | 74 ++++++++++++++++++++++++++++---------------- 3 files changed, 49 insertions(+), 55 deletions(-) -- 2.16.4
We can reject some non-sensical combinations with an error message, once we add flags for them. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 816a3055a2..65fab6c077 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -465,6 +465,18 @@ testCompareXMLToStartupXML(const void *data) } +static int +testCheckExclusiveFlags(int flags) +{ + virCheckFlags(FLAG_EXPECT_FAILURE | + FLAG_EXPECT_PARSE_ERROR | + FLAG_FIPS | + 0, -1); + + return 0; +} + + static int testCompareXMLToArgv(const void *data) { @@ -507,6 +519,9 @@ testCompareXMLToArgv(const void *data) if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) flags |= FLAG_FIPS; + if (testCheckExclusiveFlags(info->flags) < 0) + goto cleanup; + if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0) goto cleanup; -- 2.16.4
Use this macro to indicate the intention to also run the XML->startup XML test. It sets the newly introduced FLAG_STEAL_VM flag, which is the new witness for the XML->argv test to leave the VM object behind. This will allow us to report proper errors in XML->startup tests. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 65fab6c077..0c82540ced 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -293,6 +293,7 @@ typedef enum { FLAG_EXPECT_FAILURE = 1 << 0, FLAG_EXPECT_PARSE_ERROR = 1 << 1, FLAG_FIPS = 1 << 2, + FLAG_STEAL_VM = 1 << 3, } virQemuXML2ArgvTestFlags; struct testInfo { @@ -471,6 +472,7 @@ testCheckExclusiveFlags(int flags) virCheckFlags(FLAG_EXPECT_FAILURE | FLAG_EXPECT_PARSE_ERROR | FLAG_FIPS | + FLAG_STEAL_VM | 0, -1); return 0; @@ -643,7 +645,7 @@ testCompareXMLToArgv(const void *data) ret = 0; } - if (!(flags & FLAG_EXPECT_FAILURE) && ret == 0) + if (flags & FLAG_STEAL_VM) VIR_STEAL_PTR(info->vm, vm); cleanup: @@ -853,6 +855,9 @@ mymain(void) # define DO_TEST_GIC(name, gic, ...) \ DO_TEST_FULL(name, NULL, -1, 0, 0, gic, __VA_ARGS__) +# define DO_TEST_WITH_STARTUP(name, ...) \ + DO_TEST_FULL(name, NULL, -1, FLAG_STEAL_VM, 0, GIC_NONE, __VA_ARGS__) + # define DO_TEST_FAILURE(name, ...) \ DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, \ 0, GIC_NONE, __VA_ARGS__) @@ -1092,7 +1097,7 @@ mymain(void) DO_TEST_PARSE_ERROR("disk-fmt-cow", NONE); DO_TEST_PARSE_ERROR("disk-fmt-dir", NONE); DO_TEST_PARSE_ERROR("disk-fmt-iso", NONE); - DO_TEST("disk-shared", NONE); + DO_TEST_WITH_STARTUP("disk-shared", NONE); DO_TEST_CAPS_VER("disk-shared", "2.12.0"); DO_TEST_CAPS_LATEST("disk-shared"); DO_TEST_PARSE_ERROR("disk-shared-qcow", NONE); -- 2.16.4
Use the recently introduced flag as a witness. This reduces the apparent number of test cases to the real number of test cases. Note that this does not suffer from the same problem as commit 70255fa was fixing, because the condition for running virTestRun does not depend on results of previous tests. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0c82540ced..ac6df86618 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -475,6 +475,8 @@ testCheckExclusiveFlags(int flags) FLAG_STEAL_VM | 0, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1); return 0; } @@ -842,7 +844,8 @@ mymain(void) if (virTestRun("QEMU XML-2-ARGV " name, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ - if (virTestRun("QEMU XML-2-startup-XML " name, \ + if (((flags) & FLAG_STEAL_VM) && \ + virTestRun("QEMU XML-2-startup-XML " name, \ testCompareXMLToStartupXML, &info) < 0) \ ret = -1; \ virObjectUnref(info.qemuCaps); \ -- 2.16.4
Now that the function is only run if requested by the FLAG_STEAL_VM flag, we know that missing data is an error, not a request to skip the test. The existence of the output file is now checked by virTestCompareToFile, which allows usage of the VIR_TEST_REGENERATE_OUTPUT=1 env variable to generate new test cases. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ac6df86618..fe6e38e61a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -442,18 +442,15 @@ testCompareXMLToStartupXML(const void *data) char *actual = NULL; int ret = -1; - if (!info->vm) - return EXIT_AM_SKIP; + if (!info->vm) { + VIR_TEST_DEBUG("VM object missing. Did the args conversion succeed?"); + return -1; + } if (virAsprintf(&xml, "%s/qemuxml2startupxmloutdata/%s.xml", abs_srcdir, info->name) < 0) goto cleanup; - if (!virFileExists(xml)) { - ret = EXIT_AM_SKIP; - goto cleanup; - } - if (!(actual = virDomainDefFormat(info->vm->def, NULL, format_flags))) goto cleanup; -- 2.16.4
None of the things testUpdateQEMUCaps adjusts are applicable for tests that use the DO_TEST_CAPS macros, i.e. real QEMU capabilities parsed from the XML files: The architecture must be chosen before we even open the caps file, CPU models are already present and the expensive HostModel computation was already done in virQEMUCapsLoadCache. Introduce FLAG_REAL_CAPS and skip the whole testUpdateQEMUCaps function for DO_TEST_CAPS. This speeds up the test by 25 % Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fe6e38e61a..1318ac78f0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -294,6 +294,7 @@ typedef enum { FLAG_EXPECT_PARSE_ERROR = 1 << 1, FLAG_FIPS = 1 << 2, FLAG_STEAL_VM = 1 << 3, + FLAG_REAL_CAPS = 1 << 4, } virQemuXML2ArgvTestFlags; struct testInfo { @@ -470,6 +471,7 @@ testCheckExclusiveFlags(int flags) FLAG_EXPECT_PARSE_ERROR | FLAG_FIPS | FLAG_STEAL_VM | + FLAG_REAL_CAPS | 0, -1); VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1); @@ -566,7 +568,8 @@ testCompareXMLToArgv(const void *data) if (qemuProcessPrepareMonitorChr(&monitor_chr, priv->libDir) < 0) goto cleanup; - if (testUpdateQEMUCaps(info, vm, driver.caps) < 0) + if (!(info->flags & FLAG_REAL_CAPS) && + testUpdateQEMUCaps(info, vm, driver.caps) < 0) goto cleanup; log = virTestLogContentAndReset(); @@ -783,7 +786,7 @@ mymain(void) do { \ static struct testInfo info = { \ name, "." suffix, NULL, migrateFrom, migrateFrom ? 7 : -1,\ - (flags), parseFlags, false, NULL \ + (flags | FLAG_REAL_CAPS), parseFlags, false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ -- 2.16.4
Make it obvious when it is used intentionally and error out when used in combination with real capabilities. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemuxml2argvtest.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1318ac78f0..166f94575f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -295,6 +295,7 @@ typedef enum { FLAG_FIPS = 1 << 2, FLAG_STEAL_VM = 1 << 3, FLAG_REAL_CAPS = 1 << 4, + FLAG_SKIP_LEGACY_CPUS = 1 << 5, } virQemuXML2ArgvTestFlags; struct testInfo { @@ -305,7 +306,6 @@ struct testInfo { int migrateFd; unsigned int flags; unsigned int parseFlags; - bool skipLegacyCPUs; virDomainObjPtr vm; }; @@ -414,7 +414,8 @@ testUpdateQEMUCaps(const struct testInfo *info, virQEMUCapsInitQMPBasicArch(info->qemuCaps); - if (testAddCPUModels(info->qemuCaps, info->skipLegacyCPUs) < 0) + if (testAddCPUModels(info->qemuCaps, + !!(info->flags & FLAG_SKIP_LEGACY_CPUS)) < 0) goto cleanup; virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch, @@ -472,10 +473,12 @@ testCheckExclusiveFlags(int flags) FLAG_FIPS | FLAG_STEAL_VM | FLAG_REAL_CAPS | + FLAG_SKIP_LEGACY_CPUS | 0, -1); VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1); VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1); return 0; } @@ -672,7 +675,6 @@ mymain(void) { int ret = 0, i; char *fakerootdir; - bool skipLegacyCPUs = false; const char *archs[] = { "aarch64", "ppc64", @@ -786,9 +788,8 @@ mymain(void) do { \ static struct testInfo info = { \ name, "." suffix, NULL, migrateFrom, migrateFrom ? 7 : -1,\ - (flags | FLAG_REAL_CAPS), parseFlags, false, NULL \ + (flags | FLAG_REAL_CAPS), parseFlags, NULL \ }; \ - info.skipLegacyCPUs = skipLegacyCPUs; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ return EXIT_FAILURE; \ @@ -835,9 +836,8 @@ mymain(void) do { \ static struct testInfo info = { \ name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ - false, NULL \ + NULL \ }; \ - info.skipLegacyCPUs = skipLegacyCPUs; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ virQEMUCapsSetList(info.qemuCaps, __VA_ARGS__, QEMU_CAPS_LAST); \ @@ -1748,10 +1748,12 @@ mymain(void) DO_TEST("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("cpu-host-model", NONE); DO_TEST("cpu-host-model-vendor", NONE); - skipLegacyCPUs = true; - DO_TEST("cpu-host-model-fallback", NONE); - DO_TEST_FAILURE("cpu-host-model-nofallback", NONE); - skipLegacyCPUs = false; + DO_TEST_FULL("cpu-host-model-fallback", NULL, -1, + FLAG_SKIP_LEGACY_CPUS, 0, + GIC_NONE, NONE); + DO_TEST_FULL("cpu-host-model-nofallback", NULL, -1, + FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, + 0, GIC_NONE, NONE); DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM); DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM); -- 2.16.4
After removing the host CPU model re-computation, this function is no longer necessary. This reverts commits: commit d0498881a04dddd772f9f63b03de80fb4c33d090 virQEMUCapsFreeHostCPUModel: Don't always free host cpuData commit 5276ec712a44b3680569a096e8fe56a925f0d495 testUpdateQEMUCaps: Don't leak host cpuData Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 25 ++----------------------- src/qemu/qemu_capspriv.h | 5 ----- tests/qemuxml2argvtest.c | 5 ----- 3 files changed, 2 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 308f5602ca..7e6d8a71b7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1526,19 +1526,12 @@ virQEMUCapsHostCPUDataCopy(virQEMUCapsHostCPUDataPtr dst, static void -virQEMUCapsHostCPUDataClearModels(virQEMUCapsHostCPUDataPtr cpuData) +virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData) { + qemuMonitorCPUModelInfoFree(cpuData->info); virCPUDefFree(cpuData->reported); virCPUDefFree(cpuData->migratable); virCPUDefFree(cpuData->full); -} - - -static void -virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData) -{ - qemuMonitorCPUModelInfoFree(cpuData->info); - virQEMUCapsHostCPUDataClearModels(cpuData); memset(cpuData, 0, sizeof(*cpuData)); } @@ -2986,20 +2979,6 @@ virQEMUCapsNewHostCPUModel(void) } -void -virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps, - virArch hostArch, - virDomainVirtType type) -{ - virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type); - - if (!virQEMUCapsGuestIsNative(hostArch, qemuCaps->arch)) - return; - - virQEMUCapsHostCPUDataClearModels(cpuData); -} - - void virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, virArch hostArch, diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index cb5e0dd9a9..8d1a40fe74 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -56,11 +56,6 @@ void virQEMUCapsSetArch(virQEMUCapsPtr qemuCaps, virArch arch); -void -virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps, - virArch hostArch, - virDomainVirtType type); - void virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, virArch hostArch, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 166f94575f..0e43320b96 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -418,11 +418,6 @@ testUpdateQEMUCaps(const struct testInfo *info, !!(info->flags & FLAG_SKIP_LEGACY_CPUS)) < 0) goto cleanup; - virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch, - VIR_DOMAIN_VIRT_KVM); - virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch, - VIR_DOMAIN_VIRT_QEMU); - virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch, VIR_DOMAIN_VIRT_KVM); virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch, -- 2.16.4
On 09/12/2018 03:30 PM, Ján Tomko wrote:
Patch 1 and patch 3 were not acked in v1. The changes to the ACKed ones are minimal. Complete diff to v2:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 867e0d569f..0e43320b96 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -460,20 +460,20 @@ testCompareXMLToStartupXML(const void *data) }
-# define TEST_EXCLUSIVE_FLAGS(FLAG1, FLAG2) \ - if ((testFlags & FLAG1) && (testFlags & FLAG2)) { \ - VIR_TEST_DEBUG("Flags %s and %s are mutually exclusive\n", \ - #FLAG1, #FLAG2); \ - return -1; \ - } - - static int -testCheckExclusiveFlags(int testFlags ATTRIBUTE_UNUSED) +testCheckExclusiveFlags(int flags) { - TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE); - TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR); - TEST_EXCLUSIVE_FLAGS(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS); + virCheckFlags(FLAG_EXPECT_FAILURE | + FLAG_EXPECT_PARSE_ERROR | + FLAG_FIPS | + FLAG_STEAL_VM | + FLAG_REAL_CAPS | + FLAG_SKIP_LEGACY_CPUS | + 0, -1); + + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1); + VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1); return 0; }
Ján Tomko (7): tests: add a function for checking exclusive flags tests: introduce macro for qemu XML->startup XML tests: only run startup XML tests if requested tests: report errors in QEMU XML->startup XML tests tests: do not mangle real qemu caps in xml2argvtest tests: turn skipLegacyCPUs into a flag qemu: remove unnecessary virQEMUCapsFreeHostCPUModel
src/qemu/qemu_capabilities.c | 25 ++------------- src/qemu/qemu_capspriv.h | 5 --- tests/qemuxml2argvtest.c | 74 ++++++++++++++++++++++++++++---------------- 3 files changed, 49 insertions(+), 55 deletions(-)
ACK series. Michal
participants (2)
-
Ján Tomko -
Michal Privoznik