This removes a little duplication right away, and more
importantly will allow us to perform some interesting
refactoring later on.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 tests/qemucapabilitiestest.c | 15 +++++++++------
 tests/qemucaps2xmltest.c     | 13 +++++++++----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 0f875f9e24..e3c6681dd4 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -35,6 +35,7 @@ typedef struct _testQemuData testQemuData;
 typedef testQemuData *testQemuDataPtr;
 struct _testQemuData {
     virQEMUDriver driver;
+    const char *dataDir;
     const char *archName;
     const char *base;
     int ret;
@@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data)
     if (qemuTestDriverInit(&data->driver) < 0)
         return -1;
 
+    data->dataDir = abs_srcdir "/qemucapabilitiesdata";
+
     data->ret = 0;
 
     return 0;
@@ -73,10 +76,10 @@ testQemuCaps(const void *opaque)
     unsigned int fakeMicrocodeVersion = 0;
     const char *p;
 
-    if (virAsprintf(&repliesFile, "%s/qemucapabilitiesdata/%s.%s.replies",
-                    abs_srcdir, data->base, data->archName) < 0 ||
-        virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml",
-                    abs_srcdir, data->base, data->archName) < 0)
+    if (virAsprintf(&repliesFile, "%s/%s.%s.replies",
+                    data->dataDir, data->base, data->archName) < 0 ||
+        virAsprintf(&capsFile, "%s/%s.%s.xml",
+                    data->dataDir, data->base, data->archName) < 0)
         goto cleanup;
 
     if (!(mon = qemuMonitorTestNewFromFileFull(repliesFile, &data->driver, NULL)))
@@ -141,8 +144,8 @@ testQemuCapsCopy(const void *opaque)
     virQEMUCapsPtr copy = NULL;
     char *actual = NULL;
 
-    if (virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml",
-                    abs_srcdir, data->base, data->archName) < 0)
+    if (virAsprintf(&capsFile, "%s/%s.%s.xml",
+                    data->dataDir, data->base, data->archName) < 0)
         goto cleanup;
 
     if (!(caps = virCapabilitiesNew(virArchFromString(data->archName),
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index e9b0b11e35..46d2ce8b44 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -28,6 +28,8 @@
 typedef struct _testQemuData testQemuData;
 typedef testQemuData *testQemuDataPtr;
 struct _testQemuData {
+    const char *inputDir;
+    const char *outputDir;
     const char *base;
     const char *archName;
     int ret;
@@ -36,6 +38,9 @@ struct _testQemuData {
 static int
 testQemuDataInit(testQemuDataPtr data)
 {
+    data->inputDir = abs_srcdir "/qemucapabilitiesdata";
+    data->outputDir = abs_srcdir "/qemucaps2xmloutdata";
+
     data->ret = 0;
 
     return 0;
@@ -142,12 +147,12 @@ testQemuCapsXML(const void *opaque)
     char *capsXml = NULL;
     virCapsPtr capsProvided = NULL;
 
-    if (virAsprintf(&xmlFile, "%s/qemucaps2xmloutdata/caps.%s.xml",
-                    abs_srcdir, data->archName) < 0)
+    if (virAsprintf(&xmlFile, "%s/caps.%s.xml",
+                    data->outputDir, data->archName) < 0)
         goto cleanup;
 
-    if (virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml",
-                    abs_srcdir, data->base, data->archName) < 0)
+    if (virAsprintf(&capsFile, "%s/%s.%s.xml",
+                    data->inputDir, data->base, data->archName) < 0)
         goto cleanup;
 
     if (virTestLoadFile(capsFile, &capsData) < 0)
-- 
2.20.1

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

On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
> This removes a little duplication right away, and more
> importantly will allow us to perform some interesting
> refactoring later on.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  tests/qemucapabilitiestest.c | 15 +++++++++------
>  tests/qemucaps2xmltest.c     | 13 +++++++++----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
> index 0f875f9e24..e3c6681dd4 100644
> --- a/tests/qemucapabilitiestest.c
> +++ b/tests/qemucapabilitiestest.c

[...]

> @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data)
>      if (qemuTestDriverInit(&data->driver) < 0)
>          return -1;
>  
> +    data->dataDir = abs_srcdir "/qemucapabilitiesdata";
> +

I'm not entirely persuaded that you need to pass this constant in via
the data structure. Even after the last patch the value is never
modified. Could you please elaborate on the refactorings that this will
allow?

Same applies for the other data structure modified in this patch.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2019-03-13 at 09:41 +0100, Peter Krempa wrote:
> On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
> > This removes a little duplication right away, and more
> > importantly will allow us to perform some interesting
> > refactoring later on.
[...]
> > @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data)
> >      if (qemuTestDriverInit(&data->driver) < 0)
> >          return -1;
> >  
> > +    data->dataDir = abs_srcdir "/qemucapabilitiesdata";
> > +
> 
> I'm not entirely persuaded that you need to pass this constant in via
> the data structure. Even after the last patch the value is never
> modified. Could you please elaborate on the refactorings that this will
> allow?

I'll admit I kinda copy-pasted commit messages around, so that's
probably why the one for this patch sounds a bit hyperbolic :)

Anyway, by the end of the series we'll have to pass dataDir to
testQemuCapsIterate() in addition to using it to build the path for
both the input and output file, so it makes sense to store it into
a variable instead of building it from abs_srcdir plus the directory
name two or more times.

As for why it's stored in the structure, I could easily have used a
global variable instead, but this approach seemed cleaner, especially
considering that we're already using the structure to store data that
is similar in scope (the driver).

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
> On Wed, 2019-03-13 at 09:41 +0100, Peter Krempa wrote:
> > On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
> > > This removes a little duplication right away, and more
> > > importantly will allow us to perform some interesting
> > > refactoring later on.
> [...]
> > > @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data)
> > >      if (qemuTestDriverInit(&data->driver) < 0)
> > >          return -1;
> > >  
> > > +    data->dataDir = abs_srcdir "/qemucapabilitiesdata";
> > > +
> > 
> > I'm not entirely persuaded that you need to pass this constant in via
> > the data structure. Even after the last patch the value is never
> > modified. Could you please elaborate on the refactorings that this will
> > allow?
> 
> I'll admit I kinda copy-pasted commit messages around, so that's
> probably why the one for this patch sounds a bit hyperbolic :)
> 
> Anyway, by the end of the series we'll have to pass dataDir to
> testQemuCapsIterate() in addition to using it to build the path for
> both the input and output file, so it makes sense to store it into
> a variable instead of building it from abs_srcdir plus the directory
> name two or more times.
> 
> As for why it's stored in the structure, I could easily have used a
> global variable instead, but this approach seemed cleaner, especially
> considering that we're already using the structure to store data that
> is similar in scope (the driver).

