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