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

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.