On Tue, May 24, 2022 at 4:24 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Tue, May 24, 2022 at 03:55:47PM +0300, Amir Goldstein wrote:
> > > > > 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 think you missed my point.
>
> _dmhuge_cleanup() is a *temporary name*.
>
> It will *go away* once I've processed the other 1000 tests which
> will also classify anything using dmhugedisk with a _dmhuge_cleanup
> function. It's a temporary token I can use for further
> classification and deduplication, nothing more, nothing less.
>
Ok if the two confusing names are not going to co-exists I'm fine
with either name.
> > 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().
>
> Exactly the reason why I've used simple names and the only
> consistency I care about is that I use the same name for the same
> subsystem cleanup functions. I can't give them globally consistent
> names at this point and because they are temporary tokens I simply
> have not tried. I give it a name, copy the function into a register
> buffer, and then paste it into any other test that uses the same
> subsystem and has the same cleanup requirements.
>
> Once I have all the tests that use dmhugedisk converted to use a
> local _dmhuge_cleanup function, I'll derive the commonality from
> them, move it into the existing cleanup function for that subsystem,
> and register the subsystem cleanup in those tests. _dmhuge_cleanup()
> then goes away forever.
>
> Once everythign is deduplicated and common, changing names will be
> simple and straight forward. That's when you can argue all you want
> over the names of functions and I won't care one bit.
>
All right, please bear in mind that I try to do this review with as little
nit picking as I can while still doing a professional review, so some
questions need to be raised.
The work you did is huge and impressive and the review is not easy.
Anyway, after fixing the recursive _dmerror_cleanup typo, you may add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks,
Amir.
