Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]



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.




[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux