[PATCH v3 0/4] btrfs-progs: fix issues in tests

[PATCH v3 0/4] btrfs-progs: fix issues in tests

[Anand Jain]

Fixes issues found in tests.

path 3/4 canonicalized the TEST_DEV path and affected few test group.
Unit tests passed however a complete tests runs is not done.

Anand Jain (4):
  btrfs-progs: fix fsck-test/037 skip corrupt FREE_SPACE_BITMAP
  btrfs-progs: fix TEST_TOP path drop extra forward slash
  btrfs-progs: test clean start after failures
  btrfs-progs: fix misc-test/029 provide device for mount

 tests/clean-tests.sh                                      |  2 +-
 tests/cli-tests.sh                                        |  2 +-
 tests/common                                              |  6 ++++++
 tests/convert-tests.sh                                    |  2 +-
 tests/fsck-tests.sh                                       |  2 +-
 tests/fsck-tests/037-freespacetree-repair/test.sh         | 14 ++++++++++----
 tests/fuzz-tests.sh                                       |  2 +-
 tests/misc-tests.sh                                       |  3 ++-
 tests/misc-tests/029-send-p-different-mountpoints/test.sh |  3 ++-
 tests/mkfs-tests.sh                                       |  2 +-
 10 files changed, 26 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] btrfs-progs: fix fsck-test/037 skip corrupt FREE_SPACE_BITMAP

[Anand Jain]

We don't have the FREE_SPACE_BITMAP in the default filesystem,
skip the sub test by checking if the objectid is null for the
FREE_SPACE_BITMAP. The null objectid check for the FREE_SPACE_EXTENT
is not actually required, it is added to maintain similar flow in
the code above it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/fsck-tests/037-freespacetree-repair/test.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/fsck-tests/037-freespacetree-repair/test.sh b/tests/fsck-tests/037-freespacetree-repair/test.sh
index 49165fcd7390..607a4e9dff9a 100755
--- a/tests/fsck-tests/037-freespacetree-repair/test.sh
+++ b/tests/fsck-tests/037-freespacetree-repair/test.sh
@@ -29,6 +29,9 @@ corrupt_fst_item()
 		objectid=$("$TOP/btrfs" inspect-internal dump-tree -t 10 "$TEST_DEV" | \
 			grep -o "[[:digit:]]* FREE_SPACE_BITMAP [[:digit:]]*" | \
 			cut -d' ' -f1 | tail -2 | head -1)
+		if [[ $objectid == "" ]]; then
+			return 1
+		fi
 		offset=$("$TOP/btrfs" inspect-internal dump-tree -t 10 "$TEST_DEV" | \
 			grep -o "[[:digit:]]* FREE_SPACE_BITMAP [[:digit:]]*" | \
 			cut -d' ' -f3 | tail -2 | head -1)
@@ -38,6 +41,9 @@ corrupt_fst_item()
 		objectid=$("$TOP/btrfs" inspect-internal dump-tree -t 10 "$TEST_DEV" | \
 			grep -o "[[:digit:]]* FREE_SPACE_EXTENT [[:digit:]]*" | \
 			cut -d' ' -f1 | tail -2 | head -1)
+		if [[ $objectid == "" ]]; then
+			return 1
+		fi
 		offset=$("$TOP/btrfs" inspect-internal dump-tree -t 10 "$TEST_DEV" | \
 			grep -o "[[:digit:]]* FREE_SPACE_EXTENT [[:digit:]]*" | \
 			cut -d' ' -f3 | tail -2 | head -1)
@@ -48,6 +54,8 @@ corrupt_fst_item()
 
 	run_check "$INTERNAL_BIN/btrfs-corrupt-block" -r 10 -K "$objectid,$type,$offset" \
 		-f offset "$TEST_DEV"
+
+	return 0
 }
 
 if ! [ -f "/sys/fs/btrfs/features/free_space_tree" ]; then
@@ -69,8 +77,7 @@ done
 run_check_umount_test_dev
 
 # now corrupt one of the bitmap items
