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

[Cole Robinson]

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 at 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