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