* [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption
@ 2014-12-15 17:09 Eryu Guan
2014-12-15 17:09 ` [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures Eryu Guan
2014-12-15 20:31 ` [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Dave Chinner
0 siblings, 2 replies; 6+ messages in thread
From: Eryu Guan @ 2014-12-15 17:09 UTC (permalink / raw)
To: fstests; +Cc: Eryu Guan
Tests like xfs/179 depend on the return value of _check_scratch_fs to
detect fs corruption, but _check_$FSTYP_filesystem always returns 0.
Make _check_$FSTYP_filesystem return failure on corruption.
Also don't exit if these functions called by 'check', like what
_check_xfs_filesystem() does.
Signed-off-by: Eryu Guan <eguan@redhat.com>
---
common/rc | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/common/rc b/common/rc
index 692d45c..87d2928 100644
--- a/common/rc
+++ b/common/rc
@@ -1717,7 +1717,10 @@ _check_generic_filesystem()
if [ $ok -eq 0 ]; then
status=1
- exit 1
+ if [ "$iam" != "check" ]; then
+ exit 1
+ fi
+ return 1
fi
return 0
@@ -1819,6 +1822,7 @@ _check_xfs_filesystem()
if [ "$iam" != "check" ]; then
exit 1
fi
+ return 1
fi
return 0
@@ -1863,7 +1867,8 @@ _check_udf_filesystem()
$here/src/udf_test $OPT_ARG $device | tee $seqres.checkfs | egrep "Error|Warning" | \
_udf_test_known_error_filter | \
egrep -iv "Error count:.*[0-9]+.*total occurrences:.*[0-9]+|Warning count:.*[0-9]+.*total occurrences:.*[0-9]+" && \
- echo "Warning UDF Verifier reported errors see $seqres.checkfs."
+ echo "Warning UDF Verifier reported errors see $seqres.checkfs." && return 1
+ return 0
}
_check_xfs_test_fs()
@@ -1928,7 +1933,10 @@ _check_btrfs_filesystem()
if [ $ok -eq 0 ]; then
status=1
- exit 1
+ if [ "$iam" != "check" ]; then
+ exit 1
+ fi
+ return 1
fi
return 0
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures
2014-12-15 17:09 [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Eryu Guan
@ 2014-12-15 17:09 ` Eryu Guan
2014-12-15 23:27 ` Dave Chinner
2014-12-15 20:31 ` [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Dave Chinner
1 sibling, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2014-12-15 17:09 UTC (permalink / raw)
To: fstests; +Cc: Eryu Guan
Currently if _check_test_fs and/or _check_scratch_fs find corruption,
the test itself is still reported as pass, like
[root@hp-dl388eg8-01 xfstests]# ./check xfs/071 xfs/072
FSTYP -- xfs (non-debug)
PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc7+
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda6
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/sda6 /mnt/testarea/scratch
xfs/071 2s
_check_xfs_filesystem: filesystem on /dev/sda6 is inconsistent (r) (see /root/xfstests/results//xfs/071.full)
xfs/072 1s
Ran: xfs/071 xfs/072
Passed all 2 tests
[root@hp-dl388eg8-01 xfstests]# echo $?
0
Usually it's not a problem, but it does confuse scripts that depend on
return value of check. Update check to treat _check_{test,scratch}_fs
failures as test failures too, new test output is like
[root@hp-dl388eg8-01 xfstests]# ./check xfs/071 xfs/072
FSTYP -- xfs (non-debug)
PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc7+
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda6
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/sda6 /mnt/testarea/scratch
xfs/071 2s ... 2s
_check_xfs_filesystem: filesystem on /dev/sda6 is inconsistent (r) (see /root/xfstests/results//xfs/071.full)
xfs/072 1s ... 1s
Ran: xfs/071 xfs/072
Failures: xfs/071
Failed 1 of 2 tests
[root@hp-dl388eg8-01 xfstests]# echo $?
1
Signed-off-by: Eryu Guan <eguan@redhat.com>
---
Note that the xfs/071 corruption issue is an xfs_repair bug,
Eric has sent a patch to fix it, see
http://www.spinics.net/lists/xfs/msg31018.html
check | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/check b/check
index 42a1ac2..4cf7070 100755
--- a/check
+++ b/check
@@ -611,6 +611,16 @@ for section in $HOST_OPTIONS_SECTIONS; do
err=true
fi
fi
+ try="$try $seqnum"
+ n_try=`expr $n_try + 1`
+ if [ -f ${RESULT_DIR}/require_test ]; then
+ _check_test_fs || err=true
+ rm -f ${RESULT_DIR}/require_test
+ fi
+ if [ -f ${RESULT_DIR}/require_scratch ]; then
+ _check_scratch_fs || err=true
+ rm -f ${RESULT_DIR}/require_scratch
+ fi
fi
fi
@@ -623,15 +633,6 @@ for section in $HOST_OPTIONS_SECTIONS; do
n_bad=`expr $n_bad + 1`
quick=false
fi
- if [ ! -f $seqres.notrun ]
- then
- try="$try $seqnum"
- n_try=`expr $n_try + 1`
- test -f ${RESULT_DIR}/require_test && _check_test_fs
- rm -f ${RESULT_DIR}/require_test
- test -f ${RESULT_DIR}/require_scratch && _check_scratch_fs
- rm -f ${RESULT_DIR}/require_scratch
- fi
seq="after_$seqnum"
done
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption
2014-12-15 17:09 [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Eryu Guan
2014-12-15 17:09 ` [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures Eryu Guan
@ 2014-12-15 20:31 ` Dave Chinner
2014-12-16 3:28 ` Eryu Guan
1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2014-12-15 20:31 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
On Tue, Dec 16, 2014 at 01:09:26AM +0800, Eryu Guan wrote:
> Tests like xfs/179 depend on the return value of _check_scratch_fs to
> detect fs corruption, but _check_$FSTYP_filesystem always returns 0.
which means xfs/179 is wrong, because when _check_scratch_fs fails it
exits immediately with status = 1, which triggers the test harness
to record a test failure. (i.e. xfs/179 != check, so exits)
So this change is not actually going to allow tests to check the
return value of _check_scratch_fs, and check itself doesn't care if
the filesystem is corrupt or not - it just reports the state and
moves on to the next test....
> Also don't exit if these functions called by 'check', like what
> _check_xfs_filesystem() does.
That's a separate issue, and should be in it's own patch ;)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures
2014-12-15 17:09 ` [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures Eryu Guan
@ 2014-12-15 23:27 ` Dave Chinner
2014-12-16 3:31 ` Eryu Guan
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2014-12-15 23:27 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
On Tue, Dec 16, 2014 at 01:09:27AM +0800, Eryu Guan wrote:
> Currently if _check_test_fs and/or _check_scratch_fs find corruption,
> the test itself is still reported as pass, like
>
> [root@hp-dl388eg8-01 xfstests]# ./check xfs/071 xfs/072
> FSTYP -- xfs (non-debug)
> PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc7+
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda6
> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/sda6 /mnt/testarea/scratch
>
> xfs/071 2s
> _check_xfs_filesystem: filesystem on /dev/sda6 is inconsistent (r) (see /root/xfstests/results//xfs/071.full)
> xfs/072 1s
> Ran: xfs/071 xfs/072
> Passed all 2 tests
>
> [root@hp-dl388eg8-01 xfstests]# echo $?
> 0
>
> Usually it's not a problem, but it does confuse scripts that depend on
> return value of check. Update check to treat _check_{test,scratch}_fs
> failures as test failures too, new test output is like
>
> [root@hp-dl388eg8-01 xfstests]# ./check xfs/071 xfs/072
> FSTYP -- xfs (non-debug)
> PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc7+
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda6
> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 /dev/sda6 /mnt/testarea/scratch
>
> xfs/071 2s ... 2s
> _check_xfs_filesystem: filesystem on /dev/sda6 is inconsistent (r) (see /root/xfstests/results//xfs/071.full)
> xfs/072 1s ... 1s
> Ran: xfs/071 xfs/072
> Failures: xfs/071
> Failed 1 of 2 tests
>
> [root@hp-dl388eg8-01 xfstests]# echo $?
> 1
>
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
>
> Note that the xfs/071 corruption issue is an xfs_repair bug,
> Eric has sent a patch to fix it, see
>
> http://www.spinics.net/lists/xfs/msg31018.html
>
> check | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/check b/check
> index 42a1ac2..4cf7070 100755
> --- a/check
> +++ b/check
> @@ -611,6 +611,16 @@ for section in $HOST_OPTIONS_SECTIONS; do
> err=true
> fi
> fi
> + try="$try $seqnum"
> + n_try=`expr $n_try + 1`
> + if [ -f ${RESULT_DIR}/require_test ]; then
> + _check_test_fs || err=true
> + rm -f ${RESULT_DIR}/require_test
> + fi
> + if [ -f ${RESULT_DIR}/require_scratch ]; then
> + _check_scratch_fs || err=true
> + rm -f ${RESULT_DIR}/require_scratch
> + fi
> fi
>
> fi
> @@ -623,15 +633,6 @@ for section in $HOST_OPTIONS_SECTIONS; do
> n_bad=`expr $n_bad + 1`
> quick=false
> fi
> - if [ ! -f $seqres.notrun ]
> - then
> - try="$try $seqnum"
> - n_try=`expr $n_try + 1`
> - test -f ${RESULT_DIR}/require_test && _check_test_fs
> - rm -f ${RESULT_DIR}/require_test
> - test -f ${RESULT_DIR}/require_scratch && _check_scratch_fs
> - rm -f ${RESULT_DIR}/require_scratch
> - fi
>
> seq="after_$seqnum"
Looks fine as is, but in the interest of slowly cleaning up check,
can you factor this out into a helper function such as
"check_filesystems" using 8 space tabs?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption
2014-12-15 20:31 ` [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Dave Chinner
@ 2014-12-16 3:28 ` Eryu Guan
0 siblings, 0 replies; 6+ messages in thread
From: Eryu Guan @ 2014-12-16 3:28 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Tue, Dec 16, 2014 at 07:31:29AM +1100, Dave Chinner wrote:
> On Tue, Dec 16, 2014 at 01:09:26AM +0800, Eryu Guan wrote:
> > Tests like xfs/179 depend on the return value of _check_scratch_fs to
> > detect fs corruption, but _check_$FSTYP_filesystem always returns 0.
>
> which means xfs/179 is wrong, because when _check_scratch_fs fails it
> exits immediately with status = 1, which triggers the test harness
> to record a test failure. (i.e. xfs/179 != check, so exits)
>
> So this change is not actually going to allow tests to check the
> return value of _check_scratch_fs, and check itself doesn't care if
> the filesystem is corrupt or not - it just reports the state and
> moves on to the next test....
You're right, _check_scratch_fs sets status=1 before exit, this change
mainly benefits _check_{fs,scratch}_fs calls out of tests, like in
check. I'll update the commit message in v2.
>
> > Also don't exit if these functions called by 'check', like what
> > _check_xfs_filesystem() does.
>
> That's a separate issue, and should be in it's own patch ;)
Will do in v2.
Thanks for the review!
Eryu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures
2014-12-15 23:27 ` Dave Chinner
@ 2014-12-16 3:31 ` Eryu Guan
0 siblings, 0 replies; 6+ messages in thread
From: Eryu Guan @ 2014-12-16 3:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Tue, Dec 16, 2014 at 10:27:39AM +1100, Dave Chinner wrote:
> On Tue, Dec 16, 2014 at 01:09:27AM +0800, Eryu Guan wrote:
[snip]
> > check | 19 ++++++++++---------
> > 1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/check b/check
> > index 42a1ac2..4cf7070 100755
> > --- a/check
> > +++ b/check
> > @@ -611,6 +611,16 @@ for section in $HOST_OPTIONS_SECTIONS; do
> > err=true
> > fi
> > fi
> > + try="$try $seqnum"
> > + n_try=`expr $n_try + 1`
> > + if [ -f ${RESULT_DIR}/require_test ]; then
> > + _check_test_fs || err=true
> > + rm -f ${RESULT_DIR}/require_test
> > + fi
> > + if [ -f ${RESULT_DIR}/require_scratch ]; then
> > + _check_scratch_fs || err=true
> > + rm -f ${RESULT_DIR}/require_scratch
> > + fi
> > fi
> >
> > fi
> > @@ -623,15 +633,6 @@ for section in $HOST_OPTIONS_SECTIONS; do
> > n_bad=`expr $n_bad + 1`
> > quick=false
> > fi
> > - if [ ! -f $seqres.notrun ]
> > - then
> > - try="$try $seqnum"
> > - n_try=`expr $n_try + 1`
> > - test -f ${RESULT_DIR}/require_test && _check_test_fs
> > - rm -f ${RESULT_DIR}/require_test
> > - test -f ${RESULT_DIR}/require_scratch && _check_scratch_fs
> > - rm -f ${RESULT_DIR}/require_scratch
> > - fi
> >
> > seq="after_$seqnum"
>
> Looks fine as is, but in the interest of slowly cleaning up check,
> can you factor this out into a helper function such as
> "check_filesystems" using 8 space tabs?
Sure, will do in v2.
Thanks,
Eryu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-16 3:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-15 17:09 [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Eryu Guan
2014-12-15 17:09 ` [PATCH 2/2] check: treat _check_{test,scratch}_fs failures as test failures Eryu Guan
2014-12-15 23:27 ` Dave Chinner
2014-12-16 3:31 ` Eryu Guan
2014-12-15 20:31 ` [PATCH 1/2] common: return failure if _check_xxx_filesystem finds corruption Dave Chinner
2014-12-16 3:28 ` Eryu Guan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox