On Thu, Sep 21, 2023 at 07:46:12PM +0300, Amir Goldstein wrote:
> 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.
Oh, I saw you wrote as this:
+# prepare lower test dir with NOATIME flag
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+mkdir -p $lowerdir/testdir
+$CHATTR_PROG +A $lowerdir/testdir >> $seqres.full 2>&1 ||
+ _notrun "base fs $OVL_BASE_FSTYP does not support No_Atime flag"
so I thought you might want a `_require_chattr A $BASE_SCRATCH_MNT`.
>
> > _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.
I'll merge v2 patch 2/2, with ...
1) Change _require_chattr_inherit to _require_chattr.
2) Add `sleep 2s` under the "before=$(stat -c %x $lowerdir/testdir/lnk)" line.
Thanks,
Zorro
>
> 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.
>
