[libvirt] [PATCH] tests: Don't use canonical paths in virstoragetest

Andrea Bolognani posted 1 patch 12 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190322122124.1754-1-abologna@redhat.com
tests/virstoragetest.c | 50 +++++++++---------------------------------
1 file changed, 10 insertions(+), 40 deletions(-)

[libvirt] [PATCH] tests: Don't use canonical paths in virstoragetest

Posted by Andrea Bolognani 12 weeks ago
The layout of my home directory is somewhat peculiar: I store
all git repositories in ~/src/upstream, but since I spend
almost all of my time hacking on libvirt, I also have a
convenience symlink ~/src/libvirt -> ~/src/upstream/libvirt
that I use to access that specific git repository.

The above setup has served me well for years; however, ever
since commit ca1471622dd9 dropped our own custom definitions
for abs_{,top_}{src,build}dir and started using the ones
provided by autotools, virstoragetest has started reliably
failing with errors such as

   2) Storage backing chain 2 ...
  Offset 0
  Expect [chain member: 0
  path:/home/abologna/src/upstream/libvirt/tests/virstoragedata/raw
  backingStoreRaw: <null>
  capacity: 0
  encryption: 0
  relPath:<null>
  type:1
  format:1
  protocol:none
  hostname:<null>
  ]
  Actual [chain member: 0
  path:/home/abologna/src/libvirt/tests/virstoragedata/raw
  backingStoreRaw: <null>
  capacity: 0
  encryption: 0
  relPath:<null>
  type:1
  format:1
  protocol:none
  hostname:<null>
  ]
                              ... FAILED

Using abolute paths instead of canonical ones in the tests makes
the problem go away.

Note that all tests that are specifically designed to test path
canonicalization via TEST_PATH_CANONICALIZE() were passing even
before this patch and are not touched by it.

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

I'm far from being confident this is the correct approach, but
I've grown annoyed enough by the constant 'make check' failures
that I figured I'd at least get the discussion going :)

 tests/virstoragetest.c | 50 +++++++++---------------------------------
 1 file changed, 10 insertions(+), 40 deletions(-)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index fb98903f02..d2be6fffef 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -46,21 +46,16 @@ VIR_LOG_INIT("tests.storagetest");
  * sub/link2: symlink to wrap
  *
  * Relative names to these files are known at compile time, but absolute
- * and canonical names depend on where the test is run; for convenience,
+ * names depend on where the test is run; for convenience,
  * we pre-populate the computation of these names for use during the test.
 */
 
 static char *qemuimg;
 static char *absraw;
-static char *canonraw;
 static char *absqcow2;
-static char *canonqcow2;
 static char *abswrap;
-static char *canonwrap;
 static char *absqed;
-static char *canonqed;
 static char *absdir;
-static char *canondir;
 static char *abslink2;
 
 static void
@@ -68,15 +63,10 @@ testCleanupImages(void)
 {
     VIR_FREE(qemuimg);
     VIR_FREE(absraw);
-    VIR_FREE(canonraw);
     VIR_FREE(absqcow2);
-    VIR_FREE(canonqcow2);
     VIR_FREE(abswrap);
-    VIR_FREE(canonwrap);
     VIR_FREE(absqed);
-    VIR_FREE(canonqed);
     VIR_FREE(absdir);
-    VIR_FREE(canondir);
     VIR_FREE(abslink2);
 
     if (chdir(abs_builddir) < 0) {
@@ -165,10 +155,6 @@ testPrepImages(void)
         fprintf(stderr, "unable to create directory %s\n", datadir "/dir");
         goto cleanup;
     }
-    if (!(canondir = virFileCanonicalizePath(absdir))) {
-        virReportOOMError();
-        goto cleanup;
-    }
 
     if (chdir(datadir) < 0) {
         fprintf(stderr, "unable to test relative backing chains\n");
@@ -180,10 +166,6 @@ testPrepImages(void)
         fprintf(stderr, "unable to create raw file\n");
         goto cleanup;
     }
-    if (!(canonraw = virFileCanonicalizePath(absraw))) {
-        virReportOOMError();
-        goto cleanup;
-    }
 
     /* Create a qcow2 wrapping relative raw; later on, we modify its
      * metadata to test other configurations */
@@ -200,10 +182,6 @@ testPrepImages(void)
                                "-F", "raw", "-b", "raw", "qcow2", NULL);
     if (virCommandRun(cmd, NULL) < 0)
         goto skip;
