On Tue, Jul 18, 2017 at 01:30:09PM -0400, Theodore Ts'o wrote:
> 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?
Then all existing _scratch_xfs_repair calls need a rename, but I'm fine
with the rename.
>
> 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).
I think we can do minimal cleanup in this patch (like the rename of
_scratch_xfs_repair to _xfs_scratch_repair) and do other cleanups in
separate commits (if there's still any).
Thanks!
Eryu
--
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
