[libvirt] [PATCH] tests: Avoid writing into $HOME during virsh-snapshot

Eric Blake posted 1 patch 5 years ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190327184737.21783-1-eblake@redhat.com
There is a newer version of this series
tests/test-lib.sh         | 13 +++++++++++++
tests/virsh-snapshot      |  2 ++
tests/virsh-uriprecedence | 12 +-----------
3 files changed, 16 insertions(+), 11 deletions(-)
[libvirt] [PATCH] tests: Avoid writing into $HOME during virsh-snapshot
Posted by Eric Blake 5 years ago
In a constrained CI environment, where it is intentional that attempts
to write outside the current directory will fail, virsh-snapshot was
failing:

@@ -1,2 +1,3 @@
 error: invalid argument: parent s3 for snapshot s2 not found
 error: marker
+error: Failed to create '/home/travis/.cache/libvirt/virsh': Permission denied
FAIL virsh-snapshot (exit status: 1)

But we've already solved the problem in virsh-uriprecedence: tell
virsh to use XDG locations pointing to somewhere we can write rather
than its default of falling back to $HOME with the test being at risk
of breaking due to the user's environment and/or unacceptably altering
the user's normal cache.  Hoist that solution into test-lib.sh, so
that all scripts can use it as needed.

Fixes: 280a2b41e
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-lib.sh         | 13 +++++++++++++
 tests/virsh-snapshot      |  2 ++
 tests/virsh-uriprecedence | 12 +-----------
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index 49e8d22095..64f0b0d401 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -222,6 +222,19 @@ mkfifo_or_skip_()
   fi
 }

+# Create mock XDG files/directories to avoid permission problems.
+# As it points inside $test_dir_, it is automatically cleaned.
+mock_xdg_()
+{
+  export XDG_CONFIG_HOME="$test_dir_/.config"
+  export XDG_CACHE_HOME="$test_dir_/.cache"
+  export XDG_RUNTIME_HOME="XDG_CACHE_HOME"
+
+  mkdir -p "$XDG_CONFIG_HOME/libvirt" "$XDG_CONFIG_HOME/virsh"
+  mkdir -p "$XDG_CACHE_HOME/libvirt" "$XDG_CACHE_HOME/virsh"
+  mkdir -p "$XDG_RUNTIME_HOME/libvirt" "$XDG_RUNTIME_HOME/virsh"
+}
+
 test_dir_=$(pwd)

 this_test_() { echo "./$0" | sed 's,.*/,,'; }
diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
index fb8a99dd43..cb498cf54e 100755
--- a/tests/virsh-snapshot
+++ b/tests/virsh-snapshot
@@ -26,6 +26,8 @@ fi

 fail=0

+mock_xdg_ || framework_failure
+
 # The test driver loses states between restarts, so we perform a script
 # with some convenient markers for later post-processing of output.
 $abs_top_builddir/tools/virsh --connect test:///default >out 2>err '
diff --git a/tests/virsh-uriprecedence b/tests/virsh-uriprecedence
index 564e3dc42c..fd6ce108c0 100755
--- a/tests/virsh-uriprecedence
+++ b/tests/virsh-uriprecedence
@@ -11,17 +11,7 @@ virsh_cmd="$virsh_bin"
 counter=0
 ret=0

