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.
If you accept this minor change is enough
can you apply it yourself on commit?
Thanks,
Amir.
