[PATCH v2 0/4] btrfs-progs: tests: umount TEST_MNT in clean-tests.sh

[PATCH v2 0/4] btrfs-progs: tests: umount TEST_MNT in clean-tests.sh

[Zhao Lei]

If a testcase failed, we can't run it(or other tests needs mount) again,
  # ./misc-tests.sh 007
   [TEST]   007-subvolume-sync
   failed: fail
   test failed for case 007-subvolume-sync
  # ./misc-tests.sh 007
   [TEST]   007-subvolume-sync
   failed: mount /root/btrfs-progs/tests/test.img /root/btrfs-progs/tests/mnt
   test failed for case 007-subvolume-sync
  #

To solve this problem, I put umount command into test sctipt in v1,
but it will change stat of failed-test in next test, and make it
hard to debug, noticed by David Sterba <dsterba@suse.cz>:
 My expectation is that the testsuite will not fail and if it does,
 I wan't to keep the last stat so I can have a look.
 Would it be acceptable for you to enhance the clean-tests.sh
 script to unmount the TEST_DEV?

Putting umount command into clean-tests.sh need a global $TEST_MNT,
which already done in my previous patch.

This patchset rebase my previous patch of "setting global $TEST_MNT"
to git head in [PATCH 1..2/4], and add v2 of "umount TEST_MNT"
in [PATCH 3/4].

Thanks
Zhaolei

Zhao Lei (4):
  btrfs-progs: tests: Introduce init_env() to initialize common env
    variant
  btrfs-progs: tests: Fix mount fail of 013-extent-tree-rebuild
  btrfs-progs: tests: umount TEST_MNT in clean-tests.sh
  btrfs-progs: Use mount_test_dev for misc-tests/007-subvolume-sync

 tests/clean-tests.sh                             |  7 ++++--
 tests/common                                     | 30 ++++++++++++++++++++++++
 tests/convert-tests.sh                           |  1 -
 tests/fsck-tests.sh                              |  3 ---
 tests/fsck-tests/012-leaf-corruption/test.sh     |  1 -
 tests/fsck-tests/013-extent-tree-rebuild/test.sh |  9 ++-----
 tests/misc-tests.sh                              |  3 ---
 tests/misc-tests/001-btrfstune-features/test.sh  |  5 ----
 tests/misc-tests/002-uuid-rewrite/test.sh        |  5 ----
 tests/misc-tests/003-zero-log/test.sh            |  5 ----
 tests/misc-tests/007-subvolume-sync/test.sh      |  4 ++--
 11 files changed, 39 insertions(+), 34 deletions(-)

-- 
1.8.5.1

[PATCH v2 1/4] btrfs-progs: tests: Introduce init_env to initialize common env variant

[Zhao Lei]

For example, $TEST_DIR is common used in severial tests, and have
duplicated code for initialize.

These duplicated code not only benifits harddisk vendor, but have
inconsistent details, as:
  convert-tests.sh: lack of mkdir
  fsck-tests/012-leaf-corruption/test.sh: unnecessary mkdir
  fsck-tests/013-extent-tree-rebuild/test.sh: unnecessary init
  misc-tests/XXX ...
And severial error message:
  _fail "unable to create mount point on $TEST_MNT"
  _fail "failed to create mount point"
  ...

This patch move initizlizaton of $TEST_DIR to common init_env(),
to avoid above problem, and init_env() can be used to add more
things in future.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 tests/common                                     | 7 +++++++
 tests/convert-tests.sh                           | 1 -
 tests/fsck-tests.sh                              | 3 ---
 tests/fsck-tests/012-leaf-corruption/test.sh     | 1 -
 tests/fsck-tests/013-extent-tree-rebuild/test.sh | 5 -----
 tests/misc-tests.sh                              | 3 ---
 tests/misc-tests/001-btrfstune-features/test.sh  | 5 -----
 tests/misc-tests/002-uuid-rewrite/test.sh        | 5 -----
 tests/misc-tests/003-zero-log/test.sh            | 5 -----
 9 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/tests/common b/tests/common
