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