[PATCH 0/3] btrfs-progs: selftests fixes

[PATCH 0/3] btrfs-progs: selftests fixes

[Qu Wenruo]

There are 3 bugs exposed during my tests for unified mkfs features, and
they are all from the selftest itself:

- check_min_kernel_version() doesn't handle 6.x kernels at all
  The original check never handles major number check properly.
  And when we change major number, returns in correct number now.

- Missing a space between "!" and function call
  This bug is there for a long time. 

  Without previous fix, one may incorrectly remove the "!" and cause
  new problems.

- Convert/022 doesn't check if we have support for reiserfs

Qu Wenruo (3):
  btrfs-progs: tests: fix the wrong kernel version check
  btrfs-progs: mkfs-tests/025: fix the wrong function call
  btrfs-progs: convert-tests/022: add reiserfs support check

 tests/common                                  | 23 +++++++++++++++----
 .../022-reiserfs-parent-ref/test.sh           |  5 ++++
 tests/mkfs-tests/025-zoned-parallel/test.sh   |  2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.37.3

[PATCH 1/3] btrfs-progs: tests: fix the wrong kernel version check

[Qu Wenruo]

[BUG]
After upgrading to kernel v6.0-rc, btrfs-progs selftest mkfs/001 no
longer checks single device RAID0 and other new features introduced in
v5.13:

  # make TEST=001\* test-mkfs
    [TEST]   mkfs-tests.sh
    [TEST/mkfs]   001-basic-profiles
  $ grep -IR "RAID0\/1" tests/mkfs-tests-results.txt
  ^^^ No output

[CAUSE]
The existing check_min_kernel_version() is doing an incorrect check.

The old check looks like this:

	[ "$unamemajor" -lt "$argmajor" ] || return 1
	[ "$unameminor" -lt "$argminor" ] || return 1
	return 0

For 6.0-rc kernels, we have the following values for mkfs/001

 $unamemajor = 6
 $unameminor = 0
 $argmajor   = 5
 $argminor   = 12

The first check doesn't exit immediately, as 6 > 5.
Then we check the minor, which is already incorrect.

If our major is larger than target major, we should exit immediate with
0.

[FIX]
Fix the check and add extra comment.

Personally speaking I'm not a fan or short compare and return, thus all
the checks will explicit "if []; then fi" checks.

Now mkfs/001 works as expected:

  # make TEST=001\* test-mkfs
    [TEST]   mkfs-tests.sh
    [TEST/mkfs]   001-basic-profiles
  $ grep -IR "RAID0\/1" tests/mkfs-tests-results.txt
   Data,RAID0/1:          204.75MiB
   Metadata,RAID0/1:      204.75MiB
   System,RAID0/1:          8.00MiB

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/tests/common b/tests/common
index 3ee42a80dcda..d985ef72a4f1 100644
--- a/tests/common
+++ b/tests/common
@@ -659,6 +659,9 @@ check_kernel_support()
 # compare running kernel version to the given parameter, return success
 # if running is newer than requested (let caller decide if to fail or skip)
 # $1: minimum version of running kernel in major.minor format (eg. 4.19)
+#
+# Return 0 if we meet the minimal version requirement.
+# Return 1 if not.
 check_min_kernel_version()
 {
 	local unamemajor
@@ -672,10 +675,22 @@ check_min_kernel_version()
 	uname=${uname%%-*}
 	IFS=. read unamemajor unameminor tmp <<< "$uname"
 	IFS=. read argmajor argminor tmp <<< "$1"
-	# "compare versions: ${unamemajor}.${unameminor} ? ${argmajor}.${argminor}"
-	[ "$unamemajor" -lt "$argmajor" ] || return 1
-	[ "$unameminor" -lt "$argminor" ] || return 1
-	return 0
+	# If our major > target major, we definitely meet the requirement.
+	# If our major < target major, we definitely don't meet the requirement.
+	if [ "$unamemajor" -gt "$argmajor" ]; then
+		return 0
+	fi
+	if [ "$unamemajor" -lt "$argmajor" ]; then
+		return 1
+	fi
+
+	# Only when our major is the same as the target, we need to compare
+	# the minor number.
+	# In this case, if our minor >= target minor, we meet the requirement.
+	if [ "$unameminor" -ge "$argminor" ]; then
+		return 0;
+	fi
+	return 1
 }
 
 # Get subvolume id for specified path