index cf463e8..c597915 100644
--- a/tests/common
+++ b/tests/common
@@ -171,3 +171,10 @@ prepare_test_dev()
 	truncate -s "$size" "$TEST_DEV" || _not_run "create file for loop device failed"
 }
 
+init_env()
+{
+	TEST_MNT="${TEST_MNT:-$TOP/tests/mnt}"
+	export TEST_MNT
+	mkdir -p "$TEST_MNT" || { echo "Failed mkdir -p $TEST_MNT"; exit 1; }
+}
+init_env
diff --git a/tests/convert-tests.sh b/tests/convert-tests.sh
index b395e25..68c03dc 100755
--- a/tests/convert-tests.sh
+++ b/tests/convert-tests.sh
@@ -9,7 +9,6 @@ unset LANG
 LANG=C
 SCRIPT_DIR=$(dirname $(readlink -f $0))
 TOP=$(readlink -f $SCRIPT_DIR/../)
-TEST_MNT=${TEST_MNT:-$TOP/tests/mnt}
 RESULTS="$TOP/tests/convert-tests-results.txt"
 IMAGE="$TOP/tests/test.img"
 
diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
index 73538fe..b910e85 100755
--- a/tests/fsck-tests.sh
+++ b/tests/fsck-tests.sh
@@ -11,7 +11,6 @@ LANG=C
 SCRIPT_DIR=$(dirname $(readlink -f $0))
 TOP=$(readlink -f $SCRIPT_DIR/../)
 TEST_DEV=${TEST_DEV:-}
-TEST_MNT=${TEST_MNT:-$TOP/tests/mnt}
 RESULTS="$TOP/tests/fsck-tests-results.txt"
 
 source $TOP/tests/common
@@ -20,11 +19,9 @@ source $TOP/tests/common
 export TOP
 export RESULTS
 # For custom script needs to verfiy recovery
-export TEST_MNT
 export LANG
 
 rm -f $RESULTS
-mkdir -p $TEST_MNT || _fail "unable to create mount point on $TEST_MNT"
 
 # test rely on corrupting blocks tool
 check_prereq btrfs-corrupt-block