-    if (!(canonqcow2 = virFileCanonicalizePath(absqcow2))) {
-        virReportOOMError();
-        goto cleanup;
-    }
 
     /* Create a second qcow2 wrapping the first, to be sure that we
      * can correctly avoid insecure probing.  */
@@ -214,10 +192,6 @@ testPrepImages(void)
     virCommandAddArg(cmd, "wrap");
     if (virCommandRun(cmd, NULL) < 0)
         goto skip;
-    if (!(canonwrap = virFileCanonicalizePath(abswrap))) {
-        virReportOOMError();
-        goto cleanup;
-    }
 
     /* Create a qed file. */
     virCommandFree(cmd);
@@ -227,10 +201,6 @@ testPrepImages(void)
     virCommandAddArg(cmd, "qed");
     if (virCommandRun(cmd, NULL) < 0)
         goto skip;
-    if (!(canonqed = virFileCanonicalizePath(absqed))) {
-        virReportOOMError();
-        goto cleanup;
-    }
 
 #ifdef HAVE_SYMLINK
     /* Create some symlinks in a sub-directory. */
@@ -731,7 +701,7 @@ mymain(void)
 
     /* Raw image, whether with right format or no specified format */
     testFileData raw = {
-        .path = canonraw,
+        .path = absraw,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_RAW,
     };
@@ -743,12 +713,12 @@ mymain(void)
     testFileData qcow2 = {
         .expBackingStoreRaw = "raw",
         .expCapacity = 1024,
-        .path = canonqcow2,
+        .path = absqcow2,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_QCOW2,
     };
     testFileData qcow2_as_raw = {
-        .path = canonqcow2,
+        .path = absqcow2,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_RAW,
     };
@@ -772,7 +742,7 @@ mymain(void)
     testFileData wrap = {
         .expBackingStoreRaw = absqcow2,
         .expCapacity = 1024,
-        .path = canonwrap,
+        .path = abswrap,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_QCOW2,
     };
@@ -795,7 +765,7 @@ mymain(void)
     testFileData wrap_as_raw = {
         .expBackingStoreRaw = absqcow2,
         .expCapacity = 1024,
-        .path = canonwrap,
+        .path = abswrap,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_QCOW2,
     };
@@ -878,12 +848,12 @@ mymain(void)
     testFileData qed = {
         .expBackingStoreRaw = absraw,
         .expCapacity = 1024,
-        .path = canonqed,
+        .path = absqed,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_QED,
     };
     testFileData qed_as_raw = {
-        .path = canonqed,
+        .path = absqed,
         .type = VIR_STORAGE_TYPE_FILE,
         .format = VIR_STORAGE_FILE_RAW,
     };
@@ -892,12 +862,12 @@ mymain(void)
 
     /* directory */
     testFileData dir = {
-        .path = canondir,
+        .path = absdir,
         .type = VIR_STORAGE_TYPE_DIR,
         .format = VIR_STORAGE_FILE_DIR,
     };
     testFileData dir_as_raw = {
-        .path = canondir,
+        .path = absdir,
         .type = VIR_STORAGE_TYPE_DIR,
         .format = VIR_STORAGE_FILE_RAW,
     };
-- 
2.20.1

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

Re: [libvirt] [PATCH] tests: Don't use canonical paths in virstoragetest

