On Thu, Sep 21, 2023 at 8:06 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> 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`.
>
You are right. it should have been an error, not _notrun
as it is in V2.
> >
> > > _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.
>
OK.
Thanks!
Amir.
