diff mbox series

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

Message ID 185092267f0b872f526bd5b9f17372ce252b53da.1552574299.git.crobinso@redhat.com
State New
Headers show
Series tests: qemuxml2argv: support optional arguments | expand

Commit Message

Cole Robinson March 14, 2019, 2:43 p.m. UTC 
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@redhat.com>

---
 tests/qemuxml2argvtest.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Eric Blake March 14, 2019, 3:23 p.m. UTC | #1 
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@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?),...

> +            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.

Okay, you can ignore my noise on the cover letter.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 14, 2019, 7:42 p.m. UTC | #2 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Eric Blake March 14, 2019, 8:29 p.m. UTC | #3 
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.
Cole Robinson March 15, 2019, 6:47 p.m. UTC | #4 
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani March 19, 2019, 11:36 a.m. UTC | #5 
On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:
[...]
> +typedef enum {

> +    ARG_QEMU_CAPS = 1,


Any specific reason to start from 1 rather than 0, or even leaving
out the start value entirely?

> +

> +    ARG_END = QEMU_CAPS_LAST,

> +} testInfoArgNames;


The name of the enum should be singular, otherwise it will look
weird when you declare a variable of the type, like...

>  static int

>  testInfoSetArgs(struct testInfo *info, ...)

>  {

>      va_list argptr;

> -    int ret = 0;

> +    testInfoArgNames argname;


... here.

[...]
> +    while ((argname = va_arg(argptr, int)) < ARG_END) {

> +        switch (argname) {


I think you should either call va_arg() with testInfoArgNames as the
second argument, or make the argname variable int and then have an
explicit cast in the switch().

[...]
> +        case ARG_END:

> +        default:

> +            fprintf(stderr, "Unexpected test info argument");

> +            goto cleanup;

> +        }

> +    }

> +

> +    ret = 0;


One extra empty line here, please.


Reviewed-by: Andrea Bolognani <abologna@redhat.com>


-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani March 19, 2019, 12:46 p.m. UTC | #6 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 21, 2019, 4:52 p.m. UTC | #7 
On 3/19/19 7:36 AM, Andrea Bolognani wrote:
> On Thu, 2019-03-14 at 10:43 -0400, Cole Robinson wrote:

> [...]

>> +typedef enum {

>> +    ARG_QEMU_CAPS = 1,

> 

> Any specific reason to start from 1 rather than 0, or even leaving

> out the start value entirely?

>


Nope it's not necessary, I've dropped it. I think it was left over from
earlier patch state

>> +

>> +    ARG_END = QEMU_CAPS_LAST,

>> +} testInfoArgNames;

> 

> The name of the enum should be singular, otherwise it will look

> weird when you declare a variable of the type, like...

> 

>>  static int

>>  testInfoSetArgs(struct testInfo *info, ...)

>>  {

>>      va_list argptr;

>> -    int ret = 0;

>> +    testInfoArgNames argname;

> 

> ... here.

> 

> [...]

>> +    while ((argname = va_arg(argptr, int)) < ARG_END) {

>> +        switch (argname) {

> 

> I think you should either call va_arg() with testInfoArgNames as the

> second argument, or make the argname variable int and then have an

> explicit cast in the switch().

> 


I went with the former

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 3b90cd1873..0dba908c70 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -624,14 +624,35 @@  testCompareXMLToArgv(const void *data)
     return ret;
 }
 
+typedef enum {
+    ARG_QEMU_CAPS = 1,
+
+    ARG_END = QEMU_CAPS_LAST,
+} testInfoArgNames;
+
 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");
+            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) \
             return EXIT_FAILURE; \
         if (virTestRun("QEMU XML-2-ARGV " name, \
                        testCompareXMLToArgv, &info) < 0) \