diff --git a/tests/fsck-tests/012-leaf-corruption/test.sh b/tests/fsck-tests/012-leaf-corruption/test.sh
index f8701ad..a37ceda 100755
--- a/tests/fsck-tests/012-leaf-corruption/test.sh
+++ b/tests/fsck-tests/012-leaf-corruption/test.sh
@@ -85,7 +85,6 @@ check_inode()
 check_leaf_corrupt_no_data_ext()
 {
 	image=$1
-	mkdir -p $TEST_MNT || _fail "failed to create mount point"
 	$SUDO_HELPER mount -o loop $image -o ro $TEST_MNT
 
 	i=0
diff --git a/tests/fsck-tests/013-extent-tree-rebuild/test.sh b/tests/fsck-tests/013-extent-tree-rebuild/test.sh
index 88a66cc..b7909d2 100755
--- a/tests/fsck-tests/013-extent-tree-rebuild/test.sh
+++ b/tests/fsck-tests/013-extent-tree-rebuild/test.sh
@@ -7,11 +7,6 @@ check_prereq mkfs.btrfs
 setup_root_helper
 prepare_test_dev 1G
 
-if [ -z $TEST_MNT ];then
-	echo "    [NOTRUN] extent tree rebuild, need TEST_MNT variant"
-	exit 0
-fi
-
 # test whether fsck can rebuild a corrupted extent tree
 test_extent_tree_rebuild()
 {
diff --git a/tests/misc-tests.sh b/tests/misc-tests.sh
index f9c1152..d04416b 100755
--- a/tests/misc-tests.sh
+++ b/tests/misc-tests.sh
@@ -8,7 +8,6 @@ LANG=C
 SCRIPT_DIR=$(dirname $(readlink -f $0))
 TOP=$(readlink -f $SCRIPT_DIR/../)
 TEST_DEV=${TEST_DEV:-}
-TEST_MNT=${TEST_MNT:-$TOP/tests/mnt}
 RESULTS="$TOP/tests/misc-tests-results.txt"
 IMAGE="$TOP/tests/test.img"
 
@@ -20,13 +19,11 @@ source $TOP/tests/common
 export TOP
 export RESULTS
 # For custom script needs to verfiy recovery
-export TEST_MNT
 export LANG
 # For tests that only use a loop device
 export IMAGE
 
 rm -f $RESULTS
-mkdir -p $TEST_MNT || _fail "unable to create mount point on $TEST_MNT"
 
 # test rely on corrupting blocks tool
 check_prereq btrfs-corrupt-block
diff --git a/tests/misc-tests/001-btrfstune-features/test.sh b/tests/misc-tests/001-btrfstune-features/test.sh
index ea33954..836e8d3 100755
--- a/tests/misc-tests/001-btrfstune-features/test.sh
+++ b/tests/misc-tests/001-btrfstune-features/test.sh
@@ -9,11 +9,6 @@ check_prereq mkfs.btrfs
 setup_root_helper
 prepare_test_dev
 
-if [ -z $TEST_MNT ];then
-	echo "    [NOTRUN] extent tree rebuild, need TEST_MNT variant"
-	exit 0
-fi
-
 # test whether fsck can rebuild a corrupted extent tree
 # parameters:
 # - option for mkfs.btrfs -O, empty for defaults
diff --git a/tests/misc-tests/002-uuid-rewrite/test.sh b/tests/misc-tests/002-uuid-rewrite/test.sh
index bffa9b8..9b103aa 100755
--- a/tests/misc-tests/002-uuid-rewrite/test.sh
+++ b/tests/misc-tests/002-uuid-rewrite/test.sh
@@ -9,11 +9,6 @@ check_prereq mkfs.btrfs
 check_prereq btrfstune
 prepare_test_dev
 
-if [ -z $TEST_MNT ];then
-	echo "    [NOTRUN] extent tree rebuild, need TEST_MNT variant"
-	exit 0
-fi
-
 get_fs_uuid() {
 	local image
 
diff --git a/tests/misc-tests/003-zero-log/test.sh b/tests/misc-tests/003-zero-log/test.sh
index edab5db..b650930 100755
--- a/tests/misc-tests/003-zero-log/test.sh
+++ b/tests/misc-tests/003-zero-log/test.sh
@@ -8,11 +8,6 @@ check_prereq mkfs.btrfs
 check_prereq btrfs
 prepare_test_dev
 
-if [ -z $TEST_MNT ];then
-	echo "    [NOTRUN] extent tree rebuild, need TEST_MNT variant"
-	exit 0
-fi
-
 get_log_root()
 {
 	local image
-- 
1.8.5.1

[PATCH v2 2/4] btrfs-progs: tests: Fix mount fail of 013-extent-tree-rebuild

[Zhao Lei]

When using loop device for test, fsck-tests/013-extent-tree-rebuild
failed with following error message:
  # ./fsck-tests.sh
  ...
    [TEST]   013-extent-tree-rebuild
  failed: mount /data/btrfsprogs/tests/test.img /data/btrfsprogs/tests/mnt
  test failed for case 013-extent-tree-rebuild
  #

Considering that $TEST_DEV can be block or loop device, we need determine
our mount option in a condition for both case.

This patch make above request to a common function, to solve current
problem in 013-extent-tree-rebuild, and support similar request in future.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 tests/common                                     | 23 +++++++++++++++++++++++
 tests/fsck-tests/013-extent-tree-rebuild/test.sh |  4 ++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/tests/common b/tests/common
index c597915..eae6b2b 100644
--- a/tests/common
+++ b/tests/common
@@ -178,3 +178,26 @@ init_env()
 	mkdir -p "$TEST_MNT" || { echo "Failed mkdir -p $TEST_MNT"; exit 1; }
 }
 init_env
+
+mount_test_dev()
+{
+	local loop_opt
+	if [[ -b "$TEST_DEV" ]]; then
+		loop_opt=()
+	elif [[ -f "$TEST_DEV" ]]; then
+		loop_opt=(-o loop)
+	else
+		_fail "Invalid \$TEST_DEV: $TEST_DEV"
+	fi
+
+	[[ -d "$TEST_MNT" ]] || {
+		_fail "Invalid \$TEST_MNT: $TEST_MNT"
+	}
+
+	mount "${loop_opt[@]}" "$TEST_DEV" "$TEST_MNT" || _fail "mount $TEST_DEV to $TEST_MNT failed"
+}
+
+umount_test_dev()
+{
+	umount "$TEST_DEV"
+}
diff --git a/tests/fsck-tests/013-extent-tree-rebuild/test.sh b/tests/fsck-tests/013-extent-tree-rebuild/test.sh
index b7909d2..ff3c922 100755
--- a/tests/fsck-tests/013-extent-tree-rebuild/test.sh
+++ b/tests/fsck-tests/013-extent-tree-rebuild/test.sh
@@ -12,14 +12,14 @@ test_extent_tree_rebuild()
 {
 	run_check $SUDO_HELPER $TOP/mkfs.btrfs -f $TEST_DEV
 
-	run_check $SUDO_HELPER mount $TEST_DEV $TEST_MNT
+	run_check $SUDO_HELPER mount_test_dev
 	run_check $SUDO_HELPER cp -aR /lib/modules/`uname -r`/ $TEST_MNT
 
 	for i in `seq 1 100`;do
 		run_check $SUDO_HELPER $TOP/btrfs sub snapshot $TEST_MNT \
 			$TEST_MNT/snapaaaaaaa_$i
 	done
-	run_check $SUDO_HELPER umount $TEST_DEV
+	run_check $SUDO_HELPER umount_test_dev
 
 	# get extent root bytenr
 	extent_root_bytenr=`$SUDO_HELPER $TOP/btrfs-debug-tree -r $TEST_DEV | \
-- 
1.8.5.1

[PATCH v2 3/4] btrfs-progs: tests: umount TEST_MNT in clean-tests.sh

[Zhao Lei]

If a testcase failed, we can't run it(or other tests needs mount) again,
  # ./misc-tests.sh 007
   [TEST]   007-subvolume-sync
   failed: fail
   test failed for case 007-subvolume-sync
  # ./misc-tests.sh 007
   [TEST]   007-subvolume-sync
   failed: mount /root/btrfs-progs/tests/test.img /root/btrfs-progs/tests/mnt
   test failed for case 007-subvolume-sync
  #

This patch add "umount $TEST_MNT" to clean-tests.sh, to let user
clean mountpoint easily.

After patch:
  # ./misc-tests.sh  007
   [TEST]   007-subvolume-sync
   failed: fail
   test failed for case 007-subvolume-sync
  #
  # clean-tests.sh
  #
  # ./misc-tests.sh  007
   [TEST]   007-subvolume-sync
   failed: fail
   test failed for case 007-subvolume-sync
  #

Changelog v1->v2:
  Put umount command to clean-tests.sh instead of in testscript,
  to keep failed-test stat until user run clean-tests.sh
  explicitly, Suggested-by: David Sterba <dsterba@suse.cz>

Suggested-by: David Sterba <dsterba@suse.cz>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 tests/clean-tests.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/clean-tests.sh b/tests/clean-tests.sh
index c1dc56a..19d682d 100755
--- a/tests/clean-tests.sh
+++ b/tests/clean-tests.sh
@@ -1,12 +1,15 @@
 #!/bin/sh
 # remove all intermediate files from tests
 
+SCRIPT_DIR=$(dirname $(readlink -f $0))
+TOP=$(readlink -f $SCRIPT_DIR/../)
+source $TOP/tests/common
+
 if [ "$BUILD_VERBOSE" = 1 ]; then
 	verbose=-print
 fi
 
-SCRIPT_DIR=$(dirname $(readlink -f $0))
-TOP=$(readlink -f $SCRIPT_DIR/../)
+umount "$TEST_MNT" &>/dev/null
 
 if ! cd $TOP/tests; then
 	echo "ERROR: cannot cd to $TOP/tests"
-- 
1.8.5.1

[PATCH v2 4/4] btrfs-progs: tests: Use mount_test_dev for misc-tests/007-subvolume-sync

[Zhao Lei]

So this test can support both block device and loop device simply.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 tests/misc-tests/007-subvolume-sync/test.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/misc-tests/007-subvolume-sync/test.sh b/tests/misc-tests/007-subvolume-sync/test.sh
index d4019f4..1eea098 100755
--- a/tests/misc-tests/007-subvolume-sync/test.sh
+++ b/tests/misc-tests/007-subvolume-sync/test.sh
@@ -13,7 +13,7 @@ setup_root_helper
 prepare_test_dev
 
 run_check $SUDO_HELPER $TOP/mkfs.btrfs -f "$TEST_DEV"
-run_check $SUDO_HELPER mount "$TEST_DEV" "$TEST_MNT"
+run_check $SUDO_HELPER mount_test_dev
 
 # to check following thing in both 1 and multiple subvolume case:
 # 1: is subvolume sync loop indefinetelly
@@ -29,4 +29,4 @@ run_check $SUDO_HELPER $TOP/btrfs subvolume create "$TEST_MNT"/mysubvol
 run_check $SUDO_HELPER $TOP/btrfs subvolume delete "$TEST_MNT"/mysubvol
 run_check $SUDO_HELPER $TOP/btrfs subvolume sync "$TEST_MNT"
 
-run_check $SUDO_HELPER umount $TEST_MNT
+run_check $SUDO_HELPER umount_test_dev
-- 
1.8.5.1

Re: [PATCH v2 1/4] btrfs-progs: tests: Introduce init_env to initialize common env variant

[David Sterba]

On Mon, Aug 31, 2015 at 01:04:36PM +0800, Zhao Lei wrote:
> For example, $TEST_DIR is common used in severial tests, and have
> duplicated code for initialize.
> 
> These duplicated code not only benifits harddisk vendor, but have
> inconsistent details, as:
>   convert-tests.sh: lack of mkdir
>   fsck-tests/012-leaf-corruption/test.sh: unnecessary mkdir
>   fsck-tests/013-extent-tree-rebuild/test.sh: unnecessary init
>   misc-tests/XXX ...
> And severial error message:
>   _fail "unable to create mount point on $TEST_MNT"
>   _fail "failed to create mount point"
>   ...
> 
> This patch move initizlizaton of $TEST_DIR to common init_env(),
> to avoid above problem, and init_env() can be used to add more
> things in future.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH v2 2/4] btrfs-progs: tests: Fix mount fail of 013-extent-tree-rebuild

[David Sterba]

On Mon, Aug 31, 2015 at 01:04:37PM +0800, Zhao Lei wrote:
> +mount_test_dev()
> +{
> +	local loop_opt
> +	if [[ -b "$TEST_DEV" ]]; then
> +		loop_opt=()
> +	elif [[ -f "$TEST_DEV" ]]; then
> +		loop_opt=(-o loop)
> +	else
> +		_fail "Invalid \$TEST_DEV: $TEST_DEV"
> +	fi
> +
> +	[[ -d "$TEST_MNT" ]] || {
> +		_fail "Invalid \$TEST_MNT: $TEST_MNT"
> +	}
> +
> +	mount "${loop_opt[@]}" "$TEST_DEV" "$TEST_MNT" || _fail "mount $TEST_DEV to $TEST_MNT failed"

This needs the root helper, besides I don't see a reason why to use the
array to pass the loop options.

On newer systems the option 'loop' is not necessary and mount works with
that but I don't mind adding support for systems where this does not
work. However, this should not break current behaviour.

One thing that I find important is that the full commands are logged via
the run_check helpers. So I suggest to add run_check_mount_test_dev
helper that would wrapp all the commands. The combination of shell
functions does not work in all cases.

Re: [PATCH v2 3/4] btrfs-progs: tests: umount TEST_MNT in clean-tests.sh

[David Sterba]

On Mon, Aug 31, 2015 at 01:04:38PM +0800, Zhao Lei wrote:
> If a testcase failed, we can't run it(or other tests needs mount) again,
>   # ./misc-tests.sh 007
>    [TEST]   007-subvolume-sync
>    failed: fail
>    test failed for case 007-subvolume-sync
>   # ./misc-tests.sh 007
>    [TEST]   007-subvolume-sync
>    failed: mount /root/btrfs-progs/tests/test.img /root/btrfs-progs/tests/mnt
>    test failed for case 007-subvolume-sync
>   #
> 
> This patch add "umount $TEST_MNT" to clean-tests.sh, to let user
> clean mountpoint easily.
> 
> After patch:
>   # ./misc-tests.sh  007
>    [TEST]   007-subvolume-sync
>    failed: fail
>    test failed for case 007-subvolume-sync
>   #
>   # clean-tests.sh
>   #
>   # ./misc-tests.sh  007
>    [TEST]   007-subvolume-sync
>    failed: fail
>    test failed for case 007-subvolume-sync
>   #
> 
> Changelog v1->v2:
>   Put umount command to clean-tests.sh instead of in testscript,
>   to keep failed-test stat until user run clean-tests.sh
>   explicitly, Suggested-by: David Sterba <dsterba@suse.cz>
> 
> Suggested-by: David Sterba <dsterba@suse.cz>
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied, thanks.

RE: [PATCH v2 2/4] btrfs-progs: tests: Fix mount fail of 013-extent-tree-rebuild

[Zhao Lei]

Hi, David Sterba

Thanks for review.

> -----Original Message-----
> From: David Sterba [mailto:dsterba@suse.cz]
> Sent: Monday, August 31, 2015 10:48 PM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH v2 2/4] btrfs-progs: tests: Fix mount fail of
> 013-extent-tree-rebuild
> 
> On Mon, Aug 31, 2015 at 01:04:37PM +0800, Zhao Lei wrote:
> > +mount_test_dev()
> > +{
> > +	local loop_opt
> > +	if [[ -b "$TEST_DEV" ]]; then
> > +		loop_opt=()
> > +	elif [[ -f "$TEST_DEV" ]]; then
> > +		loop_opt=(-o loop)
> > +	else
> > +		_fail "Invalid \$TEST_DEV: $TEST_DEV"
> > +	fi
> > +
> > +	[[ -d "$TEST_MNT" ]] || {
> > +		_fail "Invalid \$TEST_MNT: $TEST_MNT"
> > +	}
> > +
> > +	mount "${loop_opt[@]}" "$TEST_DEV" "$TEST_MNT" || _fail "mount
> $TEST_DEV to $TEST_MNT failed"
> 
> This needs the root helper,
>
> besides I don't see a reason why to use the array to
> pass the loop options.
> 
Only personal habit, will not use it in v2.

> On newer systems the option 'loop' is not necessary and mount works with that
> 
Glad to hear it.

> but I don't mind adding support for systems where this does not work. However,
> this should not break current behaviour.
> 
> One thing that I find important is that the full commands are logged via the
> run_check helpers. So I suggest to add run_check_mount_test_dev helper that
> would wrapp all the commands. The combination of shell functions does not
> work in all cases.
>
Ok.

Thanks
Zhaolei