Posted by Michal Privoznik 11 weeks ago
On 3/22/19 1:21 PM, Andrea Bolognani wrote:
> The layout of my home directory is somewhat peculiar: I store
> all git repositories in ~/src/upstream, but since I spend
> almost all of my time hacking on libvirt, I also have a
> convenience symlink ~/src/libvirt -> ~/src/upstream/libvirt
> that I use to access that specific git repository.
> 
> The above setup has served me well for years; however, ever
> since commit ca1471622dd9 dropped our own custom definitions
> for abs_{,top_}{src,build}dir and started using the ones
> provided by autotools, virstoragetest has started reliably
> failing with errors such as
> 
>     2) Storage backing chain 2 ...
>    Offset 0
>    Expect [chain member: 0
>    path:/home/abologna/src/upstream/libvirt/tests/virstoragedata/raw
>    backingStoreRaw: <null>
>    capacity: 0
>    encryption: 0
>    relPath:<null>
>    type:1
>    format:1
>    protocol:none
>    hostname:<null>
>    ]
>    Actual [chain member: 0
>    path:/home/abologna/src/libvirt/tests/virstoragedata/raw
>    backingStoreRaw: <null>
>    capacity: 0
>    encryption: 0
>    relPath:<null>
>    type:1
>    format:1
>    protocol:none
>    hostname:<null>
>    ]
>                                ... FAILED
> 
> Using abolute paths instead of canonical ones in the tests makes
> the problem go away.
> 
> Note that all tests that are specifically designed to test path
> canonicalization via TEST_PATH_CANONICALIZE() were passing even
> before this patch and are not touched by it.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> 
> I'm far from being confident this is the correct approach, but
> I've grown annoyed enough by the constant 'make check' failures
> that I figured I'd at least get the discussion going :)
> 
>   tests/virstoragetest.c | 50 +++++++++---------------------------------
>   1 file changed, 10 insertions(+), 40 deletions(-)

ACK and safe for freeze.

Michal

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

Re: [libvirt] [PATCH] tests: Don't use canonical paths in virstoragetest

Posted by Andrea Bolognani 11 weeks ago
On Fri, 2019-03-29 at 13:40 +0100, Michal Privoznik wrote:
> On 3/22/19 1:21 PM, Andrea Bolognani wrote:
> >   tests/virstoragetest.c | 50 +++++++++---------------------------------
> >   1 file changed, 10 insertions(+), 40 deletions(-)
> 
> ACK and safe for freeze.

Thanks for the review :)

I'll actually wait until 5.2.0 is out before pushing it, as most
users are not going to be affected by it anyway...

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] tests: Don't use canonical paths in virstoragetest

Posted by Andrea Bolognani 11 weeks ago
ping

On Fri, 2019-03-22 at 13:21 +0100, Andrea Bolognani wrote:
> The layout of my home directory is somewhat peculiar: I store
> all git repositories in ~/src/upstream, but since I spend
> almost all of my time hacking on libvirt, I also have a
> convenience symlink ~/src/libvirt -> ~/src/upstream/libvirt
> that I use to access that specific git repository.
> 
> The above setup has served me well for years; however, ever
> since commit ca1471622dd9 dropped our own custom definitions
> for abs_{,top_}{src,build}dir and started using the ones
> provided by autotools, virstoragetest has started reliably
> failing with errors such as
> 
>    2) Storage backing chain 2 ...
>   Offset 0
>   Expect [chain member: 0
>   path:/home/abologna/src/upstream/libvirt/tests/virstoragedata/raw
>   backingStoreRaw: <null>
>   capacity: 0
>   encryption: 0
>   relPath:<null>
>   type:1
>   format:1
>   protocol:none
>   hostname:<null>
>   ]
>   Actual [chain member: 0
>   path:/home/abologna/src/libvirt/tests/virstoragedata/raw
>   backingStoreRaw: <null>
>   capacity: 0
>   encryption: 0
>   relPath:<null>
>   type:1
>   format:1
>   protocol:none
>   hostname:<null>
>   ]
>                               ... FAILED
> 
> Using abolute paths instead of canonical ones in the tests makes
> the problem go away.
> 
> Note that all tests that are specifically designed to test path
> canonicalization via TEST_PATH_CANONICALIZE() were passing even
> before this patch and are not touched by it.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> 
> I'm far from being confident this is the correct approach, but
> I've grown annoyed enough by the constant 'make check' failures
> that I figured I'd at least get the discussion going :)
> 
>  tests/virstoragetest.c | 50 +++++++++---------------------------------
>  1 file changed, 10 insertions(+), 40 deletions(-)
-- 
Andrea Bolognani / Red Hat / Virtualization

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