[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 07/24] tests: utils: Add virTestLoadFilePath helper



On 07/26/2017 08:39 AM, Eric Blake wrote:
> On 07/26/2017 05:00 AM, Peter Krempa wrote:
>> This new helper loads and returns a file from 'abs_srcdir'. By using
>> variable arguments for the function, it's not necessary to format the
>> path separately in the test cases.
>> ---
>>  tests/testutils.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/testutils.h |  2 ++
>>  2 files changed, 53 insertions(+)
>>
> 
>> +
>> +/**
>> + * virTestLoadFilePath:
>> + * @...: file name components.
> 
> Mention that it must end in NULL...
> 
>> + *
>> + * Constructs the test file path from variable arguments and loads the file.
>> + * 'abs_srcdir' is automatically prepended.
>> + */
>> +char *
>> +virTestLoadFilePath(const char *p, ...)
> 
> and gcc has an attribute to mark vararg functions that require a NULL
> sentinel, to let the compiler enforce correct usage.

Looking back at the patch, I see you did use it, but that I missed it
because it was must later in the email:

> 
> --- a/tests/testutils.h
> +++ b/tests/testutils.h
> @@ -52,6 +52,8 @@ int virTestRun(const char *title,
>                 int (*body)(const void *data),
>                 const void *data);
>  int virTestLoadFile(const char *file, char **buf);
> +char *virTestLoadFilePath(const char *p, ...)
> +    ATTRIBUTE_SENTINEL;

I like to use git's orderfile directive, so that my patches always list
.h changes first (when reviewing, it's nicer to see the interface
changes before the implementations); maybe libvirt should copy this idea
from qemu:
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg06438.html

So I still think the comment should mention that the list must end in
NULL, but with that, you can add

Reviewed-by: Eric Blake <eblake redhat com>

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]