[10/12] tests: qemuxml2xml: Use testQemuInfoSetArgs

Message ID 7bc2aa8526e1ceab5c756a8b52ef20493cb98afa.1554137098.git.crobinso@redhat.com
State Accepted
Commit ffa8ea8d5fcc6b7f61d7442bf839daabcde330d9
Headers show
Series
  • tests: qemuxml2xml: add DO_TEST_CAPS*
Related show

Commit Message

Cole Robinson April 1, 2019, 4:47 p.m.
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

Comments

Andrea Bolognani April 10, 2019, 4:53 p.m. | #1
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
Cole Robinson April 10, 2019, 10:53 p.m. | #2
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
Andrea Bolognani April 11, 2019, 7:20 a.m. | #3
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

Patch

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);