-corrupt_fst_item "bitmap"
-check_image "$TEST_DEV"
+corrupt_fst_item "bitmap" && check_image "$TEST_DEV"
 
 # change the freespace such that we now have at least one free_space_extent
 # object
@@ -80,5 +87,4 @@ run_check $SUDO_HELPER fallocate -l 50m "$TEST_MNT/file"
 run_check_umount_test_dev
 
 # now corrupt an extent
-corrupt_fst_item "extent"
-check_image "$TEST_DEV"
+corrupt_fst_item "extent" && check_image "$TEST_DEV"
-- 
1.8.3.1

[PATCH 2/4] btrfs-progs: fix TEST_TOP path drop extra forward slash

[Anand Jain]

From
TEST_TOP="$TOP/tests/"
to
TEST_TOP="$TOP/tests"

Affected test cases are:
tests/clean-tests.sh
tests/cli-tests.sh
tests/convert-tests.sh
tests/fsck-tests.sh
tests/fuzz-tests.sh
tests/misc-tests.sh
tests/mkfs-tests.sh

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/clean-tests.sh   | 2 +-
 tests/cli-tests.sh     | 2 +-
 tests/convert-tests.sh | 2 +-
 tests/fsck-tests.sh    | 2 +-
 tests/fuzz-tests.sh    | 2 +-
 tests/misc-tests.sh    | 2 +-
 tests/mkfs-tests.sh    | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/clean-tests.sh b/tests/clean-tests.sh
index eda16e329698..d8157ab54ee3 100755
--- a/tests/clean-tests.sh
+++ b/tests/clean-tests.sh
@@ -7,7 +7,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/cli-tests.sh b/tests/cli-tests.sh
index d302a93ea594..d8cd06724701 100755
--- a/tests/cli-tests.sh
+++ b/tests/cli-tests.sh
@@ -8,7 +8,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/convert-tests.sh b/tests/convert-tests.sh
index f6c940437eec..24b3ec0df144 100755
--- a/tests/convert-tests.sh
+++ b/tests/convert-tests.sh
@@ -9,7 +9,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
index 8fbc2b4bc40e..15e3d8d5995c 100755
--- a/tests/fsck-tests.sh
+++ b/tests/fsck-tests.sh
@@ -8,7 +8,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/fuzz-tests.sh b/tests/fuzz-tests.sh
index ae738710b8bc..0ba8f1e11f70 100755
--- a/tests/fuzz-tests.sh
+++ b/tests/fuzz-tests.sh
@@ -8,7 +8,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/misc-tests.sh b/tests/misc-tests.sh
index 3e7b9e9befe6..3b49ab012e78 100755
--- a/tests/misc-tests.sh
+++ b/tests/misc-tests.sh
@@ -8,7 +8,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
diff --git a/tests/mkfs-tests.sh b/tests/mkfs-tests.sh
index e76a805b4f0b..150f094f2303 100755
--- a/tests/mkfs-tests.sh
+++ b/tests/mkfs-tests.sh
@@ -8,7 +8,7 @@ if [ -z "$TOP" ]; then
 	TOP=$(readlink -f "$SCRIPT_DIR/../")
 	if [ -f "$TOP/configure.ac" ]; then
 		# inside git
-		TEST_TOP="$TOP/tests/"
+		TEST_TOP="$TOP/tests"
 		INTERNAL_BIN="$TOP"
 	else
 		# external, defaults to system binaries
-- 
1.8.3.1

[PATCH 3/4] btrfs-progs: test clean start after failures

[Anand Jain]

After a failure it fails for a different reason

=== START TEST
/btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints
$TEST_DEV not given, using /btrfs-progs/tests/test.img as fallback
====== RUN MAYFAIL mkdir -p
/btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
====== RUN CHECK /btrfs-progs/mkfs.btrfs -f /btrfs-progs/tests/test.img
ERROR: /btrfs-progs/tests/test.img is mounted <====

Add clean_start() helper to unmount.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/common        | 6 ++++++
 tests/misc-tests.sh | 1 +
 2 files changed, 7 insertions(+)