-cleanup_() { rm -rf "$tmphome"; }
-
-# Create all mock files/directories to avoid permission problems
-tmphome="$PWD/tmp_home"
-export XDG_CONFIG_HOME="$tmphome/.config"
-export XDG_CACHE_HOME="$tmphome/.cache"
-export XDG_RUNTIME_HOME="XDG_CACHE_HOME"
-
-mkdir -p "$XDG_CONFIG_HOME/libvirt" "$XDG_CONFIG_HOME/virsh"
-mkdir -p "$XDG_CACHE_HOME/libvirt" "$XDG_CACHE_HOME/virsh"
-mkdir -p "$XDG_RUNTIME_HOME/libvirt" "$XDG_RUNTIME_HOME/virsh"
+mock_xdg_ || framework_failure

 is_uri_good()
 {
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Avoid writing into $HOME during virsh-snapshot
Posted by Andrea Bolognani 5 years ago
On Wed, 2019-03-27 at 13:47 -0500, Eric Blake wrote:
[...]
> +# Create mock XDG files/directories to avoid permission problems.
> +# As it points inside $test_dir_, it is automatically cleaned.
> +mock_xdg_()

There doesn't seem to be a consistent pattern deciding when functions
should be named_like_this() instead of being named_like_that_(), so I
guess either one works fine.

> +{
> +  export XDG_CONFIG_HOME="$test_dir_/.config"
> +  export XDG_CACHE_HOME="$test_dir_/.cache"
> +  export XDG_RUNTIME_HOME="XDG_CACHE_HOME"

Aren't you missing the $ in front of XDG_CACHE_HOME here?  The same
was actually true of the original incarnation of the code, too.

With $XDG_RUNTIME_HOME set correctly,

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

-- 
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: Avoid writing into $HOME during virsh-snapshot
Posted by Andrea Bolognani 5 years ago
On Thu, 2019-03-28 at 10:13 +0100, Andrea Bolognani wrote:
> On Wed, 2019-03-27 at 13:47 -0500, Eric Blake wrote:
> > +  export XDG_CONFIG_HOME="$test_dir_/.config"
> > +  export XDG_CACHE_HOME="$test_dir_/.cache"
> > +  export XDG_RUNTIME_HOME="XDG_CACHE_HOME"
> 
> Aren't you missing the $ in front of XDG_CACHE_HOME here?  The same
> was actually true of the original incarnation of the code, too.

Wait, I actually spotted two more issues with this patch.

First of all, you'll want to add

  /tests/.cache/
  /tests/.config/

to .gitignore; moreover, with this patch applied distcheck will
fail with

  ERROR: files left in build directory after distclean:
  ./tests/.cache/libvirt/virsh/history
  ./tests/.config/libvirt/libvirt.conf
  make[1]: *** [Makefile:2414: distcleancheck] Error 1

so that will have to be addressed as well.

-- 
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: Avoid writing into $HOME during virsh-snapshot
Posted by Eric Blake 5 years ago
On 3/28/19 6:53 AM, Andrea Bolognani wrote:
> On Thu, 2019-03-28 at 10:13 +0100, Andrea Bolognani wrote:
>> On Wed, 2019-03-27 at 13:47 -0500, Eric Blake wrote:
>>> +  export XDG_CONFIG_HOME="$test_dir_/.config"
>>> +  export XDG_CACHE_HOME="$test_dir_/.cache"
>>> +  export XDG_RUNTIME_HOME="XDG_CACHE_HOME"
>>
>> Aren't you missing the $ in front of XDG_CACHE_HOME here?  The same
>> was actually true of the original incarnation of the code, too.
> 
> Wait, I actually spotted two more issues with this patch.
> 
> First of all, you'll want to add
> 
>   /tests/.cache/
>   /tests/.config/
> 
> to .gitignore;

Or update to use something other than $test_dir_ that actually gets
cleaned up on a per-test basis, so that .gitignore won't see it in the
first place.

> moreover, with this patch applied distcheck will
> fail with
> 
>   ERROR: files left in build directory after distclean:
>   ./tests/.cache/libvirt/virsh/history
>   ./tests/.config/libvirt/libvirt.conf
>   make[1]: *** [Makefile:2414: distcleancheck] Error 1
> 
> so that will have to be addressed as well.

Good catch; I'll be sure to test distcheck on my v2.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Avoid writing into $HOME during virsh-snapshot
Posted by Daniel P. Berrangé 5 years ago
On Wed, Mar 27, 2019 at 01:47:37PM -0500, Eric Blake wrote:
> In a constrained CI environment, where it is intentional that attempts
> to write outside the current directory will fail, virsh-snapshot was
> failing:
> 
> @@ -1,2 +1,3 @@
>  error: invalid argument: parent s3 for snapshot s2 not found
>  error: marker
> +error: Failed to create '/home/travis/.cache/libvirt/virsh': Permission denied
> FAIL virsh-snapshot (exit status: 1)
> 
> But we've already solved the problem in virsh-uriprecedence: tell
> virsh to use XDG locations pointing to somewhere we can write rather
> than its default of falling back to $HOME with the test being at risk
> of breaking due to the user's environment and/or unacceptably altering
> the user's normal cache.  Hoist that solution into test-lib.sh, so
> that all scripts can use it as needed.

Ahhh, I was wondering why other tests didn't fail for the same reason.
In fact I wonder if more virsh tests are triggering this, but simply
ignoring the error.

> 
> Fixes: 280a2b41e
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-lib.sh         | 13 +++++++++++++
>  tests/virsh-snapshot      |  2 ++
>  tests/virsh-uriprecedence | 12 +-----------
>  3 files changed, 16 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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