Well, I agree that we should pre-construct the string rather than
constructing it multiple times, but since every use of that string is
inside the function which does the testing there's no point passing it
in via the structure.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2019-03-13 at 10:22 +0100, Peter Krempa wrote:
> On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
> > Anyway, by the end of the series we'll have to pass dataDir to
> > testQemuCapsIterate() in addition to using it to build the path for
> > both the input and output file, so it makes sense to store it into
> > a variable instead of building it from abs_srcdir plus the directory
> > name two or more times.
> > 
> > As for why it's stored in the structure, I could easily have used a
> > global variable instead, but this approach seemed cleaner, especially
> > considering that we're already using the structure to store data that
> > is similar in scope (the driver).
> 
> Well, I agree that we should pre-construct the string rather than
> constructing it multiple times, but since every use of that string is
> inside the function which does the testing there's no point passing it
> in via the structure.

As mentioned above, one of the uses is as an argument to
testQemuCapsIterate(), which needs to be called from main() rather
than from doCapsTest().

So the alternatives are building the string multiple times, using a
global variable or doing what this patch does.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Wed, Mar 13, 2019 at 10:32:59 +0100, Andrea Bolognani wrote:
> On Wed, 2019-03-13 at 10:22 +0100, Peter Krempa wrote:
> > On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
> > > Anyway, by the end of the series we'll have to pass dataDir to
> > > testQemuCapsIterate() in addition to using it to build the path for
> > > both the input and output file, so it makes sense to store it into
> > > a variable instead of building it from abs_srcdir plus the directory
> > > name two or more times.
> > > 
> > > As for why it's stored in the structure, I could easily have used a
> > > global variable instead, but this approach seemed cleaner, especially
> > > considering that we're already using the structure to store data that
> > > is similar in scope (the driver).
> > 
> > Well, I agree that we should pre-construct the string rather than
> > constructing it multiple times, but since every use of that string is
> > inside the function which does the testing there's no point passing it
> > in via the structure.
> 
> As mentioned above, one of the uses is as an argument to
> testQemuCapsIterate(), which needs to be called from main() rather
> than from doCapsTest().

Um, my bad. I did not read the commit message thoroughly apparently.

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