On Tue, Jul 18, 2017 at 01:05:07PM +0800, Eryu Guan wrote:
> > > if [ -f ${RESULT_DIR}/require_test ]; then
> > > - _check_test_fs || err=true
> > > + if ! _check_test_fs ; then
> > > + err=true
> > > + echo "Trying to repair broken TEST_DEV file system"
> > > + _repair_test_fs
>
> Should we stop running if _repair_test_fs failed to fix TEST_DEV? I
> think that was what Darrick first suggested. And I think unfixable
> errors will cause subsequent tests to fail too.
Seems reasonable to me. I'll take a look at it.
> > > +_repair_test_fs()
> > > +{
> > > + case $FSTYP in
> > > + xfs)
> > > + _repair_xfs_test_fs "$@" >$tmp.repair 2>&1
> > > + res=$?
>
> Better to declare res as a local variable. I know this was copied from
> _repair_scratch_fs, but better to get it improved in new code :)
Ack.
> > Structurally this all looks ok, but it's a little weird that we have
> > _scratch_xfs_repair for the scratch device (object-verb) but
> > _repair_test_fs (verb-object) for the test device.
>
> My best guess is that _repair_scratch_fs and _repair_test_fs are helpers
> in a higher level, and are defined in common/rc, they deal with all
> filesystem types.
>
> _scratch_xfs_repair is xfs specific, and defined in common/xfs, it only
> deals with xfs. So the naming schema is different but they are also in
> different "namespace"s, which looks fine to me :)
I also didn't like _test_repair_fs because it's not clear whether
"test" is a verb or a noun.
So one thing we could do is _xfs_scratch_repair and _xfs_test_repair?
The other thing is how much of the cleanup in common/xfs should be
segregated into a separate commit (and I'm not sure how competent I'm
going to be ate doing that cleanup, but I'm willing to give it a go).
- Ted
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
