1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 00/24] qemu: refactor node name detection
  4. View patch

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

Peter Krempa posted 24 patches 7 years, 9 months ago
[libvirt] [PATCH 07/24] tests: utils: Add virTestLoadFilePath helper
Posted by Peter Krempa 7 years, 9 months ago 
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(+)

diff --git a/tests/testutils.c b/tests/testutils.c
index 7f1c4672b..f193cdf8b 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -356,6 +356,57 @@ virTestLoadFile(const char *file, char **buf)
     return 0;
 }

+
+static char *
+virTestLoadFileGetPath(const char *p,
+                       va_list ap)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    char *path = NULL;
+
+    virBufferAddLit(&buf, abs_srcdir "/");
+
+    if (p) {
+        virBufferAdd(&buf, p, -1);
+        virBufferStrcatVArgs(&buf, ap);
+    }
+
+    if (!(path = virBufferContentAndReset(&buf)))
+        VIR_TEST_VERBOSE("failed to format file path");
+
+    return path;
+}
+
+
+/**
+ * virTestLoadFilePath:
+ * @...: file name components.
+ *
+ * Constructs the test file path from variable arguments and loads the file.
+ * 'abs_srcdir' is automatically prepended.
+ */
+char *
+virTestLoadFilePath(const char *p, ...)
+{
+    char *path = NULL;
+    char *ret = NULL;
+    va_list ap;
+
+    va_start(ap, p);
+
+    if (!(path = virTestLoadFileGetPath(p, ap)))
+        goto cleanup;
+
+    ignore_value(virTestLoadFile(path, &ret));
+
+ cleanup:
+    va_end(ap);
+    VIR_FREE(path);
+
+    return ret;
+}
+
+
 #ifndef WIN32
 static
 void virTestCaptureProgramExecChild(const char *const argv[],
diff --git a/tests/testutils.h b/tests/testutils.h
index c7f02e468..98dfa990e 100644
--- 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;
 int virTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen);

 void virTestClearCommandPath(char *cmdset);
-- 
2.13.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/24] tests: utils: Add virTestLoadFilePath helper
Posted by Eric Blake 7 years, 9 months ago 
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.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/24] tests: utils: Add virTestLoadFilePath helper
Posted by Eric Blake 7 years, 9 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/24] tests: utils: Add virTestLoadFilePath helper
Posted by Peter Krempa 7 years, 9 months ago 
On Wed, Jul 26, 2017 at 10:46:56 -0500, Eric Blake wrote:
> 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(+)

[...]

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

That is indeed useful. We can also de-prioritize utils and testsuite
changes so that the driver changes pop out first.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.