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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
