On Tue, May 24, 2022 at 05:17:44PM +0300, Amir Goldstein wrote:
> 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.
Thanks Dave and Amir!
Dave did a huge and impressive change again:) The purpose of this change makes
sense to me, so there's not objections from me. But it really changed too many
cases, and are going to change more. So if anyone has any concern, please show
your review points as early as better. Thanks.
Really appreciate Amir's help, I was bothered by COVID-19 issues during the day,
just got time to read emails. You did give it a careful review line by line,
and asked lots of questions (than I thought) :) That helps a lot! Compare with
"it's fine but not perfect", I care about "it won't break any cases" more.
So if Dave can force on his main work, keep the code stable, we can do later
improvement bit by bit. As you can see, this patchset nearly affect the
whole fstests, more time we take to change it, more patch conflicts might
happen. Then Dave might need to do more 'rebase' jobs.
I'll give it lots of testing (although I'm sure Dave did test) before merging.
I'll try to help it catch the fstests release of this week. But that depends
on when we have the lastest version, and how stable this patchset is :)
Thanks,
Zorro
>
> Anyway, after fixing the recursive _dmerror_cleanup typo, you may add:
>
> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Thanks,
> Amir.
>
