On Tue, May 24, 2022 at 01:42:15PM +0300, Amir Goldstein wrote:
> On Tue, May 24, 2022 at 12:41 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Use a local cleanup function and _register_cleanup properly.
> >
> > Remove local cleanups that just do what the standard _cleanup and
> > test exist does.
> >
> > Remove local cleanups that just remove test files and then do
> > standard _cleanup functionality.
>
> As I mentioned in response to the cover letter, the call to _cleanup()
> can and I think should be chained implicitly, but I am still waiting
> for your response regarding the tests where generic _cleanup is not
> desired.
>
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > tests/xfs/004 | 7 -------
> > tests/xfs/006 | 7 ++++---
[snap]
> > diff --git a/tests/xfs/006 b/tests/xfs/006
> > index cecceaa3..fd8d8071 100755
> > --- a/tests/xfs/006
> > +++ b/tests/xfs/006
> > @@ -11,11 +11,10 @@
> > _begin_fstest auto quick mount eio
> >
> > # Override the default cleanup function.
> > -_cleanup()
> > +_dmerror_cleanup()
> > {
> > - cd /
> > - rm -f $tmp.*
> > _dmerror_cleanup
>
> That looks recursive.
Yes, but this _dmerror_cleanup() won't be run. This function will be covered by
the _dmerror_cleanup() in common/dmerror, when this case ". ./common/dmerror"
several lines later. So the later _cleanup won't be run. So this place and other
places similar with this need to change.
Thanks,
Zorro
> Again, if the call to _cleanup is implicitly chained then
> the local function is not needed and trap can call
> the global _dmerror_cleanup().
>
> > + _cleanup
> > }
> >
> > # Import common functions.
> > @@ -28,6 +27,8 @@ _require_scratch
> > _require_dm_target error
> > _require_fs_sysfs error/fail_at_unmount
> >
> > +_register_cleanup _dmerror_cleanup
> > +
> > _scratch_mkfs > $seqres.full 2>&1
> > _dmerror_init
> > _dmerror_mount
> > 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.
>
> 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.
>
> 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.
>
> [...]
>
> > 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.
>
> [...]
>
> > 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.
>
> I'd rather keep the local function name local_cleanup
> in unclear cases like this one.
>
> Thanks,
> Amir.
>
> > }
> > +_register_cleanup local_cleanup
> >
> > # Import common functions.
> > . ./common/filter
> > --
> > 2.35.1
> >
>
