| Message ID | 80f40cc264dfc120e97e8a265be8f2f1d626112e.1552574299.git.crobinso@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: qemuxml2argv: support optional arguments | expand |
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
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
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) \
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