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

From: Dave Chinner <david@fromorbit.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: fstests <fstests@vger.kernel.org>
Subject: Re: [PATCH 3/8] xfs/*: clean up _cleanup override
Date: Tue, 24 May 2022 23:24:28 +1000	[thread overview]
Message-ID: <20220524132428.GO2306852@dread.disaster.area> (raw)
In-Reply-To: <CAOQ4uxifd8gee1JLG1FN9fF3aSsVeSEuxFqjAf4D8ruCciXWYg@mail.gmail.com>

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.

> 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.

-Dave.
-- 
Dave Chinner
david@fromorbit.com