diff mbox

[1/2] common: return failure if _check_xxx_filesystem finds corruption

Message ID 1418663367-18755-1-git-send-email-eguan@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eryu Guan Dec. 15, 2014, 5:09 p.m. UTC
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(-)

Comments

Dave Chinner Dec. 15, 2014, 8:31 p.m. UTC | #1
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.
Eryu Guan Dec. 16, 2014, 3:28 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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