> > > diff --git a/tests/xfs/008 b/tests/xfs/008
> > > index a53f6c92..5bef44bb 100755
> > > --- a/tests/xfs/008
> > > +++ b/tests/xfs/008
> > > @@ -12,13 +12,6 @@ _begin_fstest rw ioctl auto quick
> > > status=0 # success is the default!
> > > pgsize=`$here/src/feature -s`
> > >
> > > -# Override the default cleanup function.
> > > -_cleanup()
> > > -{
> > > - rm -f $tmp.*
> > > - rm -rf $TEST_DIR/randholes.$$.*
> >
> > It's fine to keep the test files behind, but maybe
> > make that change more clear in commit message
> > because it was not clear to me that there was a change of
> > behavior when I read the commit message.
> >
> > Sorry to drop this extra burden on you, but this seems
> > important to me and I cannot add this information after the fact.
>
> The point of TEST_DIR is it gathers cruft from tests so exercises
> filesystem aging. If the file is tiny, there's no point in removing
> it - all we have to do is ensure it is removed in the test setup so
> we have known initial state. So If that's the case, I removed the
> cleanup of it altogether.
>
> Like I said, I have time to do a single classification pass across
> all these tests, but if I have to document every single little
> change I make in doing these cleanups, I'm not going to bother
> trying because it's too hard and takes too long.
>
> Do you want perfect commits or do you want better infrastructure?
I want better infrastructure.
>
> > Also, it was very hard to see in this review if the test files
> > that are not cleaned in cleanup() are cleaned in the beginning
> > of the test or if it is not important to clean them because they
> > are overwritten.
>
> All tests are required to control their initial state on the test
> device as many tests use the same file names and locations and there
> is no guarantee that the test device is clean. Hence tests must
> clean up during startup to ensure they have a known initial state
> to begin the test from.
>
> > I did my best to try and I think there is a missing cleanup of
> > ${OUTPUT_DIR} in the beginning of xfs/253.
> > Although that may already be considered a test bug, removing
> > the cleanup of ${OUTPUT_DIR} from cleanup() will expose this bug.
>
> OUTPUT_DIR="${SCRATCH_MNT}/test_${seq}"
>
> No cleanup is needed for tests that use the scratch device.
>
Ah, I missed that.
> > > diff --git a/tests/xfs/051 b/tests/xfs/051
> > > index ea70cb50..4718099d 100755
> > > --- a/tests/xfs/051
> > > +++ b/tests/xfs/051
> > > @@ -11,14 +11,13 @@
> > > . ./common/preamble
> > > _begin_fstest shutdown auto log metadata
> > >
> > > -# Override the default cleanup function.
> > > -_cleanup()
> > > +_fsstress_cleanup()
> > > {
> > > - cd /
> > > - rm -f $tmp.*
> > > $KILLALL_PROG -9 $FSSTRESS_PROG > /dev/null 2>&1
> > > - _scratch_unmount > /dev/null 2>&1
> > > + wait
> >
> > All right. I see that cleanup helpers are not consistent
> > in calling wait after kill and in using SIGKILL.
> > I guess we could go either way. So be it.
>
> That's the whole point of these cleanups - they will be collapsed
> down to a single helper and then we can apply the same behaviour
> consistently across them all.
Excellent.
>
> > > diff --git a/tests/xfs/310 b/tests/xfs/310
> > > index 3214e04b..c635b39a 100755
> > > --- a/tests/xfs/310
> > > +++ b/tests/xfs/310
> > > @@ -9,14 +9,12 @@
> > > . ./common/preamble
> > > _begin_fstest auto clone rmap
> > >
> > > -# Override the default cleanup function.
> > > -_cleanup()
> > > +_dmhuge_cleanup()
> > > {
> > > - cd /
> > > - umount $SCRATCH_MNT > /dev/null 2>&1
> > > _dmhugedisk_cleanup
> > > - rm -rf $tmp.*
> > > + _cleanup
> > > }
> > > +_register_cleanup _dmhuge_cleanup
> >
> > Maybe it's just me, but if the intention is to maybe make this
> > function global someday then the difference with the name
> > _dmhugedisk_cleanup() is confusing.
>
> The names I used here are for classification, so I can grep over
> the _register_cleanup line and easily determine all the tests use
> loop devices, dmflakey, dmerror, etc and then consolidate them all
> into a single cleanup function that works for them all. This is just
> the first step in that process.
>
> > I'd rather keep the local function name local_cleanup
> > in unclear cases like this one.
>
> And that defeats the entire purpose of giving them a descriptive
> name at this point in time.
I understand. It makes a lot of sense to do that.
It is still VERY confusing that there is a helper _dmhugedisk_cleanup
that SHOULD NOT be used as a trap and a helper _dmhuge_cleanup
that SHOULD be used as a trap, so a better convention is needed.
I suggest that all consolidated helpers for trap will be called
either _cleanup_* (like cleanup_dump) or _*_cleanup (like _fsstress_cleanup)
The problem is that there is no convetion in the common/* helpers.
there are _dmhugedisk_cleanup() _dmerror_cleanup() and there are
_cleanup_dump() _cleanup_delay().
Since you already converted _cleanup_dump() to be a trap trigger itself
it looks like the easier way to go is to have _cleanup_* as the convention
to trap callbacks, which seems like the more clear choice to me.
Hence in this patch that would be _cleanup_dmhugedisk.
Thanks,
Amir.
