| Message ID | 20230921143102.127526-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Test for overlayfs fix in v6.6-rc2 | expand |
On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> 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@gmail.com> > > --- > > 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.
On Thu, Sep 21, 2023 at 7:23 PM Zorro Lang <zlang@redhat.com> 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@redhat.com> 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@gmail.com> > > > > --- > > > > 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.
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@redhat.com> 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@redhat.com> 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@gmail.com> > > > > > --- > > > > > 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. >
On Thu, Sep 21, 2023 at 8:06 PM Zorro Lang <zlang@redhat.com> 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@redhat.com> 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@redhat.com> 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@gmail.com> > > > > > > --- > > > > > > 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.