[20/21] tests: qemuxml2argv: move DO_CAPS_TEST* qemuCaps init

Message ID 80f40cc264dfc120e97e8a265be8f2f1d626112e.1552574299.git.crobinso@redhat.com
State Superseded
Headers show
Series
  • tests: qemuxml2argv: support optional arguments
Related show

Commit Message

Cole Robinson March 14, 2019, 2:44 p.m.
Move DO_CAPS_TEST* qemuCaps init and all the associated setup
into testInfoSetArgs, adding ARG_CAPS_ARCH and ARG_CAPS_VER
options and using those to build the capsfile path locally

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 tests/qemuxml2argvtest.c | 68 +++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 21 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani March 19, 2019, 3:18 p.m. | #1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
[...]
> @@ -612,20 +614,25 @@ typedef enum {

>      ARG_MIGRATE_FD,

>      ARG_FLAGS,

>      ARG_PARSEFLAGS,

> +    ARG_CAPS_ARCH,

> +    ARG_CAPS_VER,


I guess these could be simply ARG_ARCH and ARG_VER, but I don't
have a terribly strong opinion on the subject.

[...]
> +testInfoSetArgs(struct testInfo *info, virHashTablePtr capslatest, ...)


One argument per line, please.

>  {

>      va_list argptr;

>      testInfoArgNames argname;

>      virQEMUCapsPtr qemuCaps = NULL;

>      int gic = GIC_NONE;

>      int ret = -1;

> +    char *capsarch = NULL;

> +    char *capsver = NULL;

> +    VIR_AUTOFREE(char *) capsfile = NULL;


ret should be the last variable in the list.

[...]
> +    if (!qemuCaps && capsarch && capsver) {

> +        bool stripmachinealiases = false;


I think it would be a good idea to perform some more validation on
the input here: for example we should error out if ARG_CAPS_ARCH has
been provided but ARG_CAPS_VER hasn't or viceversa, or if both
ARG_QEMU_CAPS and ARG_CAPS_ARCH+ARG_CAPS_VER have been provided,
because in both scenarios the user is probably not getting whatever
result they were expecting.

> +        if (STREQ(capsver, "latest")) {

> +            if (VIR_STRDUP(capsfile, virHashLookup(capslatest, capsarch)) < 0)

> +                goto cleanup;

> +            stripmachinealiases = true;

> +        } else if (virAsprintf(&capsfile,

> +                               TEST_CAPS_PATH "%s.%s.xml",

> +                               capsver, capsarch) < 0) {


I'd prefer this as

  virAsprintf(&capsfile, "%s/caps_%s.%s.xml",
              TEST_CAPS_PATH, capsver, capsarch)

with TEST_CAPS_PATH being redefined as

  abs_srcdir "/qemucapabilitiesdata"

of course: according to the name, it's supposed to be a path and
not a prefix after all.


Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 21, 2019, 7:54 p.m. | #2
On 3/19/19 11:18 AM, Andrea Bolognani wrote:
> On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:

> [...]

>> @@ -612,20 +614,25 @@ typedef enum {

>>      ARG_MIGRATE_FD,

>>      ARG_FLAGS,

>>      ARG_PARSEFLAGS,

>> +    ARG_CAPS_ARCH,

>> +    ARG_CAPS_VER,

> 

> I guess these could be simply ARG_ARCH and ARG_VER, but I don't

> have a terribly strong opinion on the subject.

> 


I figured ARG_ARCH/ARG_VER are ambiguous enough, the CAPS was to make
it clear this was specifically for caps file lookups

> [...]

>> +testInfoSetArgs(struct testInfo *info, virHashTablePtr capslatest, ...)

> 

> One argument per line, please.

> 

>>  {

>>      va_list argptr;

>>      testInfoArgNames argname;

>>      virQEMUCapsPtr qemuCaps = NULL;

>>      int gic = GIC_NONE;

>>      int ret = -1;

>> +    char *capsarch = NULL;

>> +    char *capsver = NULL;

>> +    VIR_AUTOFREE(char *) capsfile = NULL;

> 

> ret should be the last variable in the list.

> 

> [...]

>> +    if (!qemuCaps && capsarch && capsver) {

>> +        bool stripmachinealiases = false;

> 

> I think it would be a good idea to perform some more validation on

> the input here: for example we should error out if ARG_CAPS_ARCH has

> been provided but ARG_CAPS_VER hasn't or viceversa, or if both

> ARG_QEMU_CAPS and ARG_CAPS_ARCH+ARG_CAPS_VER have been provided,

> because in both scenarios the user is probably not getting whatever

> result they were expecting.

> 


I'll handle this in a separate patch

>> +        if (STREQ(capsver, "latest")) {

>> +            if (VIR_STRDUP(capsfile, virHashLookup(capslatest, capsarch)) < 0)

>> +                goto cleanup;

>> +            stripmachinealiases = true;

>> +        } else if (virAsprintf(&capsfile,

>> +                               TEST_CAPS_PATH "%s.%s.xml",

>> +                               capsver, capsarch) < 0) {

> 

> I'd prefer this as

> 

>   virAsprintf(&capsfile, "%s/caps_%s.%s.xml",

>               TEST_CAPS_PATH, capsver, capsarch)

> 

> with TEST_CAPS_PATH being redefined as

> 

>   abs_srcdir "/qemucapabilitiesdata"

> 

> of course: according to the name, it's supposed to be a path and

> not a prefix after all.

>


Sure, I'll break that change out into a prep patch. v2 incoming

Thanks,
Cole

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

Patch

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ff74892944..d5e02c26d8 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -605,6 +605,8 @@  testCompareXMLToArgv(const void *data)
     return ret;
 }
 
+# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_"
+
 typedef enum {
     ARG_QEMU_CAPS = 1,
     ARG_GIC,
@@ -612,20 +614,25 @@  typedef enum {
     ARG_MIGRATE_FD,
     ARG_FLAGS,
     ARG_PARSEFLAGS,
+    ARG_CAPS_ARCH,
+    ARG_CAPS_VER,
 
     ARG_END = QEMU_CAPS_LAST,
 } testInfoArgNames;
 
 static int
-testInfoSetArgs(struct testInfo *info, ...)
+testInfoSetArgs(struct testInfo *info, virHashTablePtr capslatest, ...)
 {
     va_list argptr;
     testInfoArgNames argname;
     virQEMUCapsPtr qemuCaps = NULL;
     int gic = GIC_NONE;
     int ret = -1;
+    char *capsarch = NULL;
+    char *capsver = NULL;
+    VIR_AUTOFREE(char *) capsfile = NULL;
 
-    va_start(argptr, info);
+    va_start(argptr, capslatest);
     while ((argname = va_arg(argptr, int)) < ARG_END) {
         switch (argname) {
         case ARG_QEMU_CAPS:
@@ -654,6 +661,14 @@  testInfoSetArgs(struct testInfo *info, ...)
             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");
@@ -661,13 +676,33 @@  testInfoSetArgs(struct testInfo *info, ...)
         }
     }
 
-    if (!info->qemuCaps) {
-        if (!qemuCaps) {
-            fprintf(stderr, "No qemuCaps generated\n");
+    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,
+                               TEST_CAPS_PATH "%s.%s.xml",
+                               capsver, capsarch) < 0) {
             goto cleanup;
         }
-        VIR_STEAL_PTR(info->qemuCaps, qemuCaps);
+
+        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 && testQemuCapsSetGIC(info->qemuCaps, gic) < 0)
         goto cleanup;
@@ -806,7 +841,6 @@  mymain(void)
  * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate
  * version.
  */
-# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_"
 
 # define DO_TEST_CAPS_INTERNAL(_name, arch, ver, ...) \
     do { \
@@ -814,20 +848,11 @@  mymain(void)
             .name = _name, \
             .suffix = "." arch "-" ver, \
         }; \
-        static const char *capsfile = TEST_CAPS_PATH ver "." arch ".xml"; \
-        static bool stripmachinealiases; \
-        if (STREQ(ver, "latest")) { \
-            capsfile = virHashLookup(capslatest, arch); \
-            stripmachinealiases = true; \
-        } \
-        if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \
-                                                            capsfile))) \
-            return EXIT_FAILURE; \
-        if (stripmachinealiases) \
-            virQEMUCapsStripMachineAliases(info.qemuCaps); \
-        if (testInfoSetArgs(&info, __VA_ARGS__, ARG_END) < 0) \
+        if (testInfoSetArgs(&info, capslatest, \
+                            ARG_CAPS_ARCH, arch, \
+                            ARG_CAPS_VER, ver, \
+                            __VA_ARGS__, ARG_END) < 0) \
             return EXIT_FAILURE; \
-        info.flags |= FLAG_REAL_CAPS; \
         if (virTestRun("QEMU XML-2-ARGV " _name "." arch "-" ver, \
                        testCompareXMLToArgv, &info) < 0) \
             ret = -1; \
@@ -865,7 +890,8 @@  mymain(void)
         static struct testInfo info = { \
             .name = _name, \
         }; \
-        if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \
+        if (testInfoSetArgs(&info, capslatest, \
+                            __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \
             return EXIT_FAILURE; \
         if (virTestRun("QEMU XML-2-ARGV " _name, \
                        testCompareXMLToArgv, &info) < 0) \