-- 
2.37.3

[PATCH 2/3] btrfs-progs: mkfs-tests/025: fix the wrong function call

[Qu Wenruo]

[BUG]
Btrfs-progs test case mkfs/025 will output the following error:

  # make TEST=025\* test-mkfs
    [TEST]   mkfs-tests.sh
    [TEST/mkfs]   025-zoned-parallel
  ./test.sh: line 11: !check_min_kernel_version: command not found

[CAUSE]
There lacks a space between "!" and the function we called.

[FIX]
Add back the missing space.

Note that, this requires the previous fix on check_min_kernel_version(),
or it will not properly work on v6.x kernels.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/mkfs-tests/025-zoned-parallel/test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/mkfs-tests/025-zoned-parallel/test.sh b/tests/mkfs-tests/025-zoned-parallel/test.sh
index 8cad906cd5d1..83274bb23447 100755
--- a/tests/mkfs-tests/025-zoned-parallel/test.sh
+++ b/tests/mkfs-tests/025-zoned-parallel/test.sh
@@ -8,7 +8,7 @@ source "$TEST_TOP/common"
 setup_root_helper
 prepare_test_dev
 
-if !check_min_kernel_version 5.12; then
+if ! check_min_kernel_version 5.12; then
 	_notrun "zoned tests need kernel 5.12 and newer"
 fi
 
-- 
2.37.3

[PATCH 3/3] btrfs-progs: convert-tests/022: add reiserfs support check

[Qu Wenruo]

[BUG]
Btrfs-progs test case convert/022 will fail if the system doesn't have
reiserfs support nor reiserfs user space tools:

  # make TEST=022\* test-convert
    [TEST]   convert-tests.sh
  WARNING: reiserfs filesystem not listed in /proc/filesystems, some tests might be skipped
    [TEST/conv]   022-reiserfs-parent-ref
  Failed system wide prerequisities: mkreiserfs
  test failed for case 022-reiserfs-parent-ref
  make: *** [Makefile:443: test-convert] Error 1

[CAUSE]
Unlike other test cases, convert/022 doesn't even check if we have
kernel support for it.

[FIX]
Add the proper check before doing system wide prerequisities checks.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/convert-tests/022-reiserfs-parent-ref/test.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/convert-tests/022-reiserfs-parent-ref/test.sh b/tests/convert-tests/022-reiserfs-parent-ref/test.sh
index 66aaff7b6502..5d5ccdc5702f 100755
--- a/tests/convert-tests/022-reiserfs-parent-ref/test.sh
+++ b/tests/convert-tests/022-reiserfs-parent-ref/test.sh
@@ -2,6 +2,11 @@
 # Test that only toplevel directory self-reference is created
 
 source "$TEST_TOP/common"
+source "$TEST_TOP/common.convert"
+
+if ! check_kernel_support_reiserfs >/dev/null; then
+	_not_run "no reiserfs support"
+fi
 
 setup_root_helper
 prepare_test_dev
-- 
2.37.3

Re: [PATCH 2/3] btrfs-progs: mkfs-tests/025: fix the wrong function call

[Wang Yugui]

Hi,

