On Wed, May 25, 2022 at 01:13:36AM +0800, Zorro Lang wrote:
> 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.
Hi Dave,
Are you going to fix this issue or making more changes on this patchset?
If not, I'll merge this patchset tomorrow (Friday) by changing above
_dmerror_cleanup to _dm_error_cleanup. Then give merged patches a regression
on Saturday. What do you think?
Thanks,
Zorro
>
> 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
> > >
> >
