On Thu, Sep 21, 2023 at 06:40:49PM +0300, Amir Goldstein wrote:
> On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > > Similar to _require_chattr, but also checks if an attribute is
> > > inheritted from parent dir to children.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > > common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 43 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index 1618ded5..00cfd434 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> > > _notrun "lsattr not supported by test filesystem type: $FSTYP"
> > > }
> > >
> > > +_check_chattr_inherit()
> > > +{
> > > + local attribute=$1
> > > + local path=$2
> > > + local inherit=$3
> >
> > As I understand, this function calls _check_chattr_inherit, so it will
> > return zero or non-zero to clarify if $path support $attribute inheritance.
> > ...
> >
> > > +
> > > + touch $path
> > > + $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > > + local ret=$?
> > > + if [ -n "$inherit" ]; then
> > > + touch "$path/$inherit"
> > > + fi
> >
> > ... but looks like it doesn't, it only create a $inherit file, then let the
> > caller check if the $attribute is inherited.
> >
> > I think that's a little confused.
>
> I agree.
>
> > I think we can name the function as _check_chattr()
>
> I agree.
>
> > and the 3rd argument $inherit as a bool variable, to
> > decide if we check inheritance or not.
> >
>
> Not my prefered choice.
>
> > Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> > _check_chattr_inherit calls _check_chattr then keep checking inheritance.
> >
> > What do you think?
>
> I think this is over engineering for a helper that may not
> be ever used by any other test.
>
> Suggest to just change the name to _check_chattr()
> to match the meaning to the return value.
>
> The 3rd inherit argument just means that we request
> to create a file after chattr + and before chattr -,
> so that the caller could check it itself later.
I still think it doesn't make sense ... but I don't want to give you
that pressure, so ...
>
> If you accept this minor change is enough
> can you apply it yourself on commit?
... If you think it's too complicated, we can drop the inheritance checking
common helper. Just change the _require_chattr(), make it to accept one more
*directory* argument (use $TEST_DIR by default). Then we can do:
_require_chattr A $BASE_SCRATCH_MNT
_require_chattr A $SCRATCH_MNT
And then do all inheritance checking in the overlay case itself (likes you did in
V1), don't make them to be a common helper. Due to only this case need the
_require_chattr_inheritance, so we can think about that common helper when more
cases need that :)
I think this change is simple enough (base on your V1 patch). Is that good to
you :)
Thanks,
Zorro
>
> Thanks,
> Amir.
>