> [BUG]
> Btrfs-progs test case mkfs/025 will output the following error:
> 
>   # make TEST=025\* test-mkfs
>     [TEST]   mkfs-tests.sh
>     [TEST/mkfs]   025-zoned-parallel
>   ./test.sh: line 11: !check_min_kernel_version: command not found
> 
> [CAUSE]
> There lacks a space between "!" and the function we called.
> 
> [FIX]
> Add back the missing space.
> 
> Note that, this requires the previous fix on check_min_kernel_version(),
> or it will not properly work on v6.x kernels.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/mkfs-tests/025-zoned-parallel/test.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/mkfs-tests/025-zoned-parallel/test.sh b/tests/mkfs-tests/025-zoned-parallel/test.sh
> index 8cad906cd5d1..83274bb23447 100755
> --- a/tests/mkfs-tests/025-zoned-parallel/test.sh
> +++ b/tests/mkfs-tests/025-zoned-parallel/test.sh
> @@ -8,7 +8,7 @@ source "$TEST_TOP/common"
>  setup_root_helper
>  prepare_test_dev
>  
> -if !check_min_kernel_version 5.12; then
> +if ! check_min_kernel_version 5.12; then
>  	_notrun "zoned tests need kernel 5.12 and newer"
>  fi

'_notrun' should be changed to '_not_run' too.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/10/05

>  
> -- 
> 2.37.3

Re: [PATCH 2/3] btrfs-progs: mkfs-tests/025: fix the wrong function call

[Qu Wenruo]

On 2022/10/5 14:43, Wang Yugui wrote:
> Hi,
>
>> [BUG]
>> Btrfs-progs test case mkfs/025 will output the following error:
>>
>>    # make TEST=025\* test-mkfs
>>      [TEST]   mkfs-tests.sh
>>      [TEST/mkfs]   025-zoned-parallel
>>    ./test.sh: line 11: !check_min_kernel_version: command not found
>>
>> [CAUSE]
>> There lacks a space between "!" and the function we called.
>>
>> [FIX]
>> Add back the missing space.
>>
>> Note that, this requires the previous fix on check_min_kernel_version(),
>> or it will not properly work on v6.x kernels.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/mkfs-tests/025-zoned-parallel/test.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/mkfs-tests/025-zoned-parallel/test.sh b/tests/mkfs-tests/025-zoned-parallel/test.sh
>> index 8cad906cd5d1..83274bb23447 100755
>> --- a/tests/mkfs-tests/025-zoned-parallel/test.sh
>> +++ b/tests/mkfs-tests/025-zoned-parallel/test.sh
>> @@ -8,7 +8,7 @@ source "$TEST_TOP/common"
>>   setup_root_helper
>>   prepare_test_dev
>>
>> -if !check_min_kernel_version 5.12; then
>> +if ! check_min_kernel_version 5.12; then
>>   	_notrun "zoned tests need kernel 5.12 and newer"
>>   fi
>
> '_notrun' should be changed to '_not_run' too.

That's right, I forgot this as my previous patch would render the path
unreachable for newer kernels.

David, can you fix it when merging?

Thanks,
Qu
>
> Best Regards
> Wang Yugui (wangyugui@e16-tech.com)
> 2022/10/05
>
>>
>> --
>> 2.37.3
>
>

Re: [PATCH 2/3] btrfs-progs: mkfs-tests/025: fix the wrong function call

[David Sterba]

On Wed, Oct 05, 2022 at 03:58:47PM +0800, Qu Wenruo wrote:
> On 2022/10/5 14:43, Wang Yugui wrote:
> >>   tests/mkfs-tests/025-zoned-parallel/test.sh | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tests/mkfs-tests/025-zoned-parallel/test.sh b/tests/mkfs-tests/025-zoned-parallel/test.sh
> >> index 8cad906cd5d1..83274bb23447 100755
> >> --- a/tests/mkfs-tests/025-zoned-parallel/test.sh
> >> +++ b/tests/mkfs-tests/025-zoned-parallel/test.sh
> >> @@ -8,7 +8,7 @@ source "$TEST_TOP/common"
> >>   setup_root_helper
> >>   prepare_test_dev
> >>
> >> -if !check_min_kernel_version 5.12; then
> >> +if ! check_min_kernel_version 5.12; then
> >>   	_notrun "zoned tests need kernel 5.12 and newer"
> >>   fi
> >
> > '_notrun' should be changed to '_not_run' too.
> 
> That's right, I forgot this as my previous patch would render the path
> unreachable for newer kernels.
> 
> David, can you fix it when merging?

