[libvirt] [PATCH 03/21] tests: qemuxml2argv: add va_arg enum handling

[Cole Robinson]

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