| Message ID | 7bc2aa8526e1ceab5c756a8b52ef20493cb98afa.1554137098.git.crobinso@redhat.com |
|---|---|
| State | Accepted |
| Commit | ffa8ea8d5fcc6b7f61d7442bf839daabcde330d9 |
| Headers | show |
| Series | tests: qemuxml2xml: add DO_TEST_CAPS* | expand |
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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);
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(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list