@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
( cc ext4 mailist, to get more review)
Thanks,
Zorro
Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7
in the above URL. I didn't add it since I'm not sure if this id will
be the final id when the commit is finally merged to the mainline
kernel (Linus tree)?
And maybe you can describe *a little* more in commit log.
Yes I can add a few words in the commit log, but actually I expect the
reader of this test reads the commit message of the mentioned commit
"ext4: Fix tune2fs checksum failure for mounted filesystem", which I
think is more precise.
quoted
Signed-off-by: Boyang Xue <redacted>
---
Hi,
This is a new regression test for the patch
@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
I will correct it in the next version. Thanks.
quoted
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm
wondering why we shouldn't do _scratch_unmount here? And I see at
least another similar structured test ext4/306 do _scratch_unmount in
_cleanup().
quoted
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
No. I will remove this line in my next version.
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
How did you test that? The error output didn't break the "golden
image" in my test.
( cc ext4 mailist, to get more review)
Thanks,
Zorro
From: Jan Kara <jack@suse.cz> Date: 2021-08-18 14:26:08
On Wed 18-08-21 21:20:44, Boyang Xue wrote:
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
FWIW I agree with Boyang here. For this test to make sense for any other
filesystem other the filesystem would need to read fs metadata through
buffer cache in _get_fs_super(). Furthermore it is somewhat ext2/3/4
specific (due to historical reasons) that reading superblock from the
buffer cache of a mounted filesystem is expected to result in something
sensible. Usually this is plain unsupported use...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7
in the above URL. I didn't add it since I'm not sure if this id will
be the final id when the commit is finally merged to the mainline
kernel (Linus tree)?
quoted
And maybe you can describe *a little* more in commit log.
Yes I can add a few words in the commit log, but actually I expect the
reader of this test reads the commit message of the mentioned commit
"ext4: Fix tune2fs checksum failure for mounted filesystem", which I
think is more precise.
quoted
quoted
Signed-off-by: Boyang Xue <redacted>
---
Hi,
This is a new regression test for the patch
@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
I will correct it in the next version. Thanks.
quoted
quoted
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm
wondering why we shouldn't do _scratch_unmount here? And I see at
least another similar structured test ext4/306 do _scratch_unmount in
_cleanup().
The SCRATCH_DEV will be umounted, don't need to do that in the end of each
test cases, except you need to do something on the unmounted SCRATCH_DEV
in the end.
I don't know why ext4/306 has that, maybe due to old reason, or we didn't
notice/care that when we reviewed it? And it's not worth sending a patch
just for removing this "not wrong but just redundant" line now. Except a
patch trys to cleanup all _cleanup().
quoted
quoted
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
No. I will remove this line in my next version.
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
Just checking. That's fine for me to keep this case as an ext4 only case.
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
How did you test that? The error output didn't break the "golden
image" in my test.
[root@xx-xxxx-xx xfstests-dev]# ./check ext4/309
FSTYP -- ext4
PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021
MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch
ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad)
--- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800
+++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800
@@ -1,2 +1,4 @@
QA output created by 309
Silence is golden
+/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev
+Couldn't find valid filesystem superblock.
...
(Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff)
Ran: ext4/309
Failures: ext4/309
Failed 1 of 1 tests
[root@xx-xxxx-xx xfstests-dev]# git diff
...
_scratch_mkfs >/dev/null 2>&1
_scratch_mount
-echo "ext4/309" > $SCRATCH_MNT/309.tmp
+echo "This is a test" > $SCRATCH_MNT/testfile
_scratch_shutdown
-_scratch_cycle_mount
-$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
-if [ $? -eq 0 ]; then
- status=0
-else
- status=1
-fi
+_scratch_cycle_mount
+$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
+status=0
exit
Thanks,
Zorro
quoted
( cc ext4 mailist, to get more review)
Thanks,
Zorro
From: Zorro Lang <hidden> Date: 2021-08-18 17:04:13
On Wed, Aug 18, 2021 at 04:26:01PM +0200, Jan Kara wrote:
On Wed 18-08-21 21:20:44, Boyang Xue wrote:
quoted
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
FWIW I agree with Boyang here. For this test to make sense for any other
filesystem other the filesystem would need to read fs metadata through
buffer cache in _get_fs_super(). Furthermore it is somewhat ext2/3/4
specific (due to historical reasons) that reading superblock from the
buffer cache of a mounted filesystem is expected to result in something
sensible. Usually this is plain unsupported use...
Thanks for this explanation:) I didn't ask for extending this test to be
a generic test, just checking others ideas:) Due to although tune2fs is
special, but the test steps are common:
1) mkfs
2) mount
3) write io
4) shutdown fs
5) umount && mount
6) read sb from a mounted fs (make sure using tune2fs for ext4)
Anyway, keep this test as ext4 only is fine for me :)
Thanks,
Zorro
Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7
in the above URL. I didn't add it since I'm not sure if this id will
be the final id when the commit is finally merged to the mainline
kernel (Linus tree)?
quoted
And maybe you can describe *a little* more in commit log.
Yes I can add a few words in the commit log, but actually I expect the
reader of this test reads the commit message of the mentioned commit
"ext4: Fix tune2fs checksum failure for mounted filesystem", which I
think is more precise.
quoted
quoted
Signed-off-by: Boyang Xue <redacted>
---
Hi,
This is a new regression test for the patch
@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
I will correct it in the next version. Thanks.
quoted
quoted
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm
wondering why we shouldn't do _scratch_unmount here? And I see at
least another similar structured test ext4/306 do _scratch_unmount in
_cleanup().
The SCRATCH_DEV will be umounted, don't need to do that in the end of each
test cases, except you need to do something on the unmounted SCRATCH_DEV
in the end.
I don't know why ext4/306 has that, maybe due to old reason, or we didn't
notice/care that when we reviewed it? And it's not worth sending a patch
just for removing this "not wrong but just redundant" line now. Except a
patch trys to cleanup all _cleanup().
OK. I will remove the _cleanup() in my next version, unless someone
else has other opinions.
quoted
quoted
quoted
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
No. I will remove this line in my next version.
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
Just checking. That's fine for me to keep this case as an ext4 only case.
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
How did you test that? The error output didn't break the "golden
image" in my test.
[root@xx-xxxx-xx xfstests-dev]# ./check ext4/309
FSTYP -- ext4
PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021
MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch
ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad)
--- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800
+++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800
@@ -1,2 +1,4 @@
QA output created by 309
Silence is golden
+/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev
+Couldn't find valid filesystem superblock.
...
(Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff)
Ran: ext4/309
Failures: ext4/309
Failed 1 of 1 tests
[root@xx-xxxx-xx xfstests-dev]# git diff
...
_scratch_mkfs >/dev/null 2>&1
_scratch_mount
-echo "ext4/309" > $SCRATCH_MNT/309.tmp
+echo "This is a test" > $SCRATCH_MNT/testfile
_scratch_shutdown
-_scratch_cycle_mount
-$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
-if [ $? -eq 0 ]; then
- status=0
-else
- status=1
-fi
+_scratch_cycle_mount
+$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
+status=0
exit
Unless I missed something, I would say that, tune2fs' error output
went to .out.bad just because you had modified the tune2fs line from
mine:
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
to yours:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
My original version redirecting stderr and stdout both to $seqres.full
doesn't break the "golden output". Test log:
As far as I can understand, there're at least two approaches to mark a
test pass or fail in xfstests:
1) Compare the output of the test with the "golden output" (i.e.
309.out), I guess this is the approach you mentioned.
2) Exit the test with the value of $status (i.e. status=0 - pass,
status=non-zero - fail)
I'm using the 2) here, not using 1) with the output of tune2fs
compared with the 309.out, because the output of tune2fs can be
different in different runs. An example output of a successful tune2fs
run is like (sorry for this long paste):
Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7
in the above URL. I didn't add it since I'm not sure if this id will
be the final id when the commit is finally merged to the mainline
kernel (Linus tree)?
quoted
And maybe you can describe *a little* more in commit log.
Yes I can add a few words in the commit log, but actually I expect the
reader of this test reads the commit message of the mentioned commit
"ext4: Fix tune2fs checksum failure for mounted filesystem", which I
think is more precise.
quoted
quoted
Signed-off-by: Boyang Xue <redacted>
---
Hi,
This is a new regression test for the patch
@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
I will correct it in the next version. Thanks.
quoted
quoted
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm
wondering why we shouldn't do _scratch_unmount here? And I see at
least another similar structured test ext4/306 do _scratch_unmount in
_cleanup().
The SCRATCH_DEV will be umounted, don't need to do that in the end of each
test cases, except you need to do something on the unmounted SCRATCH_DEV
in the end.
I don't know why ext4/306 has that, maybe due to old reason, or we didn't
notice/care that when we reviewed it? And it's not worth sending a patch
just for removing this "not wrong but just redundant" line now. Except a
patch trys to cleanup all _cleanup().
OK. I will remove the _cleanup() in my next version, unless someone
else has other opinions.
quoted
quoted
quoted
quoted
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
No. I will remove this line in my next version.
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
Just checking. That's fine for me to keep this case as an ext4 only case.
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
How did you test that? The error output didn't break the "golden
image" in my test.
[root@xx-xxxx-xx xfstests-dev]# ./check ext4/309
FSTYP -- ext4
PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021
MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch
ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad)
--- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800
+++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800
@@ -1,2 +1,4 @@
QA output created by 309
Silence is golden
+/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev
+Couldn't find valid filesystem superblock.
...
(Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff)
Ran: ext4/309
Failures: ext4/309
Failed 1 of 1 tests
[root@xx-xxxx-xx xfstests-dev]# git diff
...
_scratch_mkfs >/dev/null 2>&1
_scratch_mount
-echo "ext4/309" > $SCRATCH_MNT/309.tmp
+echo "This is a test" > $SCRATCH_MNT/testfile
_scratch_shutdown
-_scratch_cycle_mount
-$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
-if [ $? -eq 0 ]; then
- status=0
-else
- status=1
-fi
+_scratch_cycle_mount
+$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
+status=0
exit
Unless I missed something, I would say that, tune2fs' error output
went to .out.bad just because you had modified the tune2fs line from
mine:
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
to yours:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
I think what Zorro is suggesting is that *when test fails* just dump
stderr and let the error message break golden image to fail the test, so
there's no need to set exit status.
My original version redirecting stderr and stdout both to $seqres.full
doesn't break the "golden output". Test log:
And this failure doesn't give any clues why it failed, you must check
$seqres.full file to find out.
With stderr breaking golden image, you'll see what fails more easily,
e.g.
QA output created by 309
Silence is golden
+/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev
+Couldn't find valid filesystem superblock.
...
(Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff)
Thanks,
Eryu
Thanks. I see the commit id e905fbe3fd0fdb90052f6efdf88f50a78833cfe7
in the above URL. I didn't add it since I'm not sure if this id will
be the final id when the commit is finally merged to the mainline
kernel (Linus tree)?
quoted
And maybe you can describe *a little* more in commit log.
Yes I can add a few words in the commit log, but actually I expect the
reader of this test reads the commit message of the mentioned commit
"ext4: Fix tune2fs checksum failure for mounted filesystem", which I
think is more precise.
quoted
quoted
Signed-off-by: Boyang Xue <redacted>
---
Hi,
This is a new regression test for the patch
@@ -0,0 +1,42 @@+#! /bin/bash+# SPDX-License-Identifier: GPL-2.0+# Copyright (c) 2021 YOUR NAME HERE. All Rights Reserved.
^^^^^^^^^^^^^^
Write your copyright
I will correct it in the next version. Thanks.
quoted
quoted
+#
+# FS QA Test 309
+#
+# Test that tune2fs doesn't fail after ext4 shutdown
+# Regression test for commit:
+# ext4: Fix tune2fs checksum failure for mounted filesystem
+#
+. ./common/preamble
+_begin_fstest auto rw quick
+
+_cleanup()
+{
+ _scratch_unmount
+}
I think the umount isn't necessary, so the specific _cleanup isn't
needed either.
The $SCRATCH_DEV was still mounted before this _cleanup(), so I'm
wondering why we shouldn't do _scratch_unmount here? And I see at
least another similar structured test ext4/306 do _scratch_unmount in
_cleanup().
The SCRATCH_DEV will be umounted, don't need to do that in the end of each
test cases, except you need to do something on the unmounted SCRATCH_DEV
in the end.
I don't know why ext4/306 has that, maybe due to old reason, or we didn't
notice/care that when we reviewed it? And it's not worth sending a patch
just for removing this "not wrong but just redundant" line now. Except a
patch trys to cleanup all _cleanup().
OK. I will remove the _cleanup() in my next version, unless someone
else has other opinions.
quoted
quoted
quoted
quoted
+
+# Import common functions.
+. ./common/filter
Do you use any filter helpers below?
No. I will remove this line in my next version.
quoted
quoted
+
+# real QA test starts here
+_supported_fs ext4
I'm wondering if this case can be a generic case, there's nothing
ext4 specified operations, except this line:
"$TUNE2FS_PROG -l $SCRATCH_DEV"
Hmm... if we can change this line to something likes _get_fs_super(),
it might help to make this test to be a generic test.
I think this bug is heavily related to "tune2fs", ext4 only. So I
guess an ext4 only test is enough?
Just checking. That's fine for me to keep this case as an ext4 only case.
Don't need to change the status value, how about write as:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
The error output will break the golden image directly.
How did you test that? The error output didn't break the "golden
image" in my test.
[root@xx-xxxx-xx xfstests-dev]# ./check ext4/309
FSTYP -- ext4
PLATFORM -- Linux/x86_64 xx-xxxx-xx 5.14.0-rc4-xfs #14 SMP Thu Aug 12 00:56:07 CST 2021
MKFS_OPTIONS -- /dev/mapper/testvg-scratchdev
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/mapper/testvg-scratchdev /mnt/scratch
ext4/309 5s ... - output mismatch (see /root/git/xfstests-dev/results//ext4/309.out.bad)
--- tests/ext4/309.out 2021-08-18 18:17:15.925427714 +0800
+++ /root/git/xfstests-dev/results//ext4/309.out.bad 2021-08-19 00:17:57.001648868 +0800
@@ -1,2 +1,4 @@
QA output created by 309
Silence is golden
+/usr/sbin/tune2fs: Superblock checksum does not match superblock while trying to open /dev/mapper/testvg-scratchdev
+Couldn't find valid filesystem superblock.
...
(Run 'diff -u /root/git/xfstests-dev/tests/ext4/309.out /root/git/xfstests-dev/results//ext4/309.out.bad' to see the entire diff)
Ran: ext4/309
Failures: ext4/309
Failed 1 of 1 tests
[root@xx-xxxx-xx xfstests-dev]# git diff
...
_scratch_mkfs >/dev/null 2>&1
_scratch_mount
-echo "ext4/309" > $SCRATCH_MNT/309.tmp
+echo "This is a test" > $SCRATCH_MNT/testfile
_scratch_shutdown
-_scratch_cycle_mount
-$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
-if [ $? -eq 0 ]; then
- status=0
-else
- status=1
-fi
+_scratch_cycle_mount
+$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
+status=0
exit
Unless I missed something, I would say that, tune2fs' error output
went to .out.bad just because you had modified the tune2fs line from
mine:
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
to yours:
$TUNE2FS_PROG -l $SCRATCH_DEV >/dev/null
Yes, I've metioned that in my first reply above.
My original version redirecting stderr and stdout both to $seqres.full
doesn't break the "golden output". Test log:
As far as I can understand, there're at least two approaches to mark a
test pass or fail in xfstests:
1) Compare the output of the test with the "golden output" (i.e.
309.out), I guess this is the approach you mentioned.
2) Exit the test with the value of $status (i.e. status=0 - pass,
status=non-zero - fail)
I'm using the 2) here, not using 1) with the output of tune2fs
compared with the 309.out, because the output of tune2fs can be
different in different runs. An example output of a successful tune2fs
run is like (sorry for this long paste):
You can
[1]
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full
status=0
exit
Or:
[2]
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1
if [ $? -ne 0 ];then
echo "Fail to get superblock from SCRATCH_DEV"
fi
status=0
exit
Or:
[3]
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 || exit
status=0
exit
Or:
[4]
$TUNE2FS_PROG -l $SCRATCH_DEV >> $seqres.full 2>&1 && status=0
exit
I perfer the 1st one, most xfstests cases fail by breaking golden image.
I just showed my review points, anyway I don't mind if maintainer would like
to choose anyone else :)
Thanks,
Zorro
[root@kvm101 repo_xfstests]# cat results/ext4/309.full
tune2fs 1.45.6 (20-Mar-2020)
Filesystem volume name: <none>
Last mounted on: /scratch
Filesystem UUID: 22975090-96d7-49ee-9b4f-ee6afe046219
Filesystem magic number: 0xEF53
Filesystem revision #: 1 (dynamic)
Filesystem features: has_journal ext_attr resize_inode dir_index
filetype needs_recovery extent 64bit flex_bg sparse_super large_file
huge_file dir_nlink extra_isize metadata_csum
Filesystem flags: signed_directory_hash
Default mount options: user_xattr acl
Filesystem state: clean
Errors behavior: Continue
Filesystem OS type: Linux
Inode count: 720896
Block count: 11534336
Reserved block count: 576716
Free blocks: 11280570
Free inodes: 720884
First block: 1
Block size: 1024
Fragment size: 1024
Group descriptor size: 64
Reserved GDT blocks: 256
Blocks per group: 8192
Fragments per group: 8192
Inodes per group: 512
Inode blocks per group: 128
Flex block group size: 16
Filesystem created: Thu Aug 19 05:45:26 2021
Last mount time: Thu Aug 19 05:45:26 2021
Last write time: Thu Aug 19 05:45:26 2021
Mount count: 2
Maximum mount count: -1
Last checked: Thu Aug 19 05:45:26 2021
Check interval: 0 (<none>)
Lifetime writes: 462 kB
Reserved blocks uid: 0 (user root)
Reserved blocks gid: 0 (group root)
First inode: 11
Inode size: 256
Required extra isize: 32
Desired extra isize: 32
Journal inode: 8
Default directory hash: half_md4
Directory Hash Seed: f5af5862-d415-460b-9a6b-97584578600f
Journal backup: inode blocks
Checksum type: crc32c
Checksum: 0x0f5b9c13