On Thu, Sep 21, 2023 at 7:23 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> 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
This is not needed.
Overlayfs (on $SCRATCH_MNT) will not pass the require_chattr
check if the base fs does not support chattr.
> _require_chattr A $SCRATCH_MNT
This is practically equivalent to _require_chattr A
on the test partition, there is no reason to test the
scratch partition specifically.
So there is no need for the proposed change in _require_chattr.
>
> 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 :)
V1 is good enough for me as is. :)
You can take the commit message and test
header comment fixes from V2.
Note that the common _require_chattr_inheritance in V2
almost did not remove any lines from the test at all -
it moved one _notrun line into _require_chattr_inheritance
and turned another _notrun into echo "fail".
So I agree that if no other test is going to use the new helpers
their value is limited.
Thanks,
Amir.
