[libvirt] [PATCH] tests: Avoid writing into $HOME during virsh-snapshot
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
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
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
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
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 :|
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Eric Blake