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




[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