Re: [libvirt] [PATCH 03/21] tests: qemuxml2argv: add va_arg enum handling
On Fri, 2019-03-15 at 14:47 -0400, Cole Robinson wrote:
On 3/14/19 4:29 PM, Eric Blake wrote:
> On 3/14/19 2:42 PM, Cole Robinson wrote:
> > 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
[...]
+ /* 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.
+ */
Definitely squash in the comment :)
My only concern is that ARG_QEMU_CAPS requires you to be very
careful with its usage: not only you have to make sure you have
QEMU_CAPS_LAST as sentinel, but also you can only reasonably use it
as the last argument because QEMU_CAPS_LAST sharing the same value
as ARG_END would cause a macro like
#define DO_TEST_GIC_BROKEN(name, gic, ...) \
TEST_INTERNAL(name, "", \
ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, \
ARG_GIC, gic)
to ignore the ARG_GIC argument when invoked as
DO_TEST_GIC_BROKEN("broken", GIC_V2, NONE);
I guess it's fair to expect both the developer and the reviewer to
be more careful than usual when touching this kind of dark magic
though, so including the comment is probably enough.
--
Andrea Bolognani / Red Hat / Virtualization