Re: [libvirt] [PATCH 03/21] tests: qemuxml2argv: add va_arg enum handling
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