Re: [PATCH 3/8] xfs/*: clean up _cleanup override

[Zorro Lang <zlang@redhat.com>]

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@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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@redhat.com>
> > ---
> >  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
> >
>