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@xxxxxxxxxx> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
