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