[libvirt] [PATCH 00/12] tests: qemuxml2xml: add DO_TEST_CAPS*
This series moves testInfoSetArgs and friends from qemuxml2argvtest.c into testutilsqemu.c, so we can use them in qemuxml2xml and make use of the CAPS handling. It will also make things easier to test xml2xml failures for example if we wanted to. Cole Robinson (12): tests: qemuxml2xml: Break out testInfoSet*Paths tests: qemuxml2xml: Add info->{in,out}file tests: qemuxml2xml: Remove info->outActiveName tests: qemuxml2argv: Add info->{in,out}file tests: qemuxml2argv: add testInfoSetPaths tests: qemuxml2argv: Rename testInfo* to testQemuInfo* tests: Move testQemuInfo* to testutilsqemu tests: add testQemuGetCapsLatest tests: qemuxml2xml: Use struct testQemuInfo tests: qemuxml2xml: Use testQemuInfoSetArgs tests: qemuxml2xml: make GIC handling optional tests: qemuxml2xml: Add DO_TEST_CAPS* tests/qemuxml2argvdata/vhost-vsock.xml | 2 +- tests/qemuxml2argvtest.c | 238 ++----------- .../aarch64-os-firmware-efi.xml | 32 +- tests/qemuxml2xmloutdata/os-firmware-bios.xml | 69 +++- .../os-firmware-efi-secboot.xml | 69 +++- tests/qemuxml2xmloutdata/os-firmware-efi.xml | 69 +++- tests/qemuxml2xmltest.c | 313 ++++++++---------- tests/testutilsqemu.c | 181 ++++++++++ tests/testutilsqemu.h | 40 +++ 9 files changed, 618 insertions(+), 395 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-bios.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi.xml -- 2.21.0
These will need to be separate to share testInfo with qemuxml2argv Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 83a0d1cf7b..2dfa9e628b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -129,16 +129,11 @@ testInfoSetCommon(struct testInfo *info, return -1; } - static int -testInfoSet(struct testInfo *info, - const char *name, - int when, - int gic) +testInfoSetPaths(struct testInfo *info, + const char *name, + int when) { - if (testInfoSetCommon(info, gic) < 0) - return -1; - if (virAsprintf(&info->inName, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, name) < 0) goto error; @@ -186,13 +181,9 @@ testInfoSet(struct testInfo *info, static const char *statusPath = abs_srcdir "/qemustatusxml2xmldata/"; static int -testInfoSetStatus(struct testInfo *info, - const char *name, - int gic) +testInfoSetStatusPaths(struct testInfo *info, + const char *name) { - if (testInfoSetCommon(info, gic) < 0) - return -1; - if (virAsprintf(&info->inName, "%s%s-in.xml", statusPath, name) < 0 || virAsprintf(&info->outActiveName, "%s%s-out.xml", statusPath, name) < 0) goto error; @@ -236,7 +227,8 @@ mymain(void) # define DO_TEST_FULL(name, when, gic, ...) \ do { \ - if (testInfoSet(&info, name, when, gic) < 0) { \ + if (testInfoSetCommon(&info, gic) < 0 || \ + testInfoSetPaths(&info, name, when) < 0) { \ VIR_TEST_DEBUG("Failed to generate test data for '%s'", name); \ return -1; \ } \ @@ -1241,7 +1233,8 @@ mymain(void) # define DO_TEST_STATUS(name) \ do { \ - if (testInfoSetStatus(&info, name, GIC_NONE) < 0) { \ + if (testInfoSetCommon(&info, GIC_NONE) < 0 || \ + testInfoSetStatusPaths(&info, name) < 0) { \ VIR_TEST_DEBUG("Failed to generate status test data for '%s'", name); \ return -1; \ } \ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
These will need to be separate to share testInfo with qemuxml2argv
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Just renamed from existing inName and outActiveName Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2dfa9e628b..0ced565fbc 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -26,8 +26,8 @@ enum { }; struct testInfo { - char *inName; - char *outActiveName; + char *infile; + char *outfile; char *outInactiveName; virQEMUCapsPtr qemuCaps; @@ -40,7 +40,7 @@ testXML2XMLActive(const void *opaque) const struct testInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->inName, info->outActiveName, true, + info->infile, info->outfile, true, 0, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } @@ -51,7 +51,7 @@ testXML2XMLInactive(const void *opaque) { const struct testInfo *info = opaque; - return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName, + return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->infile, info->outInactiveName, false, 0, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); @@ -66,13 +66,13 @@ testCompareStatusXMLToXMLFiles(const void *opaque) char *actual = NULL; int ret = -1; - if (!(obj = virDomainObjParseFile(data->inName, driver.caps, driver.xmlopt, + if (!(obj = virDomainObjParseFile(data->infile, driver.caps, driver.xmlopt, VIR_DOMAIN_DEF_PARSE_STATUS | VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) { - VIR_TEST_DEBUG("\nfailed to parse '%s'\n", data->inName); + VIR_TEST_DEBUG("\nfailed to parse '%s'\n", data->infile); goto cleanup; } @@ -82,11 +82,11 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST))) { - VIR_TEST_DEBUG("\nfailed to format back '%s'\n", data->inName); + VIR_TEST_DEBUG("\nfailed to format back '%s'\n", data->infile); goto cleanup; } - if (virTestCompareToFile(actual, data->outActiveName) < 0) + if (virTestCompareToFile(actual, data->outfile) < 0) goto cleanup; ret = 0; @@ -101,8 +101,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) static void testInfoClear(struct testInfo *info) { - VIR_FREE(info->inName); - VIR_FREE(info->outActiveName); + VIR_FREE(info->infile); + VIR_FREE(info->outfile); VIR_FREE(info->outInactiveName); virObjectUnref(info->qemuCaps); @@ -134,7 +134,7 @@ testInfoSetPaths(struct testInfo *info, const char *name, int when) { - if (virAsprintf(&info->inName, "%s/qemuxml2argvdata/%s.xml", + if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, name) < 0) goto error; @@ -155,15 +155,15 @@ testInfoSetPaths(struct testInfo *info, } if (when & WHEN_ACTIVE) { - if (virAsprintf(&info->outActiveName, + if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s-active.xml", abs_srcdir, name) < 0) goto error; - if (!virFileExists(info->outActiveName)) { - VIR_FREE(info->outActiveName); + if (!virFileExists(info->outfile)) { + VIR_FREE(info->outfile); - if (virAsprintf(&info->outActiveName, + if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) goto error; @@ -184,8 +184,8 @@ static int testInfoSetStatusPaths(struct testInfo *info, const char *name) { - if (virAsprintf(&info->inName, "%s%s-in.xml", statusPath, name) < 0 || - virAsprintf(&info->outActiveName, "%s%s-out.xml", statusPath, name) < 0) + if (virAsprintf(&info->infile, "%s%s-in.xml", statusPath, name) < 0 || + virAsprintf(&info->outfile, "%s%s-out.xml", statusPath, name) < 0) goto error; return 0; @@ -240,7 +240,7 @@ mymain(void) ret = -1; \ } \ \ - if (info.outActiveName) { \ + if (info.outfile) { \ if (virTestRun("QEMU XML-2-XML-active " name, \ testXML2XMLActive, &info) < 0) \ ret = -1; \ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
Just renamed from existing inName and outActiveName
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Reuse info->outfile for it. This requires us to set paths before each virTestRun invocation Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 61 +++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0ced565fbc..538ccae8fd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -28,7 +28,6 @@ enum { struct testInfo { char *infile; char *outfile; - char *outInactiveName; virQEMUCapsPtr qemuCaps; }; @@ -40,8 +39,7 @@ testXML2XMLActive(const void *opaque) const struct testInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, true, - 0, + info->infile, info->outfile, true, 0, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } @@ -51,9 +49,8 @@ testXML2XMLInactive(const void *opaque) { const struct testInfo *info = opaque; - return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->infile, - info->outInactiveName, false, - 0, + return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, + info->infile, info->outfile, false, 0, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } @@ -103,7 +100,6 @@ testInfoClear(struct testInfo *info) { VIR_FREE(info->infile); VIR_FREE(info->outfile); - VIR_FREE(info->outInactiveName); virObjectUnref(info->qemuCaps); } @@ -134,40 +130,26 @@ testInfoSetPaths(struct testInfo *info, const char *name, int when) { + VIR_FREE(info->infile); + VIR_FREE(info->outfile); + if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, name) < 0) goto error; - if (when & WHEN_INACTIVE) { - if (virAsprintf(&info->outInactiveName, - "%s/qemuxml2xmloutdata/%s-inactive.xml", - abs_srcdir, name) < 0) - goto error; - - if (!virFileExists(info->outInactiveName)) { - VIR_FREE(info->outInactiveName); + if (virAsprintf(&info->outfile, + "%s/qemuxml2xmloutdata/%s-%s.xml", + abs_srcdir, name, + when == WHEN_ACTIVE ? "active" : "inactive") < 0) + goto error; - if (virAsprintf(&info->outInactiveName, - "%s/qemuxml2xmloutdata/%s.xml", - abs_srcdir, name) < 0) - goto error; - } - } + if (!virFileExists(info->outfile)) { + VIR_FREE(info->outfile); - if (when & WHEN_ACTIVE) { if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-active.xml", + "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) goto error; - - if (!virFileExists(info->outfile)) { - VIR_FREE(info->outfile); - - if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s.xml", - abs_srcdir, name) < 0) - goto error; - } } return 0; @@ -227,20 +209,27 @@ mymain(void) # define DO_TEST_FULL(name, when, gic, ...) \ do { \ - if (testInfoSetCommon(&info, gic) < 0 || \ - testInfoSetPaths(&info, name, when) < 0) { \ + if (testInfoSetCommon(&info, gic) < 0) { \ VIR_TEST_DEBUG("Failed to generate test data for '%s'", name); \ return -1; \ } \ virQEMUCapsSetList(info.qemuCaps, __VA_ARGS__, QEMU_CAPS_LAST); \ \ - if (info.outInactiveName) { \ + if (when & WHEN_INACTIVE) { \ + if (testInfoSetPaths(&info, name, WHEN_INACTIVE) < 0) { \ + VIR_TEST_DEBUG("Failed to generate inactive paths for '%s'", name); \ + return -1; \ + } \ if (virTestRun("QEMU XML-2-XML-inactive " name, \ testXML2XMLInactive, &info) < 0) \ ret = -1; \ } \ \ - if (info.outfile) { \ + if (when & WHEN_ACTIVE) { \ + if (testInfoSetPaths(&info, name, WHEN_ACTIVE) < 0) { \ + VIR_TEST_DEBUG("Failed to generate active paths for '%s'", name); \ + return -1; \ + } \ if (virTestRun("QEMU XML-2-XML-active " name, \ testXML2XMLActive, &info) < 0) \ ret = -1; \ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
@@ -40,8 +39,7 @@ testXML2XMLActive(const void *opaque) const struct testInfo *info = opaque;
return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, true, - 0, + info->infile, info->outfile, true, 0, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
Unrelated formatting change. Squash this hunk into the previous commit. [...]
@@ -134,40 +130,26 @@ testInfoSetPaths(struct testInfo *info, const char *name, int when) { + VIR_FREE(info->infile); + VIR_FREE(info->outfile);
It's a tad wasteful to free() info->infile just to generate an identical string immediately afterwards, but in the grand scheme of things I guess it's barely even a rounding error :) With the comment above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Track infile and outfile in testInfo. This is step towards moving path creation out of the test case, which will eventually help sharing more code with qemuxml2xmltest.c Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0c0dcae197..ff7bacf8db 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -299,6 +299,8 @@ typedef enum { struct testInfo { const char *name; const char *suffix; + char *infile; + char *outfile; virQEMUCapsPtr qemuCaps; const char *migrateFrom; int migrateFd; @@ -427,8 +429,6 @@ static int testCompareXMLToArgv(const void *data) { struct testInfo *info = (void *) data; - char *xml = NULL; - char *args = NULL; char *migrateURI = NULL; char *actualargv = NULL; const char *suffix = info->suffix; @@ -471,9 +471,9 @@ testCompareXMLToArgv(const void *data) if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0) goto cleanup; - if (virAsprintf(&xml, "%s/qemuxml2argvdata/%s.xml", + if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, info->name) < 0 || - virAsprintf(&args, "%s/qemuxml2argvdata/%s%s.args", + virAsprintf(&info->outfile, "%s/qemuxml2argvdata/%s%s.args", abs_srcdir, info->name, suffix) < 0) goto cleanup; @@ -486,7 +486,8 @@ testCompareXMLToArgv(const void *data) goto cleanup; parseFlags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; - if (!(vm->def = virDomainDefParseFile(xml, driver.caps, driver.xmlopt, + if (!(vm->def = virDomainDefParseFile(info->infile, + driver.caps, driver.xmlopt, NULL, parseFlags))) { if (flags & FLAG_EXPECT_PARSE_ERROR) goto ok; @@ -502,7 +503,7 @@ testCompareXMLToArgv(const void *data) goto cleanup; if (!virDomainDefCheckABIStability(vm->def, vm->def, driver.xmlopt)) { - VIR_TEST_DEBUG("ABI stability check failed on %s", xml); + VIR_TEST_DEBUG("ABI stability check failed on %s", info->infile); goto cleanup; } @@ -570,7 +571,7 @@ testCompareXMLToArgv(const void *data) if (!(actualargv = virCommandToString(cmd, false))) goto cleanup; - if (virTestCompareToFile(actualargv, args) < 0) + if (virTestCompareToFile(actualargv, info->outfile) < 0) goto cleanup; ret = 0; @@ -600,8 +601,6 @@ testCompareXMLToArgv(const void *data) virSetConnectStorage(NULL); virObjectUnref(conn); VIR_FREE(migrateURI); - VIR_FREE(xml); - VIR_FREE(args); return ret; } @@ -754,6 +753,8 @@ testInfoSetArgs(struct testInfo *info, static void testInfoClear(struct testInfo *info) { + VIR_FREE(info->infile); + VIR_FREE(info->outfile); virObjectUnref(info->qemuCaps); } -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
Track infile and outfile in testInfo. This is step towards moving path creation out of the test case, which will eventually help sharing more code with qemuxml2xmltest.c
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This moves infile and outfile building outside the test case, which better fits the pattern of qemuxml2xmltest. It also lets us drop the qemuxml2argtest-specific 'suffix' from testInfo Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ff7bacf8db..a3fee41ea9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -298,7 +298,6 @@ typedef enum { struct testInfo { const char *name; - const char *suffix; char *infile; char *outfile; virQEMUCapsPtr qemuCaps; @@ -431,7 +430,6 @@ testCompareXMLToArgv(const void *data) struct testInfo *info = (void *) data; char *migrateURI = NULL; char *actualargv = NULL; - const char *suffix = info->suffix; unsigned int flags = info->flags; unsigned int parseFlags = info->parseFlags; int ret = -1; @@ -448,9 +446,6 @@ testCompareXMLToArgv(const void *data) if (!(conn = virGetConnect())) goto cleanup; - if (!suffix) - suffix = ""; - conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver; conn->nwfilterDriver = &fakeNWFilterDriver; @@ -471,12 +466,6 @@ testCompareXMLToArgv(const void *data) if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0) goto cleanup; - if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", - abs_srcdir, info->name) < 0 || - virAsprintf(&info->outfile, "%s/qemuxml2argvdata/%s%s.args", - abs_srcdir, info->name, suffix) < 0) - goto cleanup; - if (info->migrateFrom && !(migrateURI = qemuMigrationDstGetURI(info->migrateFrom, info->migrateFd))) @@ -758,6 +747,17 @@ testInfoClear(struct testInfo *info) virObjectUnref(info->qemuCaps); } +static int +testInfoSetPaths(struct testInfo *info, const char *suffix) +{ + if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&info->outfile, "%s/qemuxml2argvdata/%s%s.args", + abs_srcdir, info->name, suffix ? suffix : "") < 0) + return -1; + return 0; +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -883,11 +883,12 @@ mymain(void) do { \ static struct testInfo info = { \ .name = _name, \ - .suffix = _suffix, \ }; \ if (testInfoSetArgs(&info, capslatest, \ __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ + if (testInfoSetPaths(&info, _suffix)) \ + return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " _name _suffix, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
@@ -758,6 +747,17 @@ testInfoClear(struct testInfo *info) virObjectUnref(info->qemuCaps); }
+static int +testInfoSetPaths(struct testInfo *info, const char *suffix)
One argument per line.
+{ + if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&info->outfile, "%s/qemuxml2argvdata/%s%s.args", + abs_srcdir, info->name, suffix ? suffix : "") < 0) + return -1;
This is very ugly. Please at least put curly braces around the if() body and leave an empty line after it. [...]
@@ -883,11 +883,12 @@ mymain(void) do { \ static struct testInfo info = { \ .name = _name, \ - .suffix = _suffix, \ }; \ if (testInfoSetArgs(&info, capslatest, \ __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ + if (testInfoSetPaths(&info, _suffix)) \ + return EXIT_FAILURE; \
You should check whether testInfoSetPaths() returns a negative value. Also in xml2xml you print out a nice message with VIR_TEST_DEBUG() when the function fails: you should do the same here as well. With the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
In preparation for moving these bits to a shared place, rename them to match one of the testqemuutils.c function prefixes. Rename info->flags handling too as it will need to be moved testInfoSetPaths isn't renamed because it will stay local Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a3fee41ea9..b87d2e3fb9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -294,9 +294,9 @@ typedef enum { FLAG_FIPS = 1 << 2, FLAG_REAL_CAPS = 1 << 3, FLAG_SKIP_LEGACY_CPUS = 1 << 4, -} virQemuXML2ArgvTestFlags; +} testQemuInfoFlags; -struct testInfo { +struct testQemuInfo { const char *name; char *infile; char *outfile; @@ -380,7 +380,7 @@ testAddCPUModels(virQEMUCapsPtr caps, bool skipLegacy) static int -testUpdateQEMUCaps(const struct testInfo *info, +testUpdateQEMUCaps(const struct testQemuInfo *info, virDomainObjPtr vm, virCapsPtr caps) { @@ -410,7 +410,7 @@ testUpdateQEMUCaps(const struct testInfo *info, static int -testCheckExclusiveFlags(int flags) +testQemuInfoCheckExclusiveFlags(int flags) { virCheckFlags(FLAG_EXPECT_FAILURE | FLAG_EXPECT_PARSE_ERROR | @@ -427,7 +427,7 @@ testCheckExclusiveFlags(int flags) static int testCompareXMLToArgv(const void *data) { - struct testInfo *info = (void *) data; + struct testQemuInfo *info = (void *) data; char *migrateURI = NULL; char *actualargv = NULL; unsigned int flags = info->flags; @@ -460,7 +460,7 @@ testCompareXMLToArgv(const void *data) if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) flags |= FLAG_FIPS; - if (testCheckExclusiveFlags(info->flags) < 0) + if (testQemuInfoCheckExclusiveFlags(info->flags) < 0) goto cleanup; if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0) @@ -605,14 +605,14 @@ typedef enum { ARG_CAPS_ARCH, ARG_CAPS_VER, ARG_END, -} testInfoArgName; +} testQemuInfoArgName; static int -testInfoSetArgs(struct testInfo *info, - virHashTablePtr capslatest, ...) +testQemuInfoSetArgs(struct testQemuInfo *info, + virHashTablePtr capslatest, ...) { va_list argptr; - testInfoArgName argname; + testQemuInfoArgName argname; virQEMUCapsPtr qemuCaps = NULL; int gic = GIC_NONE; char *capsarch = NULL; @@ -622,7 +622,7 @@ testInfoSetArgs(struct testInfo *info, int ret = -1; va_start(argptr, capslatest); - argname = va_arg(argptr, testInfoArgName); + argname = va_arg(argptr, testQemuInfoArgName); while (argname != ARG_END) { switch (argname) { case ARG_QEMU_CAPS: @@ -684,7 +684,7 @@ testInfoSetArgs(struct testInfo *info, goto cleanup; } - argname = va_arg(argptr, testInfoArgName); + argname = va_arg(argptr, testQemuInfoArgName); } if (!!capsarch ^ !!capsver) { @@ -740,7 +740,7 @@ testInfoSetArgs(struct testInfo *info, } static void -testInfoClear(struct testInfo *info) +testQemuInfoClear(struct testQemuInfo *info) { VIR_FREE(info->infile); VIR_FREE(info->outfile); @@ -748,7 +748,7 @@ testInfoClear(struct testInfo *info) } static int -testInfoSetPaths(struct testInfo *info, const char *suffix) +testInfoSetPaths(struct testQemuInfo *info, const char *suffix) { if (virAsprintf(&info->infile, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, info->name) < 0 || @@ -881,18 +881,18 @@ mymain(void) */ # define DO_TEST_INTERNAL(_name, _suffix, ...) \ do { \ - static struct testInfo info = { \ + static struct testQemuInfo info = { \ .name = _name, \ }; \ - if (testInfoSetArgs(&info, capslatest, \ - __VA_ARGS__, ARG_END) < 0) \ + if (testQemuInfoSetArgs(&info, capslatest, \ + __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ if (testInfoSetPaths(&info, _suffix)) \ return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " _name _suffix, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ - testInfoClear(&info); \ + testQemuInfoClear(&info); \ } while (0) # define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
In preparation for moving these bits to a shared place, rename them to match one of the testqemuutils.c function prefixes. Rename
s/testqemuutils/testutilsqemu/ [...]
static int -testCheckExclusiveFlags(int flags) +testQemuInfoCheckExclusiveFlags(int flags)
According to the logic used in the commit message to justify not renaming testInfoSetPaths(), this one should either keep its name as well or be moved to testutilsqemu. I would go for the former right now, but it would be neat if you explored whether calling it from xml2xml too would make sense, for a follow-up patch. Everything else looks good, so assuming that you don't rename the function Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
So it can eventually be shared with qemuxml2xml Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 173 --------------------------------------- tests/testutilsqemu.c | 144 ++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 39 +++++++++ 3 files changed, 183 insertions(+), 173 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b87d2e3fb9..a51bdb2453 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -288,25 +288,6 @@ static virNWFilterDriver fakeNWFilterDriver = { .nwfilterBindingDelete = fakeNWFilterBindingDelete, }; -typedef enum { - FLAG_EXPECT_FAILURE = 1 << 0, - FLAG_EXPECT_PARSE_ERROR = 1 << 1, - FLAG_FIPS = 1 << 2, - FLAG_REAL_CAPS = 1 << 3, - FLAG_SKIP_LEGACY_CPUS = 1 << 4, -} testQemuInfoFlags; - -struct testQemuInfo { - const char *name; - char *infile; - char *outfile; - virQEMUCapsPtr qemuCaps; - const char *migrateFrom; - int migrateFd; - unsigned int flags; - unsigned int parseFlags; -}; - static int testAddCPUModels(virQEMUCapsPtr caps, bool skipLegacy) @@ -593,160 +574,6 @@ testCompareXMLToArgv(const void *data) return ret; } -# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata" - -typedef enum { - ARG_QEMU_CAPS, - ARG_GIC, - ARG_MIGRATE_FROM, - ARG_MIGRATE_FD, - ARG_FLAGS, - ARG_PARSEFLAGS, - ARG_CAPS_ARCH, - ARG_CAPS_VER, - ARG_END, -} testQemuInfoArgName; - -static int -testQemuInfoSetArgs(struct testQemuInfo *info, - virHashTablePtr capslatest, ...) -{ - va_list argptr; - testQemuInfoArgName argname; - virQEMUCapsPtr qemuCaps = NULL; - int gic = GIC_NONE; - char *capsarch = NULL; - char *capsver = NULL; - VIR_AUTOFREE(char *) capsfile = NULL; - int flag; - int ret = -1; - - va_start(argptr, capslatest); - argname = va_arg(argptr, testQemuInfoArgName); - while (argname != ARG_END) { - switch (argname) { - case ARG_QEMU_CAPS: - if (qemuCaps || !(qemuCaps = virQEMUCapsNew())) - goto cleanup; - - while ((flag = va_arg(argptr, int)) < QEMU_CAPS_LAST) - virQEMUCapsSet(qemuCaps, flag); - - /* Some tests are run with NONE capabilities, which is just - * another name for QEMU_CAPS_LAST. If that is the case the - * arguments look like this : - * - * ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END - * - * Fetch one argument more and if it is QEMU_CAPS_LAST then - * break from the switch() to force getting next argument - * in the line. If it is not QEMU_CAPS_LAST then we've - * fetched real ARG_* and we must process it. - */ - if ((flag = va_arg(argptr, int)) != QEMU_CAPS_LAST) { - argname = flag; - continue; - } - - break; - - case ARG_GIC: - gic = va_arg(argptr, int); - break; - - case ARG_MIGRATE_FROM: - info->migrateFrom = va_arg(argptr, char *); - break; - - case ARG_MIGRATE_FD: - info->migrateFd = va_arg(argptr, int); - break; - - case ARG_FLAGS: - info->flags = va_arg(argptr, int); - break; - - case ARG_PARSEFLAGS: - info->parseFlags = va_arg(argptr, int); - break; - - case ARG_CAPS_ARCH: - capsarch = va_arg(argptr, char *); - break; - - case ARG_CAPS_VER: - capsver = va_arg(argptr, char *); - break; - - case ARG_END: - default: - fprintf(stderr, "Unexpected test info argument"); - goto cleanup; - } - - argname = va_arg(argptr, testQemuInfoArgName); - } - - if (!!capsarch ^ !!capsver) { - fprintf(stderr, "ARG_CAPS_ARCH and ARG_CAPS_VER " - "must be specified together.\n"); - goto cleanup; - } - - if (qemuCaps && (capsarch || capsver)) { - fprintf(stderr, "ARG_QEMU_CAPS can not be combined with ARG_CAPS_ARCH " - "or ARG_CAPS_VER\n"); - goto cleanup; - } - - if (!qemuCaps && capsarch && capsver) { - bool stripmachinealiases = false; - - if (STREQ(capsver, "latest")) { - if (VIR_STRDUP(capsfile, virHashLookup(capslatest, capsarch)) < 0) - goto cleanup; - stripmachinealiases = true; - } else if (virAsprintf(&capsfile, "%s/caps_%s.%s.xml", - TEST_CAPS_PATH, capsver, capsarch) < 0) { - goto cleanup; - } - - if (!(qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(capsarch), - capsfile))) { - goto cleanup; - } - - if (stripmachinealiases) - virQEMUCapsStripMachineAliases(qemuCaps); - info->flags |= FLAG_REAL_CAPS; - } - - if (!qemuCaps) { - fprintf(stderr, "No qemuCaps generated\n"); - goto cleanup; - } - VIR_STEAL_PTR(info->qemuCaps, qemuCaps); - - if (gic != GIC_NONE && testQemuCapsSetGIC(info->qemuCaps, gic) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virObjectUnref(qemuCaps); - va_end(argptr); - - return ret; -} - -static void -testQemuInfoClear(struct testQemuInfo *info) -{ - VIR_FREE(info->infile); - VIR_FREE(info->outfile); - virObjectUnref(info->qemuCaps); -} - static int testInfoSetPaths(struct testQemuInfo *info, const char *suffix) { diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index dffe473944..6286c7b3c7 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -918,3 +918,147 @@ testQemuCapsIterate(const char *dirname, return ret; } + + +#define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata" + +int +testQemuInfoSetArgs(struct testQemuInfo *info, + virHashTablePtr capslatest, ...) +{ + va_list argptr; + testQemuInfoArgName argname; + virQEMUCapsPtr qemuCaps = NULL; + int gic = GIC_NONE; + char *capsarch = NULL; + char *capsver = NULL; + VIR_AUTOFREE(char *) capsfile = NULL; + int flag; + int ret = -1; + + va_start(argptr, capslatest); + argname = va_arg(argptr, testQemuInfoArgName); + while (argname != ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + if (qemuCaps || !(qemuCaps = virQEMUCapsNew())) + goto cleanup; + + while ((flag = va_arg(argptr, int)) < QEMU_CAPS_LAST) + virQEMUCapsSet(qemuCaps, flag); + + /* Some tests are run with NONE capabilities, which is just + * another name for QEMU_CAPS_LAST. If that is the case the + * arguments look like this : + * + * ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END + * + * Fetch one argument more and if it is QEMU_CAPS_LAST then + * break from the switch() to force getting next argument + * in the line. If it is not QEMU_CAPS_LAST then we've + * fetched real ARG_* and we must process it. + */ + if ((flag = va_arg(argptr, int)) != QEMU_CAPS_LAST) { + argname = flag; + continue; + } + + break; + + case ARG_GIC: + gic = va_arg(argptr, int); + break; + + case ARG_MIGRATE_FROM: + info->migrateFrom = va_arg(argptr, char *); + break; + + case ARG_MIGRATE_FD: + info->migrateFd = va_arg(argptr, int); + break; + + case ARG_FLAGS: + info->flags = va_arg(argptr, int); + break; + + case ARG_PARSEFLAGS: + info->parseFlags = va_arg(argptr, int); + break; + + case ARG_CAPS_ARCH: + capsarch = va_arg(argptr, char *); + break; + + case ARG_CAPS_VER: + capsver = va_arg(argptr, char *); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument"); + goto cleanup; + } + + argname = va_arg(argptr, testQemuInfoArgName); + } + + if (!!capsarch ^ !!capsver) { + fprintf(stderr, "ARG_CAPS_ARCH and ARG_CAPS_VER " + "must be specified together.\n"); + goto cleanup; + } + + if (qemuCaps && (capsarch || capsver)) { + fprintf(stderr, "ARG_QEMU_CAPS can not be combined with ARG_CAPS_ARCH " + "or ARG_CAPS_VER\n"); + goto cleanup; + } + + if (!qemuCaps && capsarch && capsver) { + bool stripmachinealiases = false; + + if (STREQ(capsver, "latest")) { + if (VIR_STRDUP(capsfile, virHashLookup(capslatest, capsarch)) < 0) + goto cleanup; + stripmachinealiases = true; + } else if (virAsprintf(&capsfile, "%s/caps_%s.%s.xml", + TEST_CAPS_PATH, capsver, capsarch) < 0) { + goto cleanup; + } + + if (!(qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(capsarch), + capsfile))) { + goto cleanup; + } + + if (stripmachinealiases) + virQEMUCapsStripMachineAliases(qemuCaps); + info->flags |= FLAG_REAL_CAPS; + } + + if (!qemuCaps) { + fprintf(stderr, "No qemuCaps generated\n"); + goto cleanup; + } + VIR_STEAL_PTR(info->qemuCaps, qemuCaps); + + if (gic != GIC_NONE && testQemuCapsSetGIC(info->qemuCaps, gic) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(qemuCaps); + va_end(argptr); + + return ret; +} + + +void +testQemuInfoClear(struct testQemuInfo *info) +{ + VIR_FREE(info->infile); + VIR_FREE(info->outfile); + virObjectUnref(info->qemuCaps); +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 183ce915f1..f6ae2a38d3 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -32,6 +32,41 @@ enum { GIC_BOTH, }; + +typedef enum { + ARG_QEMU_CAPS, + ARG_GIC, + ARG_MIGRATE_FROM, + ARG_MIGRATE_FD, + ARG_FLAGS, + ARG_PARSEFLAGS, + ARG_CAPS_ARCH, + ARG_CAPS_VER, + ARG_END, +} testQemuInfoArgName; + + +typedef enum { + FLAG_EXPECT_FAILURE = 1 << 0, + FLAG_EXPECT_PARSE_ERROR = 1 << 1, + FLAG_FIPS = 1 << 2, + FLAG_REAL_CAPS = 1 << 3, + FLAG_SKIP_LEGACY_CPUS = 1 << 4, +} testQemuInfoFlags; + + +struct testQemuInfo { + const char *name; + char *infile; + char *outfile; + virQEMUCapsPtr qemuCaps; + const char *migrateFrom; + int migrateFd; + unsigned int flags; + unsigned int parseFlags; +}; + + virCapsPtr testQemuCapsInit(void); virDomainXMLOptionPtr testQemuXMLConfInit(void); @@ -71,6 +106,10 @@ int testQemuCapsIterate(const char *dirname, testQemuCapsIterateCallback callback, void *opaque); +int testQemuInfoSetArgs(struct testQemuInfo *info, + virHashTablePtr capslatest, ...); +void testQemuInfoClear(struct testQemuInfo *info); + # endif #endif /* LIBVIRT_TESTUTILSQEMU_H */ -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
+#define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata"
Note to self: rename this to TEST_QEMU_CAPS_PATH, move it to testutilsqemu.h, and start using it throughout the test suite instead of repeating the raw string over and over again. [...]
@@ -32,6 +32,41 @@ enum { GIC_BOTH, };
+ +typedef enum { + ARG_QEMU_CAPS, + ARG_GIC, + ARG_MIGRATE_FROM, + ARG_MIGRATE_FD, + ARG_FLAGS, + ARG_PARSEFLAGS, + ARG_CAPS_ARCH, + ARG_CAPS_VER, + ARG_END, +} testQemuInfoArgName; + +
While the .c file is all over the place, the .h file is almost entirely consistent in its use of a single empty line rather than two as vertical separator. Please make sure you follow that pattern. With that addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Move the capslatest building from qemuxml2argv to testutilsqemu Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 25 ++----------------------- tests/testutilsqemu.c | 37 +++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 1 + 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a51bdb2453..b2dda3845d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -590,15 +590,8 @@ testInfoSetPaths(struct testQemuInfo *info, const char *suffix) static int mymain(void) { - int ret = 0, i; + int ret = 0; char *fakerootdir; - const char *archs[] = { - "aarch64", - "ppc64", - "riscv64", - "s390x", - "x86_64", - }; virHashTablePtr capslatest = NULL; if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { @@ -667,24 +660,10 @@ mymain(void) if (VIR_STRDUP(driver.config->nvramDir, "/var/lib/libvirt/qemu/nvram") < 0) return EXIT_FAILURE; - capslatest = virHashCreate(4, virHashValueFree); + capslatest = testQemuGetCapsLatest(); if (!capslatest) return EXIT_FAILURE; - VIR_TEST_VERBOSE("\n"); - - for (i = 0; i < ARRAY_CARDINALITY(archs); ++i) { - char *cap = testQemuGetLatestCapsForArch(abs_srcdir "/qemucapabilitiesdata", - archs[i], "xml"); - - if (!cap || virHashAddEntry(capslatest, archs[i], cap) < 0) - return EXIT_FAILURE; - - VIR_TEST_VERBOSE("latest caps for %s: %s\n", archs[i], cap); - } - - VIR_TEST_VERBOSE("\n"); - virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 6286c7b3c7..66464d4101 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -1062,3 +1062,40 @@ testQemuInfoClear(struct testQemuInfo *info) VIR_FREE(info->outfile); virObjectUnref(info->qemuCaps); } + + +virHashTablePtr +testQemuGetCapsLatest(void) +{ + const char *archs[] = { + "aarch64", + "ppc64", + "riscv64", + "s390x", + "x86_64", + }; + virHashTablePtr capslatest; + size_t i; + + if (!(capslatest = virHashCreate(4, virHashValueFree))) + goto error; + + VIR_TEST_VERBOSE("\n"); + + for (i = 0; i < ARRAY_CARDINALITY(archs); ++i) { + char *cap = testQemuGetLatestCapsForArch(abs_srcdir "/qemucapabilitiesdata", + archs[i], "xml"); + + if (!cap || virHashAddEntry(capslatest, archs[i], cap) < 0) + goto error; + + VIR_TEST_VERBOSE("latest caps for %s: %s\n", archs[i], cap); + } + + VIR_TEST_VERBOSE("\n"); + return capslatest; + + error: + virHashFree(capslatest); + return NULL; +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index f6ae2a38d3..6d4719dc12 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -109,6 +109,7 @@ int testQemuCapsIterate(const char *dirname, int testQemuInfoSetArgs(struct testQemuInfo *info, virHashTablePtr capslatest, ...); void testQemuInfoClear(struct testQemuInfo *info); +virHashTablePtr testQemuGetCapsLatest(void); # endif -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
+virHashTablePtr +testQemuGetCapsLatest(void)
A better name for this would be testQemuGetLatestCaps(), since it's literally calling testQemuGetLatestCapsForArch() over a number of architectures. Incidentally, both these functions could use TEST_QEMU_CAPS_PATH once it's introduced, and lose an hardcoded string and a parameter respectively O:-) [...]
+++ b/tests/testutilsqemu.h @@ -109,6 +109,7 @@ int testQemuCapsIterate(const char *dirname, int testQemuInfoSetArgs(struct testQemuInfo *info, virHashTablePtr capslatest, ...); void testQemuInfoClear(struct testQemuInfo *info); +virHashTablePtr testQemuGetCapsLatest(void);
Both in the source and in the header file, order this new function right after the existing testQemuGetLatestCapsForArch(). With the function named and placed appropriately, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
The qemuxml2xml testInfo is now just a subset of testQemuInfo, so it's a drop in replacement Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 538ccae8fd..ba622a3d2f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -25,18 +25,11 @@ enum { WHEN_BOTH = 3, }; -struct testInfo { - char *infile; - char *outfile; - - virQEMUCapsPtr qemuCaps; -}; - static int testXML2XMLActive(const void *opaque) { - const struct testInfo *info = opaque; + const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->infile, info->outfile, true, 0, @@ -47,7 +40,7 @@ testXML2XMLActive(const void *opaque) static int testXML2XMLInactive(const void *opaque) { - const struct testInfo *info = opaque; + const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->infile, info->outfile, false, 0, @@ -58,7 +51,7 @@ testXML2XMLInactive(const void *opaque) static int testCompareStatusXMLToXMLFiles(const void *opaque) { - const struct testInfo *data = opaque; + const struct testQemuInfo *data = opaque; virDomainObjPtr obj = NULL; char *actual = NULL; int ret = -1; @@ -95,18 +88,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) } -static void -testInfoClear(struct testInfo *info) -{ - VIR_FREE(info->infile); - VIR_FREE(info->outfile); - - virObjectUnref(info->qemuCaps); -} - - static int -testInfoSetCommon(struct testInfo *info, +testInfoSetCommon(struct testQemuInfo *info, int gic) { if (!(info->qemuCaps = virQEMUCapsNew())) @@ -121,12 +104,12 @@ testInfoSetCommon(struct testInfo *info, return 0; error: - testInfoClear(info); + testQemuInfoClear(info); return -1; } static int -testInfoSetPaths(struct testInfo *info, +testInfoSetPaths(struct testQemuInfo *info, const char *name, int when) { @@ -155,7 +138,7 @@ testInfoSetPaths(struct testInfo *info, return 0; error: - testInfoClear(info); + testQemuInfoClear(info); return -1; } @@ -163,7 +146,7 @@ testInfoSetPaths(struct testInfo *info, static const char *statusPath = abs_srcdir "/qemustatusxml2xmldata/"; static int -testInfoSetStatusPaths(struct testInfo *info, +testInfoSetStatusPaths(struct testQemuInfo *info, const char *name) { if (virAsprintf(&info->infile, "%s%s-in.xml", statusPath, name) < 0 || @@ -173,7 +156,7 @@ testInfoSetStatusPaths(struct testInfo *info, return 0; error: - testInfoClear(info); + testQemuInfoClear(info); return -1; } @@ -185,7 +168,7 @@ mymain(void) { int ret = 0; char *fakerootdir; - struct testInfo info; + struct testQemuInfo info; virQEMUDriverConfigPtr cfg = NULL; if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { @@ -234,7 +217,7 @@ mymain(void) testXML2XMLActive, &info) < 0) \ ret = -1; \ } \ - testInfoClear(&info); \ + testQemuInfoClear(&info); \ } while (0) # define NONE QEMU_CAPS_LAST @@ -1232,7 +1215,7 @@ mymain(void) testCompareStatusXMLToXMLFiles, &info) < 0) \ ret = -1; \ \ - testInfoClear(&info); \ + testQemuInfoClear(&info); \ } while (0) -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
The qemuxml2xml testInfo is now just a subset of testQemuInfo, so it's a drop in replacement
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
No functional change, just replacing the old custom infrastructure Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ba622a3d2f..e60c69872a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -88,26 +88,6 @@ testCompareStatusXMLToXMLFiles(const void *opaque) } -static int -testInfoSetCommon(struct testQemuInfo *info, - int gic) -{ - if (!(info->qemuCaps = virQEMUCapsNew())) - goto error; - - if (testQemuCapsSetGIC(info->qemuCaps, gic) < 0) - goto error; - - if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0) - goto error; - - return 0; - - error: - testQemuInfoClear(info); - return -1; -} - static int testInfoSetPaths(struct testQemuInfo *info, const char *name, @@ -170,6 +150,11 @@ mymain(void) char *fakerootdir; struct testQemuInfo info; virQEMUDriverConfigPtr cfg = NULL; + virHashTablePtr capslatest = NULL; + + capslatest = testQemuGetCapsLatest(); + if (!capslatest) + abort(); if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { fprintf(stderr, "Out of memory\n"); @@ -192,11 +177,14 @@ mymain(void) # define DO_TEST_FULL(name, when, gic, ...) \ do { \ - if (testInfoSetCommon(&info, gic) < 0) { \ + if (testQemuInfoSetArgs(&info, capslatest, \ + ARG_GIC, gic, \ + ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \ + ARG_END) < 0 || \ + qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0) { \ VIR_TEST_DEBUG("Failed to generate test data for '%s'", name); \ return -1; \ } \ - virQEMUCapsSetList(info.qemuCaps, __VA_ARGS__, QEMU_CAPS_LAST); \ \ if (when & WHEN_INACTIVE) { \ if (testInfoSetPaths(&info, name, WHEN_INACTIVE) < 0) { \ @@ -1205,7 +1193,10 @@ mymain(void) # define DO_TEST_STATUS(name) \ do { \ - if (testInfoSetCommon(&info, GIC_NONE) < 0 || \ + if (testQemuInfoSetArgs(&info, capslatest, \ + ARG_QEMU_CAPS, QEMU_CAPS_LAST, \ + ARG_END) < 0 || \ + qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0 || \ testInfoSetStatusPaths(&info, name) < 0) { \ VIR_TEST_DEBUG("Failed to generate status test data for '%s'", name); \ return -1; \ @@ -1262,6 +1253,7 @@ mymain(void) if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); + virHashFree(capslatest); qemuTestDriverFree(&driver); VIR_FREE(fakerootdir); -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
@@ -170,6 +150,11 @@ mymain(void) char *fakerootdir; struct testQemuInfo info; virQEMUDriverConfigPtr cfg = NULL; + virHashTablePtr capslatest = NULL; + + capslatest = testQemuGetCapsLatest(); + if (!capslatest) + abort();
Woah, that's a bit harsh, isn't it? How about a nice and polite return EXIT_FAILURE; instead? :) [...]
@@ -192,11 +177,14 @@ mymain(void)
# define DO_TEST_FULL(name, when, gic, ...) \ do { \ - if (testInfoSetCommon(&info, gic) < 0) { \ + if (testQemuInfoSetArgs(&info, capslatest, \ + ARG_GIC, gic, \ + ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \ + ARG_END) < 0 || \ + qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0) { \
I haven't really spent any time digging, but that call to qemuTestCapsCacheInsert() looks odd to me. What exactly is the point in caching if we're going to be using a different set of capabilities pretty much every single time? Considering it is pre-existing, of course, your change is perfectly fine. Feel free to investigate further, though :) Additional note: xml2argv calls this in the test function rather than in the corresponding macro, and it would be nice if they would be made consistent. Follow-up material, of course. With the abort() call replaced with something more gentle, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 4/10/19 12:53 PM, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
@@ -170,6 +150,11 @@ mymain(void) char *fakerootdir; struct testQemuInfo info; virQEMUDriverConfigPtr cfg = NULL; + virHashTablePtr capslatest = NULL; + + capslatest = testQemuGetCapsLatest(); + if (!capslatest) + abort();
Woah, that's a bit harsh, isn't it? How about a nice and polite
return EXIT_FAILURE;
instead? :)
I was just following the pattern of the error conditions right after this, but I'll use your suggestion.
[...]
@@ -192,11 +177,14 @@ mymain(void)
# define DO_TEST_FULL(name, when, gic, ...) \ do { \ - if (testInfoSetCommon(&info, gic) < 0) { \ + if (testQemuInfoSetArgs(&info, capslatest, \ + ARG_GIC, gic, \ + ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \ + ARG_END) < 0 || \ + qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0) { \
I haven't really spent any time digging, but that call to qemuTestCapsCacheInsert() looks odd to me. What exactly is the point in caching if we're going to be using a different set of capabilities pretty much every single time?
IIRC it's not really about caching or speed or whatever, it's required to get our manually populated qemuCaps into the standard src/qemu driver routines, which use virQEMUCapsCacheLookup everywhere Thanks, Cole
On Wed, 2019-04-10 at 18:53 -0400, Cole Robinson wrote:
On 4/10/19 12:53 PM, Andrea Bolognani wrote:
I haven't really spent any time digging, but that call to qemuTestCapsCacheInsert() looks odd to me. What exactly is the point in caching if we're going to be using a different set of capabilities pretty much every single time?
IIRC it's not really about caching or speed or whatever, it's required to get our manually populated qemuCaps into the standard src/qemu driver routines, which use virQEMUCapsCacheLookup everywhere
Yeah, I realized the motivation on my own while getting to the office this morning. A good night's sleep always helps see things more clearly :) -- Andrea Bolognani / Red Hat / Virtualization
Make all users of GIC_X use ARG_GIC explicitly, and drop the required gic parameter from DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2xmltest.c | 85 ++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 30 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e60c69872a..3b5314df78 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -175,11 +175,10 @@ mymain(void) cfg = virQEMUDriverGetConfig(&driver); -# define DO_TEST_FULL(name, when, gic, ...) \ +# define DO_TEST_FULL(name, when, ...) \ do { \ if (testQemuInfoSetArgs(&info, capslatest, \ - ARG_GIC, gic, \ - ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \ + __VA_ARGS__, \ ARG_END) < 0 || \ qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0) { \ VIR_TEST_DEBUG("Failed to generate test data for '%s'", name); \ @@ -211,7 +210,7 @@ mymain(void) # define NONE QEMU_CAPS_LAST # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, __VA_ARGS__) + DO_TEST_FULL(name, WHEN_BOTH, ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST) @@ -490,17 +489,22 @@ mymain(void) DO_TEST("blkdeviotune-max-length", NONE); DO_TEST("controller-usb-order", NONE); - DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, NONE); - DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, QEMU_CAPS_LAST); + DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, + ARG_QEMU_CAPS, QEMU_CAPS_LAST, NONE); + DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE); DO_TEST("seclabel-static", NONE); DO_TEST("seclabel-static-labelskip", NONE); DO_TEST("seclabel-none", NONE); DO_TEST("seclabel-dac-none", NONE); DO_TEST("seclabel-dynamic-none", NONE); DO_TEST("seclabel-device-multiple", NONE); - DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, GIC_NONE, NONE); + DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE); DO_TEST("numad-static-vcpu-no-numatune", NONE); DO_TEST("disk-scsi-lun-passthrough-sgio", @@ -1048,27 +1052,48 @@ mymain(void) QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_VNC); - DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-default-v2", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-default-v3", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-default-both", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, GIC_BOTH, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, GIC_BOTH, NONE); + DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-none-v2", WHEN_BOTH, + ARG_GIC, GIC_V2, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-none-v3", WHEN_BOTH, + ARG_GIC, GIC_V3, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-default-v2", WHEN_BOTH, + ARG_GIC, GIC_V2, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-default-v3", WHEN_BOTH, + ARG_GIC, GIC_V3, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-default-both", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, + ARG_GIC, GIC_V2, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, + ARG_GIC, GIC_V3, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, + ARG_GIC, GIC_V2, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, + ARG_GIC, GIC_V3, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-v3", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, + ARG_GIC, GIC_V2, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, + ARG_GIC, GIC_V3, ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("aarch64-gic-host", WHEN_BOTH, + ARG_GIC, GIC_BOTH, ARG_QEMU_CAPS, NONE); DO_TEST("memory-hotplug", NONE); DO_TEST("memory-hotplug-nonuma", NONE); -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: [...]
# define DO_TEST(name, ...) \ - DO_TEST_FULL(name, WHEN_BOTH, GIC_NONE, __VA_ARGS__) + DO_TEST_FULL(name, WHEN_BOTH, ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST)
Please move ARG_QEMU_CAPS and its arguments to a separate line. [...]
+ DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, QEMU_CAPS_LAST); + DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, + ARG_QEMU_CAPS, QEMU_CAPS_LAST, NONE); + DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("seclabel-dynamic-relabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE);
The first two test cases use QEMU_CAPS_LAST instead of NONE. Also the alignment is out of whack. [...]
+ DO_TEST_FULL("seclabel-dynamic-none-relabel", WHEN_INACTIVE, + ARG_QEMU_CAPS, NONE);
This is misaligned, too. [...]
+ DO_TEST_FULL("aarch64-gic-none", WHEN_BOTH, + ARG_GIC, GIC_NONE, ARG_QEMU_CAPS, NONE);
For this and all of the other "aarch64-gic-*" test cases, put ARG_GIC and ARG_QEMU_CAPS on separate lines. With the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed. One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename. This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behavior Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvdata/vhost-vsock.xml | 2 +- .../aarch64-os-firmware-efi.xml | 32 +++++++- tests/qemuxml2xmloutdata/os-firmware-bios.xml | 69 +++++++++++++++- .../os-firmware-efi-secboot.xml | 69 +++++++++++++++- tests/qemuxml2xmloutdata/os-firmware-efi.xml | 69 +++++++++++++++- tests/qemuxml2xmltest.c | 81 +++++++------------ 6 files changed, 267 insertions(+), 55 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-bios.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi.xml diff --git a/tests/qemuxml2argvdata/vhost-vsock.xml b/tests/qemuxml2argvdata/vhost-vsock.xml index bc550ace4e..f594c53f48 100644 --- a/tests/qemuxml2argvdata/vhost-vsock.xml +++ b/tests/qemuxml2argvdata/vhost-vsock.xml @@ -15,7 +15,7 @@ <on_crash>restart</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='usb' index='0'> + <controller type='usb' index='0' model='piix3-uhci'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> <controller type='virtio-serial' index='0'> diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml deleted file mode 120000 index beea6b2955..0000000000 --- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/aarch64-os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml new file mode 100644 index 0000000000..529ce6f3c2 --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>aarch64test</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='aarch64' machine='virt-4.0'>hvm</type> + <kernel>/aarch64.kernel</kernel> + <initrd>/aarch64.initrd</initrd> + <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline> + <dtb>/aarch64.dtb</dtb> + <boot dev='hd'/> + </os> + <features> + <apic/> + <pae/> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='allow'>cortex-a53</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.xml deleted file mode 120000 index 3d36d5df68..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-bios.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-bios.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.xml new file mode 100644 index 0000000000..63886666dd --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-bios.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='bios'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml deleted file mode 120000 index 93e184e2d2..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-efi-secboot.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml new file mode 100644 index 0000000000..a285e06334 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='yes'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.xml deleted file mode 120000 index 15cfad1ea0..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-efi.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.xml new file mode 100644 index 0000000000..46a7b1b780 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3b5314df78..6ae3cbb11f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -91,7 +91,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix) { VIR_FREE(info->infile); VIR_FREE(info->outfile); @@ -101,9 +102,10 @@ testInfoSetPaths(struct testQemuInfo *info, goto error; if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml", abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error; if (!virFileExists(info->outfile)) { @@ -175,7 +177,7 @@ mymain(void) cfg = virQEMUDriverGetConfig(&driver); -# define DO_TEST_FULL(name, when, ...) \ +# define DO_TEST_INTERNAL(name, suffix, when, ...) \ do { \ if (testQemuInfoSetArgs(&info, capslatest, \ __VA_ARGS__, \ @@ -186,7 +188,7 @@ mymain(void) } \ \ if (when & WHEN_INACTIVE) { \ - if (testInfoSetPaths(&info, name, WHEN_INACTIVE) < 0) { \ + if (testInfoSetPaths(&info, name, WHEN_INACTIVE, suffix) < 0) { \ VIR_TEST_DEBUG("Failed to generate inactive paths for '%s'", name); \ return -1; \ } \ @@ -196,7 +198,7 @@ mymain(void) } \ \ if (when & WHEN_ACTIVE) { \ - if (testInfoSetPaths(&info, name, WHEN_ACTIVE) < 0) { \ + if (testInfoSetPaths(&info, name, WHEN_ACTIVE, suffix) < 0) { \ VIR_TEST_DEBUG("Failed to generate active paths for '%s'", name); \ return -1; \ } \ @@ -209,9 +211,26 @@ mymain(void) # define NONE QEMU_CAPS_LAST +# define DO_TEST_FULL(name, when, ...) \ + DO_TEST_INTERNAL(name, "", when, __VA_ARGS__) + # define DO_TEST(name, ...) \ DO_TEST_FULL(name, WHEN_BOTH, ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST) +# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__) + +# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch, "latest", __VA_ARGS__) + +# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) + +# define DO_TEST_CAPS_LATEST(name) \ + DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") /* Unset or set all envvars here that are copied in qemudBuildCommandLine @@ -979,36 +998,14 @@ mymain(void) DO_TEST("smbios", NONE); DO_TEST("smbios-multiple-type2", NONE); - DO_TEST("os-firmware-bios", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); - DO_TEST("os-firmware-efi", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); - DO_TEST("os-firmware-efi-secboot", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); + DO_TEST_CAPS_LATEST("os-firmware-bios"); + DO_TEST_CAPS_LATEST("os-firmware-efi"); + DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); DO_TEST("aarch64-aavmf-virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); - DO_TEST("aarch64-os-firmware-efi", - QEMU_CAPS_DEVICE_VIRTIO_MMIO); + DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); DO_TEST("aarch64-virtio-pci-default", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, QEMU_CAPS_DEVICE_VIRTIO_MMIO, @@ -1256,24 +1253,8 @@ mymain(void) DO_TEST("riscv64-virt-pci", QEMU_CAPS_OBJECT_GPEX); - DO_TEST("virtio-transitional", - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_VIRTIO_RNG, - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, - QEMU_CAPS_DEVICE_VHOST_VSOCK, - QEMU_CAPS_VIRTIO_INPUT_HOST, - QEMU_CAPS_VIRTIO_SCSI); - DO_TEST("virtio-non-transitional", - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_VIRTIO_RNG, - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, - QEMU_CAPS_DEVICE_VHOST_VSOCK, - QEMU_CAPS_VIRTIO_INPUT_HOST, - QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_CAPS_LATEST("virtio-transitional"); + DO_TEST_CAPS_LATEST("virtio-non-transitional"); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.21.0
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed.
Please have each of these changes as separate preparatory commits.
One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename.
This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behavior
More on this later. [...]
static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix)
'suffix' should come before 'when', to match the corresponding function in xml2argv. Additionally, we're passing 'name' to the function here but we're storing it inside 'info' in xml2argv - we should be doing the same here. Please make that change as a separate preparatory commit. [...]
if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml", abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error;
if (!virFileExists(info->outfile)) {
Missing from the context, but immediately after this line we have: if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) ... We should format 'suffix' here too. --- Following up from the commit message: I was wondering about the way this test works, and discussing it with Jano (CC'd since he had his own opinion on the matter). There are several situations we can run into with these tests: 1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and WHEN_INACTIVE cases match; 2) same as the above, but the outputs don't match; 3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE. Case 1) covers the vast majority of existing test cases. As for output files, I would expect respectively 1) a single output file, with no suffix; 2) two output files with different suffixes; 3) a single output file with the corresponding -inactive or -active suffix. The problem with the current algorithm is that, when VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new test case so no output files exist yet, you'll end up with 1) the expected output, yay!; 2) failure, because both WHEN variants will write the (different) output to the unsuffixed file and step on each other's toes every time. This is annoying but ultimately okay, because the developer can break the loop simply by touching the suffixed files before running the test program; 3) the unsuffixed file being created instead of the suffixed one. The third scenario is suboptimal but not necessarily a very big deal either. One way to get rid of the above would be to pass an extra flags that controls whether falling back to the unsuffixed output files should be considered at all: the default would be to pass it for WHEN_BOTH, so that scenario 1) would be covered, and not to pass it for WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few test cases that fall into scenario 2) would have to go for a more verbose macro and *not* pass the flag manually. I feel like that would be acceptable given that most tests cases fall into 1) anyway. --- None of the above is really connected to whether or not we should use 'suffix' as I suggested earlier: we should definitely format it, even if it causes test suite churn. Not only that: you should also make sure... [...]
+# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__)
... you copy DO_TEST_CAPS_ARCH_VER() and DO_TEST_CAPS_VER() from xml2argv too, so that you can later introduce... [...]
+ DO_TEST_CAPS_LATEST("virtio-transitional"); + DO_TEST_CAPS_LATEST("virtio-non-transitional");
DO_TEST_CAPS_VER("virtio-transitional", "3.1.0"); DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); and friends like in xml2argv, too. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed.
Please have each of these changes as separate preparatory commits.
One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename.
This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behavior
[...]
static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix)
'suffix' should come before 'when', to match the corresponding function in xml2argv.
Additionally, we're passing 'name' to the function here but we're storing it inside 'info' in xml2argv - we should be doing the same here. Please make that change as a separate preparatory commit.
[...]
if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml",
I'd definitely put another minus between these suffixes (also, I'd like to see them in use).
abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error;
if (!virFileExists(info->outfile)) {
As for this virFileExists - I really dislike it. It is on my TODO list(TM) to change this to either: A) specify exactly which output files the test needs by using the appropriate DO_TEST macro or B) add a lot of symlinks for the expected output to the output directory.
Missing from the context, but immediately after this line we have:
if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) ...
We should format 'suffix' here too.
---
Following up from the commit message: I was wondering about the way this test works, and discussing it with Jano (CC'd since he had his own opinion on the matter).
There are several situations we can run into with these tests:
1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and WHEN_INACTIVE cases match;
2) same as the above, but the outputs don't match;
3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE.
Case 1) covers the vast majority of existing test cases.
As for output files, I would expect respectively
1) a single output file, with no suffix;
2) two output files with different suffixes;
3) a single output file with the corresponding -inactive or -active suffix.
The problem with the current algorithm is that, when VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new test case so no output files exist yet, you'll end up with
1) the expected output, yay!;
2) failure, because both WHEN variants will write the (different) output to the unsuffixed file and step on each other's toes every time. This is annoying but ultimately okay, because the developer can break the loop simply by touching the suffixed files before running the test program;
3) the unsuffixed file being created instead of the suffixed one.
The third scenario is suboptimal but not necessarily a very big deal either.
One way to get rid of the above would be to pass an extra flags that controls whether falling back to the unsuffixed output files should be considered at all: the default would be to pass it for WHEN_BOTH, so that scenario 1) would be covered, and not to pass it for WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few test cases that fall into scenario 2) would have to go for a more verbose macro and *not* pass the flag manually. I feel like that would be acceptable given that most tests cases fall into 1) anyway.
---
None of the above is really connected to whether or not we should use 'suffix' as I suggested earlier: we should definitely format it, even if it causes test suite churn. Not only that: you should also make sure...
The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests to coexist with the ones with enumerated capabilities. But it also contains the architecture, so even if -latest would be the prevailing case, I'd rather format it anyway. Jano
[...]
+# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__)
... you copy DO_TEST_CAPS_ARCH_VER() and DO_TEST_CAPS_VER() from xml2argv too, so that you can later introduce...
[...]
+ DO_TEST_CAPS_LATEST("virtio-transitional"); + DO_TEST_CAPS_LATEST("virtio-non-transitional");
DO_TEST_CAPS_VER("virtio-transitional", "3.1.0"); DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0");
and friends like in xml2argv, too.
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-04-11 at 14:15 +0200, Ján Tomko wrote:
On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml",
I'd definitely put another minus between these suffixes (also, I'd like to see them in use).
Looking at xml2argv, and as I suggested the implementation should be the same in xml2xml, we have # define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ DO_TEST_INTERNAL(name, "." arch "-" ver, \ ARG_CAPS_ARCH, arch, \ ARG_CAPS_VER, ver, \ __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ DO_TEST_CAPS_INTERNAL(name, arch, "latest", __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) # define DO_TEST_CAPS_LATEST(name) \ DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") so when you use DO_TEST_CAPS_LATEST("virtio-transitional"); 'suffix' will be ".x86_64-latest" and the resulting filename will be qemuxml2xmloutdata/virtio-transitional.x86_64-latest.xml If you had a similar test for WHEN_ACTIVE only, then you'd get qemuxml2xmloutdata/virtio-transitional-active.x86_64-latest.xml It doesn't look to me like any extra separators are necessary. [...]
None of the above is really connected to whether or not we should use 'suffix' as I suggested earlier: we should definitely format it, even if it causes test suite churn. Not only that: you should also make sure...
The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests to coexist with the ones with enumerated capabilities.
But it also contains the architecture, so even if -latest would be the prevailing case, I'd rather format it anyway.
Looks like we agree then :) -- Andrea Bolognani / Red Hat / Virtualization
On 4/11/19 8:15 AM, Ján Tomko wrote:
On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote:
Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed.
Please have each of these changes as separate preparatory commits.
One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename.
This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behavior
[...]
static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix)
'suffix' should come before 'when', to match the corresponding function in xml2argv.
Additionally, we're passing 'name' to the function here but we're storing it inside 'info' in xml2argv - we should be doing the same here. Please make that change as a separate preparatory commit.
[...]
if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml",
I'd definitely put another minus between these suffixes (also, I'd like to see them in use).
Sure I'll add example usage for this in the next version
abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error;
if (!virFileExists(info->outfile)) {
As for this virFileExists - I really dislike it. It is on my TODO list(TM) to change this to either: A) specify exactly which output files the test needs by using the appropriate DO_TEST macro or B) add a lot of symlinks for the expected output to the output directory.
Agreed that this should go away. I think the whole WHEN_X concept should go away: always test active and inactive paths, but the only DO_TEST distinction is active vs inactive output is expected to be the same, vs expected to be different. The latter case should mandate that -active and -inactive filesnames exist, the former case doesn't look for any state suffix. I think only the few WEHN_INACTIVE cases will need some extra output but otherwise it's pretty compatible with the current state of things.
Missing from the context, but immediately after this line we have:
if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) ...
We should format 'suffix' here too.
---
Following up from the commit message: I was wondering about the way this test works, and discussing it with Jano (CC'd since he had his own opinion on the matter).
There are several situations we can run into with these tests:
1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and WHEN_INACTIVE cases match;
2) same as the above, but the outputs don't match;
3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE.
Case 1) covers the vast majority of existing test cases.
As for output files, I would expect respectively
1) a single output file, with no suffix;
2) two output files with different suffixes;
3) a single output file with the corresponding -inactive or -active suffix.
The problem with the current algorithm is that, when VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new test case so no output files exist yet, you'll end up with
1) the expected output, yay!;
2) failure, because both WHEN variants will write the (different) output to the unsuffixed file and step on each other's toes every time. This is annoying but ultimately okay, because the developer can break the loop simply by touching the suffixed files before running the test program;
3) the unsuffixed file being created instead of the suffixed one.
The third scenario is suboptimal but not necessarily a very big deal either.
One way to get rid of the above would be to pass an extra flags that controls whether falling back to the unsuffixed output files should be considered at all: the default would be to pass it for WHEN_BOTH, so that scenario 1) would be covered, and not to pass it for WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few test cases that fall into scenario 2) would have to go for a more verbose macro and *not* pass the flag manually. I feel like that would be acceptable given that most tests cases fall into 1) anyway.
---
None of the above is really connected to whether or not we should use 'suffix' as I suggested earlier: we should definitely format it, even if it causes test suite churn. Not only that: you should also make sure...
The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests to coexist with the ones with enumerated capabilities.
But it also contains the architecture, so even if -latest would be the prevailing case, I'd rather format it anyway.
Hopefully one day we get to a point that DO_TEST always uses latest caps, and we have a DO_TEST_CAPS_LIST for the explicit caps list... Thanks, Cole
participants (3)
-
Andrea Bolognani -
Cole Robinson -
Ján Tomko