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