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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list