Fixed and pushed, thanks.

Re: [PATCH 1/3] btrfs-progs: tests: fix the wrong kernel version check

[David Sterba]

On Wed, Oct 05, 2022 at 10:25:12AM +0800, Qu Wenruo wrote:
> [BUG]
> After upgrading to kernel v6.0-rc, btrfs-progs selftest mkfs/001 no
> longer checks single device RAID0 and other new features introduced in
> v5.13:
> 
>   # make TEST=001\* test-mkfs
>     [TEST]   mkfs-tests.sh
>     [TEST/mkfs]   001-basic-profiles
>   $ grep -IR "RAID0\/1" tests/mkfs-tests-results.txt
>   ^^^ No output
> 
> [CAUSE]
> The existing check_min_kernel_version() is doing an incorrect check.
> 
> The old check looks like this:
> 
> 	[ "$unamemajor" -lt "$argmajor" ] || return 1
> 	[ "$unameminor" -lt "$argminor" ] || return 1
> 	return 0
> 
> For 6.0-rc kernels, we have the following values for mkfs/001
> 
>  $unamemajor = 6
>  $unameminor = 0
>  $argmajor   = 5
>  $argminor   = 12
> 
> The first check doesn't exit immediately, as 6 > 5.
> Then we check the minor, which is already incorrect.
> 
> If our major is larger than target major, we should exit immediate with
> 0.
> 
> [FIX]
> Fix the check and add extra comment.
> 
> Personally speaking I'm not a fan or short compare and return, thus all
> the checks will explicit "if []; then fi" checks.

Agreed the explicit if should be used in most cases, what is probably ok
a 'command || _fail' pattern for simple commands. I try to unify the
shell coding style in new patches but some bits may slip through.

Re: [PATCH 3/3] btrfs-progs: convert-tests/022: add reiserfs support check

[David Sterba]

On Wed, Oct 05, 2022 at 10:25:14AM +0800, Qu Wenruo wrote:
> [BUG]
> Btrfs-progs test case convert/022 will fail if the system doesn't have
> reiserfs support nor reiserfs user space tools:
> 
>   # make TEST=022\* test-convert
>     [TEST]   convert-tests.sh
>   WARNING: reiserfs filesystem not listed in /proc/filesystems, some tests might be skipped
>     [TEST/conv]   022-reiserfs-parent-ref
>   Failed system wide prerequisities: mkreiserfs
>   test failed for case 022-reiserfs-parent-ref
>   make: *** [Makefile:443: test-convert] Error 1
> 
> [CAUSE]
> Unlike other test cases, convert/022 doesn't even check if we have
> kernel support for it.
> 
> [FIX]
> Add the proper check before doing system wide prerequisities checks.

I've moved it before the mkreiserfs check, I'd like to keep the
setup_root_helper and prepare_test_dev first in the list for
consistency.

Re: [PATCH 0/3] btrfs-progs: selftests fixes

[David Sterba]

On Wed, Oct 05, 2022 at 10:25:11AM +0800, Qu Wenruo wrote:
> There are 3 bugs exposed during my tests for unified mkfs features, and
> they are all from the selftest itself:
> 
> - check_min_kernel_version() doesn't handle 6.x kernels at all
>   The original check never handles major number check properly.
>   And when we change major number, returns in correct number now.
> 
> - Missing a space between "!" and function call
>   This bug is there for a long time. 
> 
>   Without previous fix, one may incorrectly remove the "!" and cause
>   new problems.
> 
> - Convert/022 doesn't check if we have support for reiserfs
> 
> Qu Wenruo (3):
>   btrfs-progs: tests: fix the wrong kernel version check
>   btrfs-progs: mkfs-tests/025: fix the wrong function call
>   btrfs-progs: convert-tests/022: add reiserfs support check

I've foled patch 2 to the one that that introduced it so we don't have
broken tests. The other two merged to devel, thanks.