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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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