diff --git a/tests/common b/tests/common
index 71661e950db0..bf55500e94ec 100644
--- a/tests/common
+++ b/tests/common
@@ -788,6 +788,12 @@ cleanup_loopdevs()
 	run_check $SUDO_HELPER losetup --all
 }
 
+# add all the cleanups here
+clean_start()
+{
+	grep -q $TEST_MNT /proc/self/mounts && umount $TEST_MNT
+}
+
 init_env()
 {
 	TEST_MNT="${TEST_MNT:-$TEST_TOP/mnt}"
diff --git a/tests/misc-tests.sh b/tests/misc-tests.sh
index 3b49ab012e78..3dc96f258540 100755
--- a/tests/misc-tests.sh
+++ b/tests/misc-tests.sh
@@ -40,6 +40,7 @@ export IMAGE
 export TEST_DEV
 
 rm -f "$RESULTS"
+clean_start
 
 # test rely on corrupting blocks tool
 check_prereq btrfs-corrupt-block
-- 
1.8.3.1

[PATCH v3 4/4] btrfs-progs: fix misc-test/029 provide device for mount

[Anand Jain]

The mount fails with 'file exists' error. Fix it by providing the device
name.

$ make TEST=029\* test-misc
    [TEST]   misc-tests.sh
    [TEST/misc]   029-send-p-different-mountpoints
failed: mount -t btrfs -o subvol=subv1 /btrfs-progs/tests//test.img /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
test failed for case 029-send-p-different-mountpoints
make: *** [test-misc] Error 1

====== RUN CHECK mount -t btrfs -o subvol=subv1
/btrfs-progs/tests//test.img
/btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
mount: mount /dev/loop1 on
/btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
failed: File exists
failed: mount -t btrfs -o subvol=subv1 /btrfs-progs/tests//test.img
/btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
test failed for case 029-send-p-different-mountpoints

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: use readlink to sanitize the TEST_DEV path
v3: readlink for TEST_DEV is not needed drop it

 tests/misc-tests/029-send-p-different-mountpoints/test.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/misc-tests/029-send-p-different-mountpoints/test.sh b/tests/misc-tests/029-send-p-different-mountpoints/test.sh
index a478b3d26495..b05e4172ca0f 100755
--- a/tests/misc-tests/029-send-p-different-mountpoints/test.sh
+++ b/tests/misc-tests/029-send-p-different-mountpoints/test.sh
@@ -20,7 +20,8 @@ run_check_mkfs_test_dev
 run_check_mount_test_dev
 
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
-run_check $SUDO_HELPER mount -t btrfs -o subvol=subv1 "$TEST_DEV" "$SUBVOL_MNT"
+lodev=$(losetup  | grep ${TEST_DEV} | awk '{print $1}')
+run_check $SUDO_HELPER mount -t btrfs -o subvol=subv1 "$lodev" "$SUBVOL_MNT"
 
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/test-subvol"
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r \
-- 
1.8.3.1

Re: [PATCH v3 4/4] btrfs-progs: fix misc-test/029 provide device for mount

[David Sterba]

On Fri, Apr 03, 2020 at 10:10:43AM +0800, Anand Jain wrote:
> The mount fails with 'file exists' error. Fix it by providing the device
> name.
> 
> $ make TEST=029\* test-misc
>     [TEST]   misc-tests.sh
>     [TEST/misc]   029-send-p-different-mountpoints
> failed: mount -t btrfs -o subvol=subv1 /btrfs-progs/tests//test.img /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
> test failed for case 029-send-p-different-mountpoints
> make: *** [test-misc] Error 1
> 
> ====== RUN CHECK mount -t btrfs -o subvol=subv1
> /btrfs-progs/tests//test.img
> /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
> mount: mount /dev/loop1 on
> /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
> failed: File exists
> failed: mount -t btrfs -o subvol=subv1 /btrfs-progs/tests//test.img
> /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
> test failed for case 029-send-p-different-mountpoints
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: use readlink to sanitize the TEST_DEV path
> v3: readlink for TEST_DEV is not needed drop it
> 
>  tests/misc-tests/029-send-p-different-mountpoints/test.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/misc-tests/029-send-p-different-mountpoints/test.sh b/tests/misc-tests/029-send-p-different-mountpoints/test.sh
> index a478b3d26495..b05e4172ca0f 100755
> --- a/tests/misc-tests/029-send-p-different-mountpoints/test.sh
> +++ b/tests/misc-tests/029-send-p-different-mountpoints/test.sh
> @@ -20,7 +20,8 @@ run_check_mkfs_test_dev
>  run_check_mount_test_dev
>  
>  run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> -run_check $SUDO_HELPER mount -t btrfs -o subvol=subv1 "$TEST_DEV" "$SUBVOL_MNT"
> +lodev=$(losetup  | grep ${TEST_DEV} | awk '{print $1}')
> +run_check $SUDO_HELPER mount -t btrfs -o subvol=subv1 "$lodev" "$SUBVOL_MNT"

The test dev is not necessarily a loop device, yes by default it is, but
TEST_DEV can be set externally.

I still wonder why the test fails for you because it does not for me and
without more information it's not clear what is mount reporting as
'File exists', which is EEXIST.

Re: [PATCH 3/4] btrfs-progs: test clean start after failures

[David Sterba]

On Fri, Apr 03, 2020 at 10:10:42AM +0800, Anand Jain wrote:
> After a failure it fails for a different reason
> 
> === START TEST
> /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints
> $TEST_DEV not given, using /btrfs-progs/tests/test.img as fallback
> ====== RUN MAYFAIL mkdir -p
> /btrfs-progs/tests/misc-tests/029-send-p-different-mountpoints/subvol_mnt
> ====== RUN CHECK /btrfs-progs/mkfs.btrfs -f /btrfs-progs/tests/test.img
> ERROR: /btrfs-progs/tests/test.img is mounted <====
> 
> Add clean_start() helper to unmount.

The testsuite is designed to stop on first failure and it's up the user
to investigate and clean it up before starting it again. For that
there's the test-clean make target. In some cases it might not clean up
the environment completely, but that's typically if the test image is
not unmoutnable due to some process having the directory open.

> +# add all the cleanups here
> +clean_start()
> +{
> +	grep -q $TEST_MNT /proc/self/mounts && umount $TEST_MNT
> +}
> +
>  init_env()
>  {
>  	TEST_MNT="${TEST_MNT:-$TEST_TOP/mnt}"
> diff --git a/tests/misc-tests.sh b/tests/misc-tests.sh
> index 3b49ab012e78..3dc96f258540 100755
> --- a/tests/misc-tests.sh
> +++ b/tests/misc-tests.sh
> @@ -40,6 +40,7 @@ export IMAGE
>  export TEST_DEV
>  
>  rm -f "$RESULTS"
> +clean_start

Instead of this I'd rather make it a hard stop if there's some problem
with the environment, like the mountpoint still mounted, or more loop
devices from the specific tests still set up as loop devices etc.

When I get to this situation I fix it manually (checkint mount output,
losetup -D), but it'd be nice to have that automated.

Re: [PATCH 2/4] btrfs-progs: fix TEST_TOP path drop extra forward slash

[David Sterba]

On Fri, Apr 03, 2020 at 10:10:41AM +0800, Anand Jain wrote:
> From
> TEST_TOP="$TOP/tests/"
> to
> TEST_TOP="$TOP/tests"

So this basically repeats the diff

> Affected test cases are:
> tests/clean-tests.sh
> tests/cli-tests.sh
> tests/convert-tests.sh
> tests/fsck-tests.sh
> tests/fuzz-tests.sh
> tests/misc-tests.sh
> tests/mkfs-tests.sh

And this repeats the diffstat.

The changelog should explain why the change is made, summarize some key
points, etc.

I'll apply this patch and write a changelog but please try to improve
that. What works is to postpone sending the patches and read them after
a day or two, if it still makes sense then send it. Second look reveals
all the typos and missing information.