[libvirt] [PATCH 00/21] tests: qemuxml2argv: support optional arguments
Right now in qemuxml2argv we have a proliferation of DO_*TEST* macros. They essentially fill in data in the testInfo struct and invoke the test function. There's several bits of data that we want to specify for a small subset of tests: flags, parseFlags, migrateFrom/migrateFd, gic. The base macros need to handle these in their argument lists, and we provide many convenience macros that fill in default values for the less common parameters. Any time we want to add a new bit of data though, the base macros are extended, and every caller of those needs to be adjusted, and we are faced with the question of whether to extend the combinatorial explosion of convenience macros which have only a subset of options exposed. This series adds a testInfoSetArgs which uses va_args to make these mandatory parameters optional. The general format is: DO_TEST_FULL(... ARG_FOO, foovalue, ARG_BAR, barvalue, ...) Specifically, one of the migrate tests went from: DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE); to: DO_TEST_FULL("restore-v2", ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); Which may not seem directly compelling, but adding new testInfo fields will be much easier. This will also be a base for a later series to share the VIR_TEST_CAPS infrastructure with qemuxml2xmltest Cole Robinson (21): qemu: add virQEMUCapsSetVList tests: qemuxml2argv: add testInfoSetArgs tests: qemuxml2argv: add va_arg enum handling tests: qemuxml2argv: push ARG_QEMU_CAPS to callers tests: qemuxml2argv: break apart testInitQEMUCaps tests: qemuxml2argv: handle gic with vaargs tests: qemuxml2argv: handle migrate* with varargs tests: qemuxml2argv: handle flags with varargs tests: qemuxml2argv: handle parseFlags with varargs tests: qemuxml2argv: remove DO_TEST_PARSE_FLAGS_ERROR tests: qemuxml2argv: remove unused DO_TEST_CAPS* macros tests: qemuxml2argv: add a comment separating DO_TEST* macros tests: qemuxml2argv: remove unused CAPS migrateFrom tests: qemuxml2argv: use varargs for CAPS flags tests: qemuxml2argv: remove full testInfo initialization tests: qemuxml2argv: centralize CAPS suffix building tests: qemuxml2argv: build capsfile in DO_TEST_CAPS_INTERNAL tests: qemuxml2argv: add testInfoClear tests: qemuxml2argv: move DO_TEST qemuCaps init tests: qemuxml2argv: move DO_CAPS_TEST* qemuCaps init tests: qemuxml2argv: add TEST_INTERNAL src/qemu/qemu_capabilities.c | 14 +- src/qemu/qemu_capabilities.h | 2 + tests/qemuxml2argvtest.c | 273 +++++++++++++++++++++++------------ 3 files changed, 193 insertions(+), 96 deletions(-) -- 2.20.1
It will be used in future patches Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_capabilities.c | 14 +++++++++++--- src/qemu/qemu_capabilities.h | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4c8229fbda..a274ce7992 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1664,14 +1664,22 @@ virQEMUCapsSet(virQEMUCapsPtr qemuCaps, void -virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) +virQEMUCapsSetVList(virQEMUCapsPtr qemuCaps, va_list list) { - va_list list; int flag; - va_start(list, qemuCaps); while ((flag = va_arg(list, int)) < QEMU_CAPS_LAST) ignore_value(virBitmapSetBit(qemuCaps->flags, flag)); +} + + +void +virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) +{ + va_list list; + + va_start(list, qemuCaps); + virQEMUCapsSetVList(qemuCaps, list); va_end(list); } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 06c7606e2f..3601207989 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -518,6 +518,8 @@ virQEMUCapsPtr virQEMUCapsNew(void); void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1); +void virQEMUCapsSetVList(virQEMUCapsPtr qemuCaps, + va_list list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) ATTRIBUTE_NONNULL(1); void virQEMUCapsClear(virQEMUCapsPtr qemuCaps, -- 2.20.1
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:
It will be used in future patches
You're actually changing the existing virQEMUCapsSetList() to use it... I'd point that out in the commit message. You can still mention the fact that you're going to add more users later on. [...]
void +virQEMUCapsSetVList(virQEMUCapsPtr qemuCaps, va_list list)
I'd call this virQEMUCapsSetVAList(), given the type of the second argument; the existing virCommandNewVAList() already uses this naming scheme. The second argument should be on a separate line. Everything else looks good, so Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
For now it just fills in the qemuCaps list. We will expand it in future patches Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 67c5c74ec5..3b90cd1873 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -624,6 +624,18 @@ testCompareXMLToArgv(const void *data) return ret; } +static int +testInfoSetArgs(struct testInfo *info, ...) +{ + va_list argptr; + int ret = 0; + + va_start(argptr, info); + virQEMUCapsSetVList(info->qemuCaps, argptr); + va_end(argptr); + return ret; +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -809,7 +821,8 @@ mymain(void) }; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ - virQEMUCapsSetList(info.qemuCaps, __VA_ARGS__, QEMU_CAPS_LAST); \ + if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST) < 0) \ + return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " name, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ -- 2.20.1
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote: [...]
+static int +testInfoSetArgs(struct testInfo *info, ...) +{ + va_list argptr; + int ret = 0; + + va_start(argptr, info); + virQEMUCapsSetVList(info->qemuCaps, argptr); + va_end(argptr);
Please leave an extra empty line here.
+ return ret; +}
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This establishes a pattern that will allow us to make test macros more general purpose, by taking optional arguments. The general format will be: DO_TEST_FULL(... ARG_FOO, <value1>, ARG_BAR, <value2>) ARG_X are just enum values that we look for in the va_args and know how to interpret. Implement this for the existing implicit qemuCaps va_args Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3b90cd1873..0dba908c70 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -624,14 +624,35 @@ testCompareXMLToArgv(const void *data) return ret; } +typedef enum { + ARG_QEMU_CAPS = 1, + + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames; + static int testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; - int ret = 0; + testInfoArgNames argname; + int ret = -1; va_start(argptr, info); - virQEMUCapsSetVList(info->qemuCaps, argptr); + while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + virQEMUCapsSetVList(info->qemuCaps, argptr); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument"); + goto cleanup; + } + } + + ret = 0; + cleanup: va_end(argptr); return ret; } @@ -821,7 +842,8 @@ mymain(void) }; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ - if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST) < 0) \ + if (testInfoSetArgs(&info, ARG_QEMU_CAPS, \ + __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " name, \ testCompareXMLToArgv, &info) < 0) \ -- 2.20.1
On 3/14/19 9:43 AM, Cole Robinson wrote:
This establishes a pattern that will allow us to make test macros more general purpose, by taking optional arguments. The general format will be:
DO_TEST_FULL(... ARG_FOO, <value1>, ARG_BAR, <value2>)
ARG_X are just enum values that we look for in the va_args and know how to interpret.
Implement this for the existing implicit qemuCaps va_args
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
+typedef enum { + ARG_QEMU_CAPS = 1, + + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames; +
Reading this after my cover letter reply: Oh, so you _do_ have a sentinel...
static int testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; - int ret = 0; + testInfoArgNames argname; + int ret = -1;
va_start(argptr, info); - virQEMUCapsSetVList(info->qemuCaps, argptr); + while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + virQEMUCapsSetVList(info->qemuCaps, argptr); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument");
...and you are handling it (except that you ALWAYS handle it by printing an error, is that intentional?),...
+ goto cleanup; + } + } + + ret = 0; + cleanup: va_end(argptr); return ret; } @@ -821,7 +842,8 @@ mymain(void) }; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ - if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST) < 0) \ + if (testInfoSetArgs(&info, ARG_QEMU_CAPS, \ + __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \
...and you are merely ensuring that the macro supplies the sentinel automatically, instead of the users having to be aware of it. Works because all users are calling a macro rather than the direct function. In fact, for this patch, you are supplying a double-sentinel, and I'm suspecting (without reading ahead) that later patches improve things as you add more ARG_ markers, and replacing QEMU_CAPS_LAST (which is now identical to ARG_END, and confusingly given twice) with something more obvious. Okay, you can ignore my noise on the cover letter. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 3/14/19 11:23 AM, Eric Blake wrote:
On 3/14/19 9:43 AM, Cole Robinson wrote:
This establishes a pattern that will allow us to make test macros more general purpose, by taking optional arguments. The general format will be:
DO_TEST_FULL(... ARG_FOO, <value1>, ARG_BAR, <value2>)
ARG_X are just enum values that we look for in the va_args and know how to interpret.
Implement this for the existing implicit qemuCaps va_args
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
+typedef enum { + ARG_QEMU_CAPS = 1, + + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames; +
Reading this after my cover letter reply: Oh, so you _do_ have a sentinel...
static int testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; - int ret = 0; + testInfoArgNames argname; + int ret = -1;
va_start(argptr, info); - virQEMUCapsSetVList(info->qemuCaps, argptr); + while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + virQEMUCapsSetVList(info->qemuCaps, argptr); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument");
...and you are handling it (except that you ALWAYS handle it by printing an error, is that intentional?),...
See the while() condition: if we see ARG_END, we exit the loop, so we shouldn't ever hit this condition and it's only in the switch to appease gcc
+ goto cleanup; + } + } + + ret = 0; + cleanup: va_end(argptr); return ret; } @@ -821,7 +842,8 @@ mymain(void) }; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ - if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST) < 0) \ + if (testInfoSetArgs(&info, ARG_QEMU_CAPS, \ + __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \
...and you are merely ensuring that the macro supplies the sentinel automatically, instead of the users having to be aware of it. Works because all users are calling a macro rather than the direct function.
In fact, for this patch, you are supplying a double-sentinel, and I'm suspecting (without reading ahead) that later patches improve things as you add more ARG_ markers, and replacing QEMU_CAPS_LAST (which is now identical to ARG_END, and confusingly given twice) with something more obvious.
The double sentinel is actually a requirement of the current code, because once we see ARG_QEMU_CAPS, we pass off the va_list to virQEMUCapsSetVList which does its own arg processing and uses QEMU_CAPS_LAST as a sentinel. We then kick back to this while() loop, which sees ARG_END, and completes parsing. The ARG_END = QEMU_CAPS_LAST is a bit weird, but it handles the DO_TEST(..., NONE) case, which translates to testInfoSetArgs(&info, ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END) Sine NONE == QEMU_CAPS_LAST == ARG_END, we finish parsing on QEMU_CAPS_LAST. If ARG_END != QEMU_CAPS_LAST, the loop would try to interpret QEMU_CAPS_LAST as an ARG_X value, and fail Thanks, Cole
On 3/14/19 2:42 PM, Cole Robinson wrote:
+typedef enum { + ARG_QEMU_CAPS = 1, + + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames; +
Do you need some sort of compile-time check that QEMU_CAPS_LAST doesn't overlap with any other ARG_*?
+ while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + virQEMUCapsSetVList(info->qemuCaps, argptr); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument");
...and you are handling it (except that you ALWAYS handle it by printing an error, is that intentional?),...
See the while() condition: if we see ARG_END, we exit the loop, so we shouldn't ever hit this condition and it's only in the switch to appease gcc
Indeed, now that you point it out, it makes sense.
In fact, for this patch, you are supplying a double-sentinel, and I'm suspecting (without reading ahead) that later patches improve things as you add more ARG_ markers, and replacing QEMU_CAPS_LAST (which is now identical to ARG_END, and confusingly given twice) with something more obvious.
The double sentinel is actually a requirement of the current code, because once we see ARG_QEMU_CAPS, we pass off the va_list to virQEMUCapsSetVList which does its own arg processing and uses QEMU_CAPS_LAST as a sentinel. We then kick back to this while() loop, which sees ARG_END, and completes parsing.
The ARG_END = QEMU_CAPS_LAST is a bit weird, but it handles the DO_TEST(..., NONE) case, which translates to
testInfoSetArgs(&info, ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END)
Sine NONE == QEMU_CAPS_LAST == ARG_END, we finish parsing on QEMU_CAPS_LAST. If ARG_END != QEMU_CAPS_LAST, the loop would try to interpret QEMU_CAPS_LAST as an ARG_X value, and fail
Clever. May be worth a comment in the code and/or commit message, but you've convinced me why it works. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 3/14/19 4:29 PM, Eric Blake wrote:
On 3/14/19 2:42 PM, Cole Robinson wrote:
+typedef enum { + ARG_QEMU_CAPS = 1, + + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames; +
Do you need some sort of compile-time check that QEMU_CAPS_LAST doesn't overlap with any other ARG_*?
Sure but it seems extremely unlikely that will ever happen, there's presently 300+ QEMU_CAPS_X. But if wanna provide the magic incantation I'll squash it in
+ while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) { + case ARG_QEMU_CAPS: + virQEMUCapsSetVList(info->qemuCaps, argptr); + break; + + case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument");
...and you are handling it (except that you ALWAYS handle it by printing an error, is that intentional?),...
See the while() condition: if we see ARG_END, we exit the loop, so we shouldn't ever hit this condition and it's only in the switch to appease gcc
Indeed, now that you point it out, it makes sense.
In fact, for this patch, you are supplying a double-sentinel, and I'm suspecting (without reading ahead) that later patches improve things as you add more ARG_ markers, and replacing QEMU_CAPS_LAST (which is now identical to ARG_END, and confusingly given twice) with something more obvious.
The double sentinel is actually a requirement of the current code, because once we see ARG_QEMU_CAPS, we pass off the va_list to virQEMUCapsSetVList which does its own arg processing and uses QEMU_CAPS_LAST as a sentinel. We then kick back to this while() loop, which sees ARG_END, and completes parsing.
The ARG_END = QEMU_CAPS_LAST is a bit weird, but it handles the DO_TEST(..., NONE) case, which translates to
testInfoSetArgs(&info, ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END)
Sine NONE == QEMU_CAPS_LAST == ARG_END, we finish parsing on QEMU_CAPS_LAST. If ARG_END != QEMU_CAPS_LAST, the loop would try to interpret QEMU_CAPS_LAST as an ARG_X value, and fail
Clever. May be worth a comment in the code and/or commit message, but you've convinced me why it works.
Good point, I'll squash this in diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0dba908c70..d56acc8de1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -627,6 +627,17 @@ testCompareXMLToArgv(const void *data) typedef enum { ARG_QEMU_CAPS = 1, + /* ARG_END is our va_args sentinel. The value QEMU_CAPS_LATEST is + * necessary to handle the DO_TEST(..., NONE) case, which through macro + * magic will give the va_args list: + * + * ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END + * + * SetArgs consumes the first item, hands off control to virQEMUCapsX + * virQEMUCapsX sees NONE aka QEMU_CAPS_LAST, returns to SetArgs. + * SetArgs sees QEMU_CAPS_LAST aka ARG_END, and exits the parse loop. + * If ARG_END != QEMU_CAPS_LAST, this last step would generate an error. + */ ARG_END = QEMU_CAPS_LAST, } testInfoArgNames; - Cole
On Fri, 2019-03-15 at 14:47 -0400, Cole Robinson wrote:
On 3/14/19 4:29 PM, Eric Blake wrote:
On 3/14/19 2:42 PM, Cole Robinson wrote:
The double sentinel is actually a requirement of the current code, because once we see ARG_QEMU_CAPS, we pass off the va_list to virQEMUCapsSetVList which does its own arg processing and uses QEMU_CAPS_LAST as a sentinel. We then kick back to this while() loop, which sees ARG_END, and completes parsing.
The ARG_END = QEMU_CAPS_LAST is a bit weird, but it handles the DO_TEST(..., NONE) case, which translates to
testInfoSetArgs(&info, ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END)
Sine NONE == QEMU_CAPS_LAST == ARG_END, we finish parsing on QEMU_CAPS_LAST. If ARG_END != QEMU_CAPS_LAST, the loop would try to interpret QEMU_CAPS_LAST as an ARG_X value, and fail
Clever. May be worth a comment in the code and/or commit message, but you've convinced me why it works.
Good point, I'll squash this in
[...]
+ /* ARG_END is our va_args sentinel. The value QEMU_CAPS_LATEST is + * necessary to handle the DO_TEST(..., NONE) case, which through macro + * magic will give the va_args list: + * + * ARG_QEMU_CAPS, NONE, QEMU_CAPS_LAST, ARG_END + * + * SetArgs consumes the first item, hands off control to virQEMUCapsX + * virQEMUCapsX sees NONE aka QEMU_CAPS_LAST, returns to SetArgs. + * SetArgs sees QEMU_CAPS_LAST aka ARG_END, and exits the parse loop. + * If ARG_END != QEMU_CAPS_LAST, this last step would generate an error. + */
Definitely squash in the comment :) My only concern is that ARG_QEMU_CAPS requires you to be very careful with its usage: not only you have to make sure you have QEMU_CAPS_LAST as sentinel, but also you can only reasonably use it as the last argument because QEMU_CAPS_LAST sharing the same value as ARG_END would cause a macro like #define DO_TEST_GIC_BROKEN(name, gic, ...) \ TEST_INTERNAL(name, "", \ ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \ ARG_GIC, gic) to ignore the ARG_GIC argument when invoked as DO_TEST_GIC_BROKEN("broken", GIC_V2, NONE); I guess it's fair to expect both the developer and the reviewer to be more careful than usual when touching this kind of dark magic though, so including the comment is probably enough. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote: [...]
+typedef enum { + ARG_QEMU_CAPS = 1,
Any specific reason to start from 1 rather than 0, or even leaving out the start value entirely?
+ + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames;
The name of the enum should be singular, otherwise it will look weird when you declare a variable of the type, like...
static int testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; - int ret = 0; + testInfoArgNames argname;
... here. [...]
+ while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) {
I think you should either call va_arg() with testInfoArgNames as the second argument, or make the argname variable int and then have an explicit cast in the switch(). [...]
+ case ARG_END: + default: + fprintf(stderr, "Unexpected test info argument"); + goto cleanup; + } + } + + ret = 0;
One extra empty line here, please. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 3/19/19 7:36 AM, Andrea Bolognani wrote:
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote: [...]
+typedef enum { + ARG_QEMU_CAPS = 1,
Any specific reason to start from 1 rather than 0, or even leaving out the start value entirely?
Nope it's not necessary, I've dropped it. I think it was left over from earlier patch state
+ + ARG_END = QEMU_CAPS_LAST, +} testInfoArgNames;
The name of the enum should be singular, otherwise it will look weird when you declare a variable of the type, like...
static int testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; - int ret = 0; + testInfoArgNames argname;
... here.
[...]
+ while ((argname = va_arg(argptr, int)) < ARG_END) { + switch (argname) {
I think you should either call va_arg() with testInfoArgNames as the second argument, or make the argname variable int and then have an explicit cast in the switch().
I went with the former Thanks, Cole
This is necessary before we can start adding more optional parameter implementations to DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0dba908c70..393d2399fa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -842,8 +842,7 @@ mymain(void) }; \ if (testInitQEMUCaps(&info, gic) < 0) \ return EXIT_FAILURE; \ - if (testInfoSetArgs(&info, ARG_QEMU_CAPS, \ - __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ + if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " name, \ testCompareXMLToArgv, &info) < 0) \ @@ -852,24 +851,29 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, GIC_NONE, __VA_ARGS__) + DO_TEST_FULL(name, NULL, -1, 0, 0, GIC_NONE, \ + ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_GIC(name, gic, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, gic, __VA_ARGS__) + DO_TEST_FULL(name, NULL, -1, 0, 0, gic, \ + ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, \ - 0, GIC_NONE, __VA_ARGS__) + 0, GIC_NONE, \ + ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, NULL, -1, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ - 0, GIC_NONE, __VA_ARGS__) + 0, GIC_NONE, \ + ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ DO_TEST_FULL(name, NULL, -1, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ - parseFlags, GIC_NONE, __VA_ARGS__) + parseFlags, GIC_NONE, \ + ARG_QEMU_CAPS, __VA_ARGS__) # define NONE QEMU_CAPS_LAST @@ -1707,12 +1711,17 @@ mymain(void) QEMU_CAPS_CCW_CSSID_UNRESTRICTED, QEMU_CAPS_DEVICE_VFIO_CCW); - DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE); - DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, GIC_NONE, NONE); - DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, GIC_NONE, NONE); - DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, GIC_NONE, NONE); + DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, + ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, GIC_NONE, + ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, GIC_NONE, + ARG_QEMU_CAPS, NONE); + DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, GIC_NONE, + ARG_QEMU_CAPS, NONE); DO_TEST_FULL("migrate-numa-unaligned", "stdio", 7, 0, 0, GIC_NONE, + ARG_QEMU_CAPS, QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM); @@ -1756,10 +1765,12 @@ mymain(void) DO_TEST("cpu-host-model-vendor", NONE); DO_TEST_FULL("cpu-host-model-fallback", NULL, -1, FLAG_SKIP_LEGACY_CPUS, 0, - GIC_NONE, NONE); + GIC_NONE, + ARG_QEMU_CAPS, NONE); DO_TEST_FULL("cpu-host-model-nofallback", NULL, -1, FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, - 0, GIC_NONE, NONE); + 0, GIC_NONE, + ARG_QEMU_CAPS, NONE); DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM); DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM); @@ -2849,6 +2860,7 @@ mymain(void) QEMU_CAPS_PIIX3_USB_UHCI); DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", NULL, -1, 0, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, GIC_NONE, + ARG_QEMU_CAPS, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_DEVICE_QEMU_XHCI); -- 2.20.1
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:
This is necessary before we can start adding more optional parameter implementations to DO_TEST_FULL
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 393d2399fa..06a28178d0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -378,25 +378,6 @@ testAddCPUModels(virQEMUCapsPtr caps, bool skipLegacy) } -static int -testInitQEMUCaps(struct testInfo *info, - int gic) -{ - int ret = -1; - - if (!(info->qemuCaps = virQEMUCapsNew())) - goto cleanup; - - if (testQemuCapsSetGIC(info->qemuCaps, gic) < 0) - goto cleanup; - - ret = 0; - - cleanup: - return ret; -} - - static int testUpdateQEMUCaps(const struct testInfo *info, virDomainObjPtr vm, @@ -840,7 +821,9 @@ mymain(void) static struct testInfo info = { \ name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ }; \ - if (testInitQEMUCaps(&info, gic) < 0) \ + if (!(info.qemuCaps = virQEMUCapsNew())) \ + return EXIT_FAILURE; \ + if (testQemuCapsSetGIC(info.qemuCaps, gic) < 0) \ return EXIT_FAILURE; \ if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ -- 2.20.1
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This allows us to drop stub gic values from DO_TEST_FULL calls Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 06a28178d0..60df2c3a00 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -607,6 +607,7 @@ testCompareXMLToArgv(const void *data) typedef enum { ARG_QEMU_CAPS = 1, + ARG_GIC, ARG_END = QEMU_CAPS_LAST, } testInfoArgNames; @@ -625,6 +626,12 @@ testInfoSetArgs(struct testInfo *info, ...) virQEMUCapsSetVList(info->qemuCaps, argptr); break; + case ARG_GIC: + if (testQemuCapsSetGIC(info->qemuCaps, + va_arg(argptr, int)) < 0) + goto cleanup; + break; + case ARG_END: default: fprintf(stderr, "Unexpected test info argument"); @@ -816,15 +823,13 @@ mymain(void) DO_TEST_CAPS_FULL(name, 0, 0, ver) # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ - parseFlags, gic, ...) \ + parseFlags, ...) \ do { \ static struct testInfo info = { \ name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ }; \ if (!(info.qemuCaps = virQEMUCapsNew())) \ return EXIT_FAILURE; \ - if (testQemuCapsSetGIC(info.qemuCaps, gic) < 0) \ - return EXIT_FAILURE; \ if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " name, \ @@ -834,28 +839,29 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, GIC_NONE, \ + DO_TEST_FULL(name, NULL, -1, 0, 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_GIC(name, gic, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, gic, \ + DO_TEST_FULL(name, NULL, -1, 0, 0, \ + ARG_GIC, gic, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, \ - 0, GIC_NONE, \ + 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, NULL, -1, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ - 0, GIC_NONE, \ + 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ DO_TEST_FULL(name, NULL, -1, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ - parseFlags, GIC_NONE, \ + parseFlags, \ ARG_QEMU_CAPS, __VA_ARGS__) # define NONE QEMU_CAPS_LAST @@ -1694,16 +1700,16 @@ mymain(void) QEMU_CAPS_CCW_CSSID_UNRESTRICTED, QEMU_CAPS_DEVICE_VFIO_CCW); - DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, + DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, GIC_NONE, + DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, GIC_NONE, + DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, GIC_NONE, + DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate-numa-unaligned", "stdio", 7, 0, 0, GIC_NONE, + DO_TEST_FULL("migrate-numa-unaligned", "stdio", 7, 0, 0, ARG_QEMU_CAPS, QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM); @@ -1748,11 +1754,10 @@ mymain(void) DO_TEST("cpu-host-model-vendor", NONE); DO_TEST_FULL("cpu-host-model-fallback", NULL, -1, FLAG_SKIP_LEGACY_CPUS, 0, - GIC_NONE, ARG_QEMU_CAPS, NONE); DO_TEST_FULL("cpu-host-model-nofallback", NULL, -1, FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, - 0, GIC_NONE, + 0, ARG_QEMU_CAPS, NONE); DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM); DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM); @@ -2842,7 +2847,7 @@ mymain(void) QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_PIIX3_USB_UHCI); DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", NULL, -1, 0, - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, GIC_NONE, + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, ARG_QEMU_CAPS, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_NEC_USB_XHCI, -- 2.20.1
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:
Subject: [libvirt] [PATCH 06/21] tests: qemuxml2argv: handle gic with vaargs
This allows us to drop stub gic values from DO_TEST_FULL calls
s/vaargs/varargs/ s/gic/GIC/ Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This allows us to drop migrateFrom and migrateFd from DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 46 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 60df2c3a00..c6bd240c54 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -608,6 +608,8 @@ testCompareXMLToArgv(const void *data) typedef enum { ARG_QEMU_CAPS = 1, ARG_GIC, + ARG_MIGRATE_FROM, + ARG_MIGRATE_FD, ARG_END = QEMU_CAPS_LAST, } testInfoArgNames; @@ -632,6 +634,14 @@ testInfoSetArgs(struct testInfo *info, ...) goto cleanup; break; + case ARG_MIGRATE_FROM: + info->migrateFrom = va_arg(argptr, char *); + break; + + case ARG_MIGRATE_FD: + info->migrateFd = va_arg(argptr, int); + break; + case ARG_END: default: fprintf(stderr, "Unexpected test info argument"); @@ -822,11 +832,10 @@ mymain(void) # define DO_TEST_CAPS(name, ver) \ DO_TEST_CAPS_FULL(name, 0, 0, ver) -# define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ - parseFlags, ...) \ +# define DO_TEST_FULL(name, flags, parseFlags, ...) \ do { \ static struct testInfo info = { \ - name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ + name, NULL, NULL, NULL, -1, (flags), parseFlags, \ }; \ if (!(info.qemuCaps = virQEMUCapsNew())) \ return EXIT_FAILURE; \ @@ -839,27 +848,27 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, \ + DO_TEST_FULL(name, 0, 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_GIC(name, gic, ...) \ - DO_TEST_FULL(name, NULL, -1, 0, 0, \ + DO_TEST_FULL(name, 0, 0, \ ARG_GIC, gic, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ - DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, \ + DO_TEST_FULL(name, FLAG_EXPECT_FAILURE, \ 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ - DO_TEST_FULL(name, NULL, -1, \ + DO_TEST_FULL(name, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ - DO_TEST_FULL(name, NULL, -1, \ + DO_TEST_FULL(name, \ FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ parseFlags, \ ARG_QEMU_CAPS, __VA_ARGS__) @@ -1700,16 +1709,21 @@ mymain(void) QEMU_CAPS_CCW_CSSID_UNRESTRICTED, QEMU_CAPS_DEVICE_VFIO_CCW); - DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, + DO_TEST_FULL("restore-v2", 0, 0, + ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, 0, + ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, 0, + ARG_MIGRATE_FROM, "fd:7", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate", "tcp:10.0.0.1:5000", -1, 0, 0, + DO_TEST_FULL("migrate", 0, 0, + ARG_MIGRATE_FROM, "tcp:10.0.0.1:5000", ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate-numa-unaligned", "stdio", 7, 0, 0, + DO_TEST_FULL("migrate-numa-unaligned", 0, 0, + ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM); @@ -1752,10 +1766,10 @@ mymain(void) DO_TEST("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("cpu-host-model", NONE); DO_TEST("cpu-host-model-vendor", NONE); - DO_TEST_FULL("cpu-host-model-fallback", NULL, -1, + DO_TEST_FULL("cpu-host-model-fallback", FLAG_SKIP_LEGACY_CPUS, 0, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("cpu-host-model-nofallback", NULL, -1, + DO_TEST_FULL("cpu-host-model-nofallback", FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, 0, ARG_QEMU_CAPS, NONE); @@ -2846,7 +2860,7 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_PIIX3_USB_UHCI); - DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", NULL, -1, 0, + DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", 0, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, ARG_QEMU_CAPS, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
- DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, + DO_TEST_FULL("restore-v2", 0, 0, + ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, 0, + ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", "fd:7", 7, 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, 0, + ARG_MIGRATE_FROM, "fd:7", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE);
I'd put ARG_MIGRATE_FROM and ARG_MIGRATE_FD on separate lines here. Either way, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
This allows us to drop migrateFrom and migrateFd from DO_TEST_FULL
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 46 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 16 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Tue, 2019-03-19 at 14:10 +0100, Andrea Bolognani wrote:
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
This allows us to drop migrateFrom and migrateFd from DO_TEST_FULL
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 46 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 16 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Disregard this message - I replied to the wrong patch by mistake. -- Andrea Bolognani / Red Hat / Virtualization
This allows us to drop flags from DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c6bd240c54..bb3d53070f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -610,6 +610,7 @@ typedef enum { ARG_GIC, ARG_MIGRATE_FROM, ARG_MIGRATE_FD, + ARG_FLAGS, ARG_END = QEMU_CAPS_LAST, } testInfoArgNames; @@ -642,6 +643,10 @@ testInfoSetArgs(struct testInfo *info, ...) info->migrateFd = va_arg(argptr, int); break; + case ARG_FLAGS: + info->flags = va_arg(argptr, int); + break; + case ARG_END: default: fprintf(stderr, "Unexpected test info argument"); @@ -832,10 +837,10 @@ mymain(void) # define DO_TEST_CAPS(name, ver) \ DO_TEST_CAPS_FULL(name, 0, 0, ver) -# define DO_TEST_FULL(name, flags, parseFlags, ...) \ +# define DO_TEST_FULL(name, parseFlags, ...) \ do { \ static struct testInfo info = { \ - name, NULL, NULL, NULL, -1, (flags), parseFlags, \ + name, NULL, NULL, NULL, -1, 0, parseFlags, \ }; \ if (!(info.qemuCaps = virQEMUCapsNew())) \ return EXIT_FAILURE; \ @@ -848,29 +853,30 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, 0, 0, \ + DO_TEST_FULL(name, 0, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_GIC(name, gic, ...) \ - DO_TEST_FULL(name, 0, 0, \ + DO_TEST_FULL(name, 0, \ ARG_GIC, gic, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ - DO_TEST_FULL(name, FLAG_EXPECT_FAILURE, \ + DO_TEST_FULL(name, \ 0, \ + ARG_FLAGS, FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, \ - FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ 0, \ + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ DO_TEST_FULL(name, \ - FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ parseFlags, \ + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) # define NONE QEMU_CAPS_LAST @@ -1709,20 +1715,20 @@ mymain(void) QEMU_CAPS_CCW_CSSID_UNRESTRICTED, QEMU_CAPS_DEVICE_VFIO_CCW); - DO_TEST_FULL("restore-v2", 0, 0, + DO_TEST_FULL("restore-v2", 0, ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", 0, 0, + DO_TEST_FULL("restore-v2-fd", 0, ARG_MIGRATE_FROM, "fd:7", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate", 0, 0, + DO_TEST_FULL("migrate", 0, ARG_MIGRATE_FROM, "tcp:10.0.0.1:5000", ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate-numa-unaligned", 0, 0, + DO_TEST_FULL("migrate-numa-unaligned", 0, ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, QEMU_CAPS_NUMA, @@ -1767,11 +1773,12 @@ mymain(void) DO_TEST("cpu-host-model", NONE); DO_TEST("cpu-host-model-vendor", NONE); DO_TEST_FULL("cpu-host-model-fallback", - FLAG_SKIP_LEGACY_CPUS, 0, + 0, + ARG_FLAGS, FLAG_SKIP_LEGACY_CPUS, ARG_QEMU_CAPS, NONE); DO_TEST_FULL("cpu-host-model-nofallback", - FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, 0, + ARG_FLAGS, FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, ARG_QEMU_CAPS, NONE); DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM); DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM); @@ -2860,7 +2867,7 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_PIIX3_USB_UHCI); - DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", 0, + DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, ARG_QEMU_CAPS, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
This allows us to drop flags from DO_TEST_FULL
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This allows us to drop parseFlags from DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bb3d53070f..070f7f0a05 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -611,6 +611,7 @@ typedef enum { ARG_MIGRATE_FROM, ARG_MIGRATE_FD, ARG_FLAGS, + ARG_PARSEFLAGS, ARG_END = QEMU_CAPS_LAST, } testInfoArgNames; @@ -647,6 +648,10 @@ testInfoSetArgs(struct testInfo *info, ...) info->flags = va_arg(argptr, int); break; + case ARG_PARSEFLAGS: + info->parseFlags = va_arg(argptr, int); + break; + case ARG_END: default: fprintf(stderr, "Unexpected test info argument"); @@ -837,10 +842,10 @@ mymain(void) # define DO_TEST_CAPS(name, ver) \ DO_TEST_CAPS_FULL(name, 0, 0, ver) -# define DO_TEST_FULL(name, parseFlags, ...) \ +# define DO_TEST_FULL(name, ...) \ do { \ static struct testInfo info = { \ - name, NULL, NULL, NULL, -1, 0, parseFlags, \ + name, NULL, NULL, NULL, -1, 0, 0, \ }; \ if (!(info.qemuCaps = virQEMUCapsNew())) \ return EXIT_FAILURE; \ @@ -853,29 +858,27 @@ mymain(void) } while (0) # define DO_TEST(name, ...) \ - DO_TEST_FULL(name, 0, \ + DO_TEST_FULL(name, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_GIC(name, gic, ...) \ - DO_TEST_FULL(name, 0, \ + DO_TEST_FULL(name, \ ARG_GIC, gic, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_FAILURE(name, ...) \ DO_TEST_FULL(name, \ - 0, \ ARG_FLAGS, FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, \ - 0, \ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ DO_TEST_FULL(name, \ - parseFlags, \ + ARG_PARSEFLAGS, parseFlags, \ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) @@ -1715,20 +1718,20 @@ mymain(void) QEMU_CAPS_CCW_CSSID_UNRESTRICTED, QEMU_CAPS_DEVICE_VFIO_CCW); - DO_TEST_FULL("restore-v2", 0, + DO_TEST_FULL("restore-v2", ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", 0, + DO_TEST_FULL("restore-v2-fd", ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("restore-v2-fd", 0, + DO_TEST_FULL("restore-v2-fd", ARG_MIGRATE_FROM, "fd:7", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate", 0, + DO_TEST_FULL("migrate", ARG_MIGRATE_FROM, "tcp:10.0.0.1:5000", ARG_QEMU_CAPS, NONE); - DO_TEST_FULL("migrate-numa-unaligned", 0, + DO_TEST_FULL("migrate-numa-unaligned", ARG_MIGRATE_FROM, "stdio", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, QEMU_CAPS_NUMA, @@ -1773,11 +1776,9 @@ mymain(void) DO_TEST("cpu-host-model", NONE); DO_TEST("cpu-host-model-vendor", NONE); DO_TEST_FULL("cpu-host-model-fallback", - 0, ARG_FLAGS, FLAG_SKIP_LEGACY_CPUS, ARG_QEMU_CAPS, NONE); DO_TEST_FULL("cpu-host-model-nofallback", - 0, ARG_FLAGS, FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE, ARG_QEMU_CAPS, NONE); DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM); @@ -2868,7 +2869,7 @@ mymain(void) QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_PIIX3_USB_UHCI); DO_TEST_FULL("ppc64-usb-controller-qemu-xhci", - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, ARG_QEMU_CAPS, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_NEC_USB_XHCI, -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
This allows us to drop parseFlags from DO_TEST_FULL
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
It only has one caller. Just use DO_TEST_FULL Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 070f7f0a05..1ce8bade48 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -876,12 +876,6 @@ mymain(void) ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ ARG_QEMU_CAPS, __VA_ARGS__) -# define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ - DO_TEST_FULL(name, \ - ARG_PARSEFLAGS, parseFlags, \ - ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ - ARG_QEMU_CAPS, __VA_ARGS__) - # define NONE QEMU_CAPS_LAST /* Unset or set all envvars here that are copied in qemudBuildCommandLine @@ -2887,9 +2881,10 @@ mymain(void) /* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag * will avoid the error. Still, we expect qemu driver to complain about * missing machine error, and not crash */ - DO_TEST_PARSE_FLAGS_ERROR("missing-machine", - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE, - NONE); + DO_TEST_FULL("missing-machine", + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE, + ARG_QEMU_CAPS, NONE); DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+ DO_TEST_FULL("missing-machine", + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE, + ARG_QEMU_CAPS, NONE);
Technically speaking you should be able to leave ARG_QEMU_CAPS out here, but it's okay if you prefer to leave it in. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 3/19/19 9:24 AM, Andrea Bolognani wrote:
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+ DO_TEST_FULL("missing-machine", + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE, + ARG_QEMU_CAPS, NONE);
Technically speaking you should be able to leave ARG_QEMU_CAPS out here, but it's okay if you prefer to leave it in.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The other DO_TEST_FULL instances still have it explicitly listed, so I left it in for this one too Thanks, Cole
They are potentially useful at the moment, but we will be making things much more flexible Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1ce8bade48..130ab29d6e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -832,16 +832,6 @@ mymain(void) # define DO_TEST_CAPS_LATEST_PARSE_ERROR(name) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0) -/** - * The following test macros should be used only in cases when the tests require - * testing of some non-standard combination of capability flags - */ -# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \ - DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver) - -# define DO_TEST_CAPS(name, ver) \ - DO_TEST_CAPS_FULL(name, 0, 0, ver) - # define DO_TEST_FULL(name, ...) \ do { \ static struct testInfo info = { \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
-# define DO_TEST_CAPS(name, ver) \ - DO_TEST_CAPS_FULL(name, 0, 0, ver)
I fail to see why this one existed in the first place - isn't it basically the same as DO_TEST_CAPS_VER()? Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Mar 19, 2019 at 02:31:47PM +0100, Andrea Bolognani wrote:
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
-# define DO_TEST_CAPS(name, ver) \ - DO_TEST_CAPS_FULL(name, 0, 0, ver)
I fail to see why this one existed in the first place - isn't it basically the same as DO_TEST_CAPS_VER()?
Probably a leftover from earlier versions. It is unused since its introduction in: commit 976fbb98c8dbccbba93fb5f5a8bb2f94df0be346 tests: qemuxml2argv: Add infrastructure for testing with real qemuCaps Jano
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 130ab29d6e..b555166e82 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -832,6 +832,9 @@ mymain(void) # define DO_TEST_CAPS_LATEST_PARSE_ERROR(name) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0) + +/* All the following macros require an explicit QEMU_CAPS_* list + * at the end of the argument list, or the NONE placeholder */ # define DO_TEST_FULL(name, ...) \ do { \ static struct testInfo info = { \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 130ab29d6e..b555166e82 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -832,6 +832,9 @@ mymain(void) # define DO_TEST_CAPS_LATEST_PARSE_ERROR(name) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0)
+ +/* All the following macros require an explicit QEMU_CAPS_* list + * at the end of the argument list, or the NONE placeholder */ # define DO_TEST_FULL(name, ...) \ do { \ static struct testInfo info = { \
The comment does actually not apply to DO_TEST_FULL(), which expects a series of ARG_* and corresponding arguments rather than a list of QEMU_CAPS_*, so you should move it one macro down - right before DO_TEST() - instead. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b555166e82..7708e8db70 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -786,11 +786,11 @@ mymain(void) * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate * version. */ -# define DO_TEST_CAPS_INTERNAL(name, suffix, migrateFrom, flags, parseFlags, \ +# define DO_TEST_CAPS_INTERNAL(name, suffix, flags, parseFlags, \ arch, capsfile, stripmachinealiases) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, migrateFrom, migrateFrom ? 7 : -1,\ + name, "." suffix, NULL, NULL, -1,\ (flags | FLAG_REAL_CAPS), parseFlags, \ }; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ @@ -807,7 +807,7 @@ mymain(void) # define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" # define DO_TEST_CAPS_ARCH_VER_FULL(name, flags, parseFlags, arch, ver) \ - DO_TEST_CAPS_INTERNAL(name, arch "-" ver, NULL, flags, parseFlags, \ + DO_TEST_CAPS_INTERNAL(name, arch "-" ver, flags, parseFlags, \ arch, TEST_CAPS_PATH ver "." arch ".xml", false) # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ @@ -817,7 +817,7 @@ mymain(void) DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver) # define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \ - DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, flags, parseFlags, arch, \ + DO_TEST_CAPS_INTERNAL(name, arch "-latest", flags, parseFlags, arch, \ virHashLookup(capslatest, arch), true) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7708e8db70..176e4eff3e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -786,18 +786,20 @@ mymain(void) * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate * version. */ -# define DO_TEST_CAPS_INTERNAL(name, suffix, flags, parseFlags, \ - arch, capsfile, stripmachinealiases) \ +# define DO_TEST_CAPS_INTERNAL(name, suffix, \ + arch, capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, NULL, -1,\ - (flags | FLAG_REAL_CAPS), parseFlags, \ + name, "." suffix, NULL, NULL, -1, 0, 0, \ }; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ return EXIT_FAILURE; \ if (stripmachinealiases) \ virQEMUCapsStripMachineAliases(info.qemuCaps); \ + if (testInfoSetArgs(&info, __VA_ARGS__, ARG_END) < 0) \ + return EXIT_FAILURE; \ + info.flags |= FLAG_REAL_CAPS; \ if (virTestRun("QEMU XML-2-ARGV " name "." suffix, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ @@ -806,31 +808,35 @@ mymain(void) # define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" -# define DO_TEST_CAPS_ARCH_VER_FULL(name, flags, parseFlags, arch, ver) \ - DO_TEST_CAPS_INTERNAL(name, arch "-" ver, flags, parseFlags, \ - arch, TEST_CAPS_PATH ver "." arch ".xml", false) +# define DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch "-" ver, \ + arch, TEST_CAPS_PATH ver "." arch ".xml", false, \ + __VA_ARGS__) # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ - DO_TEST_CAPS_ARCH_VER_FULL(name, 0, 0, arch, ver) + DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END) # define DO_TEST_CAPS_VER(name, ver) \ DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver) -# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, flags, parseFlags) \ - DO_TEST_CAPS_INTERNAL(name, arch "-latest", flags, parseFlags, arch, \ - virHashLookup(capslatest, arch), true) +# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch "-latest", arch, \ + virHashLookup(capslatest, arch), true, \ + __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ - DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) # define DO_TEST_CAPS_LATEST(name) \ DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") # define DO_TEST_CAPS_LATEST_FAILURE(name) \ - DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_FAILURE, 0) + DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", \ + ARG_FLAGS, FLAG_EXPECT_FAILURE) # define DO_TEST_CAPS_LATEST_PARSE_ERROR(name) \ - DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", FLAG_EXPECT_PARSE_ERROR, 0) + DO_TEST_CAPS_ARCH_LATEST_FULL(name, "x86_64", \ + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR) /* All the following macros require an explicit QEMU_CAPS_* list -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+# define DO_TEST_CAPS_INTERNAL(name, suffix, \ + arch, capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, NULL, -1,\ - (flags | FLAG_REAL_CAPS), parseFlags, \ + name, "." suffix, NULL, NULL, -1, 0, 0, \ }; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ return EXIT_FAILURE; \ if (stripmachinealiases) \ virQEMUCapsStripMachineAliases(info.qemuCaps); \ + if (testInfoSetArgs(&info, __VA_ARGS__, ARG_END) < 0) \ + return EXIT_FAILURE; \
Since you already have an explicit ARG_END here, it's kinda ugly that you need another one... [...]
+# define DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch "-" ver, \ + arch, TEST_CAPS_PATH ver "." arch ".xml", false, \ + __VA_ARGS__)
# define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ - DO_TEST_CAPS_ARCH_VER_FULL(name, 0, 0, arch, ver) + DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END)
... both here... [...]
+# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch "-latest", arch, \ + virHashLookup(capslatest, arch), true, \ + __VA_ARGS__)
# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ - DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, 0, 0) + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END)
... and here, but the underlying macro can't take zero variadic arguments so there's no way around it I guess... Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Only initialize the fields that are passed in Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 176e4eff3e..b1c18c6c09 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -786,11 +786,12 @@ mymain(void) * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate * version. */ -# define DO_TEST_CAPS_INTERNAL(name, suffix, \ +# define DO_TEST_CAPS_INTERNAL(_name, _suffix, \ arch, capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, NULL, -1, 0, 0, \ + .name = _name, \ + .suffix = "." _suffix, \ }; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ @@ -800,7 +801,7 @@ mymain(void) if (testInfoSetArgs(&info, __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ info.flags |= FLAG_REAL_CAPS; \ - if (virTestRun("QEMU XML-2-ARGV " name "." suffix, \ + if (virTestRun("QEMU XML-2-ARGV " _name "." _suffix, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ virObjectUnref(info.qemuCaps); \ @@ -841,16 +842,16 @@ mymain(void) /* All the following macros require an explicit QEMU_CAPS_* list * at the end of the argument list, or the NONE placeholder */ -# define DO_TEST_FULL(name, ...) \ +# define DO_TEST_FULL(_name, ...) \ do { \ static struct testInfo info = { \ - name, NULL, NULL, NULL, -1, 0, 0, \ + .name = _name, \ }; \ if (!(info.qemuCaps = virQEMUCapsNew())) \ return EXIT_FAILURE; \ if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ - if (virTestRun("QEMU XML-2-ARGV " name, \ + if (virTestRun("QEMU XML-2-ARGV " _name, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ virObjectUnref(info.qemuCaps); \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+# define DO_TEST_CAPS_INTERNAL(_name, _suffix, \ arch, capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, NULL, -1, 0, 0, \ + .name = _name, \ + .suffix = "." _suffix, \ }; \
It would probably have been a lot better to perform this kind of conversion early in the series, and then gradually get rid of the initialization for single members at the same time as support for the corresponding ARG_* was implemented... But I'm not gonna ask you to go back and do that :) Note that we're also effectively changing the default value for migrateFd from -1 to 0, but doing so doesn't cause any failures since the value is only used when migrateFrom is "stdio", and in all test cases where that is true we also already set migrateFd explicitly. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 3/19/19 10:12 AM, Andrea Bolognani wrote:
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+# define DO_TEST_CAPS_INTERNAL(_name, _suffix, \ arch, capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ - name, "." suffix, NULL, NULL, -1, 0, 0, \ + .name = _name, \ + .suffix = "." _suffix, \ }; \
It would probably have been a lot better to perform this kind of conversion early in the series, and then gradually get rid of the initialization for single members at the same time as support for the corresponding ARG_* was implemented... But I'm not gonna ask you to go back and do that :)
Thank you :)
Note that we're also effectively changing the default value for migrateFd from -1 to 0, but doing so doesn't cause any failures since the value is only used when migrateFrom is "stdio", and in all test cases where that is true we also already set migrateFd explicitly.
Hmm I didn't notice that... but since like you say it doesn't cause test failures, I'll leave it as is Thanks for the reviews. I fixed all the other issues in patches 1-19 and pushed. I'll send a v2 with patch #20+ fixed Thanks, Cole
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b1c18c6c09..4b34bbcb63 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -786,12 +786,12 @@ mymain(void) * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate * version. */ -# define DO_TEST_CAPS_INTERNAL(_name, _suffix, \ - arch, capsfile, stripmachinealiases, ...) \ +# define DO_TEST_CAPS_INTERNAL(_name, arch, ver, \ + capsfile, stripmachinealiases, ...) \ do { \ static struct testInfo info = { \ .name = _name, \ - .suffix = "." _suffix, \ + .suffix = "." arch "-" ver, \ }; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ @@ -801,7 +801,7 @@ mymain(void) if (testInfoSetArgs(&info, __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ info.flags |= FLAG_REAL_CAPS; \ - if (virTestRun("QEMU XML-2-ARGV " _name "." _suffix, \ + if (virTestRun("QEMU XML-2-ARGV " _name "." arch "-" ver, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ virObjectUnref(info.qemuCaps); \ @@ -810,8 +810,8 @@ mymain(void) # define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" # define DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ...) \ - DO_TEST_CAPS_INTERNAL(name, arch "-" ver, \ - arch, TEST_CAPS_PATH ver "." arch ".xml", false, \ + DO_TEST_CAPS_INTERNAL(name, arch, ver, \ + TEST_CAPS_PATH ver "." arch ".xml", false, \ __VA_ARGS__) # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ @@ -821,7 +821,7 @@ mymain(void) DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver) # define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ - DO_TEST_CAPS_INTERNAL(name, arch "-latest", arch, \ + DO_TEST_CAPS_INTERNAL(name, arch, "latest", \ virHashLookup(capslatest, arch), true, \ __VA_ARGS__) -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Rather than make callers do it. The operative info is just arch and ver which we are passing in already. Fold in stripmachinealiases too since it is just dependent on ver value Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4b34bbcb63..496e33f4e5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -786,13 +786,20 @@ mymain(void) * the test cases should be forked using DO_TEST_CAPS_VER with the appropriate * version. */ -# define DO_TEST_CAPS_INTERNAL(_name, arch, ver, \ - capsfile, stripmachinealiases, ...) \ +# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" + +# define DO_TEST_CAPS_INTERNAL(_name, arch, ver, ...) \ do { \ static struct testInfo info = { \ .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; \ @@ -807,23 +814,15 @@ mymain(void) virObjectUnref(info.qemuCaps); \ } while (0) -# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" - -# define DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ...) \ - DO_TEST_CAPS_INTERNAL(name, arch, ver, \ - TEST_CAPS_PATH ver "." arch ".xml", false, \ - __VA_ARGS__) # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ - DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END) + DO_TEST_CAPS_INTERNAL(name, arch, ver, ARG_END) # define DO_TEST_CAPS_VER(name, ver) \ DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver) # define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ - DO_TEST_CAPS_INTERNAL(name, arch, "latest", \ - virHashLookup(capslatest, arch), true, \ - __VA_ARGS__) + DO_TEST_CAPS_INTERNAL(name, arch, "latest", __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
Rather than make callers do it. The operative info is just arch and ver which we are passing in already.
Fold in stripmachinealiases too since it is just dependent on ver value
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This is closer to the pattern of qemuxml2xml tests, and will make things easier if we extend testInfo to contain more freeable data Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 496e33f4e5..813871d6b8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -665,6 +665,12 @@ testInfoSetArgs(struct testInfo *info, ...) return ret; } +static void +testInfoClear(struct testInfo *info) +{ + virObjectUnref(info->qemuCaps); +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -811,10 +817,9 @@ mymain(void) if (virTestRun("QEMU XML-2-ARGV " _name "." arch "-" ver, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ - virObjectUnref(info.qemuCaps); \ + testInfoClear(&info); \ } while (0) - # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_INTERNAL(name, arch, ver, ARG_END) @@ -853,7 +858,7 @@ mymain(void) if (virTestRun("QEMU XML-2-ARGV " _name, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ - virObjectUnref(info.qemuCaps); \ + testInfoClear(&info); \ } while (0) # define DO_TEST(name, ...) \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
This is closer to the pattern of qemuxml2xml tests, and will make things easier if we extend testInfo to contain more freeable data
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Move DO_TEST* qemuCaps init into testInfoSetArgs. This is a step towards unifying the different test macro implementations Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 813871d6b8..ff74892944 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -621,19 +621,21 @@ testInfoSetArgs(struct testInfo *info, ...) { va_list argptr; testInfoArgNames argname; + virQEMUCapsPtr qemuCaps = NULL; + int gic = GIC_NONE; int ret = -1; va_start(argptr, info); while ((argname = va_arg(argptr, int)) < ARG_END) { switch (argname) { case ARG_QEMU_CAPS: - virQEMUCapsSetVList(info->qemuCaps, argptr); + if (qemuCaps || !(qemuCaps = virQEMUCapsNew())) + goto cleanup; + virQEMUCapsSetVList(qemuCaps, argptr); break; case ARG_GIC: - if (testQemuCapsSetGIC(info->qemuCaps, - va_arg(argptr, int)) < 0) - goto cleanup; + gic = va_arg(argptr, int); break; case ARG_MIGRATE_FROM: @@ -659,8 +661,20 @@ testInfoSetArgs(struct testInfo *info, ...) } } + if (!info->qemuCaps) { + 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; + ret = 0; cleanup: + virObjectUnref(qemuCaps); va_end(argptr); return ret; } @@ -851,8 +865,6 @@ mymain(void) static struct testInfo info = { \ .name = _name, \ }; \ - if (!(info.qemuCaps = virQEMUCapsNew())) \ - return EXIT_FAILURE; \ if (testInfoSetArgs(&info, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END) < 0) \ return EXIT_FAILURE; \ if (virTestRun("QEMU XML-2-ARGV " _name, \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+ if (gic && testQemuCapsSetGIC(info->qemuCaps, gic) < 0) + goto cleanup;
Please change the first part of the condition to 'gic != GIC_NONE' so that it doesn't implicitly rely on the actual value. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
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(-) 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) \ -- 2.20.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
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
Base macro to unify the actual testCompareXMLToArgv test calls Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvtest.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d5e02c26d8..a901582ef6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -842,23 +842,27 @@ mymain(void) * version. */ -# define DO_TEST_CAPS_INTERNAL(_name, arch, ver, ...) \ +# define TEST_INTERNAL(_name, _suffix, ...) \ do { \ static struct testInfo info = { \ .name = _name, \ - .suffix = "." arch "-" ver, \ + .suffix = _suffix, \ }; \ if (testInfoSetArgs(&info, capslatest, \ - ARG_CAPS_ARCH, arch, \ - ARG_CAPS_VER, ver, \ __VA_ARGS__, ARG_END) < 0) \ return EXIT_FAILURE; \ - if (virTestRun("QEMU XML-2-ARGV " _name "." arch "-" ver, \ + if (virTestRun("QEMU XML-2-ARGV " _name _suffix, \ testCompareXMLToArgv, &info) < 0) \ ret = -1; \ testInfoClear(&info); \ } while (0) +# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + TEST_INTERNAL(name, "." arch "-" ver, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__) + # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_INTERNAL(name, arch, ver, ARG_END) @@ -885,19 +889,8 @@ mymain(void) /* All the following macros require an explicit QEMU_CAPS_* list * at the end of the argument list, or the NONE placeholder */ -# define DO_TEST_FULL(_name, ...) \ - do { \ - static struct testInfo info = { \ - .name = _name, \ - }; \ - 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) \ - ret = -1; \ - testInfoClear(&info); \ - } while (0) +# define DO_TEST_FULL(name, ...) \ + TEST_INTERNAL(name, "", __VA_ARGS__, QEMU_CAPS_LAST) # define DO_TEST(name, ...) \ DO_TEST_FULL(name, \ -- 2.20.1
On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote: [...]
+# define TEST_INTERNAL(_name, _suffix, ...) \
This macro should be called DO_TEST_INTERNAL() - you're still running the test from it, so the DO_ prefix is fully warranted. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 3/14/19 9:43 AM, Cole Robinson wrote:
Right now in qemuxml2argv we have a proliferation of DO_*TEST* macros. They essentially fill in data in the testInfo struct and invoke the test function.
There's several bits of data that we want to specify for a small subset of tests: flags, parseFlags, migrateFrom/migrateFd, gic. The base macros need to handle these in their argument lists, and we provide many convenience macros that fill in default values for the less common parameters. Any time we want to add a new bit of data though, the base macros are extended, and every caller of those needs to be adjusted, and we are faced with the question of whether to extend the combinatorial explosion of convenience macros which have only a subset of options exposed.
This series adds a testInfoSetArgs which uses va_args to make these mandatory parameters optional. The general format is:
DO_TEST_FULL(... ARG_FOO, foovalue, ARG_BAR, barvalue, ...)
Specifically, one of the migrate tests went from:
DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE);
to:
DO_TEST_FULL("restore-v2", ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE);
Umm, what's your sentinel value here? It is undefined behavior to call va_arg more times than there are arguments present, but without some sort of sentinel, you don't know when to stop calling va_arg. The overall idea is nice, but I think you need an ARG_END sentinel. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 3/14/19 11:17 AM, Eric Blake wrote:
On 3/14/19 9:43 AM, Cole Robinson wrote:
Right now in qemuxml2argv we have a proliferation of DO_*TEST* macros. They essentially fill in data in the testInfo struct and invoke the test function.
There's several bits of data that we want to specify for a small subset of tests: flags, parseFlags, migrateFrom/migrateFd, gic. The base macros need to handle these in their argument lists, and we provide many convenience macros that fill in default values for the less common parameters. Any time we want to add a new bit of data though, the base macros are extended, and every caller of those needs to be adjusted, and we are faced with the question of whether to extend the combinatorial explosion of convenience macros which have only a subset of options exposed.
This series adds a testInfoSetArgs which uses va_args to make these mandatory parameters optional. The general format is:
DO_TEST_FULL(... ARG_FOO, foovalue, ARG_BAR, barvalue, ...)
Specifically, one of the migrate tests went from:
DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE);
to:
DO_TEST_FULL("restore-v2", ARG_MIGRATE_FROM, "exec:cat", ARG_MIGRATE_FD, 7, ARG_QEMU_CAPS, NONE);
Umm, what's your sentinel value here? It is undefined behavior to call va_arg more times than there are arguments present, but without some sort of sentinel, you don't know when to stop calling va_arg.
The overall idea is nice, but I think you need an ARG_END sentinel.
You saw it in the later patches, but just to respond here if anyone else is watching: there is an ARG_END but it's hidden in the macro definitions. - Cole - Cole
participants (4)
-
Andrea Bolognani -
Cole Robinson -
Eric Blake -
Ján Tomko