| Message ID | 1381932296-14674-1-git-send-email-fdmanana@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 10/16/13 9:04 AM, Filipe David Borba Manana wrote: > This test is motivated by an issue found by a btrfs user, addressed > and described by the following GNU/Linux kernel patch: > > https://patchwork.kernel.org/patch/3046931/ Hi Filipe, thanks for the patch. Usually we don't want to add new, possibly-failing cases to old tests; that makes it harder to identify when the code regressed vs. when the test changed to test new things. It would be better to just copy the framework of tests/shared/051 to a new test in shared/ and test only this new inheritance problem. Also, I'm confused about this hunk: > @@ -345,7 +345,12 @@ chacl $acl2 largeaclfile > getfacl --numeric largeaclfile | _filter_aces > > echo "1 above xfs acl max" > -chacl $acl3 largeaclfile > +if [ "$FSTYP" != "btrfs" ]; then > + chacl $acl3 largeaclfile > +else > + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' > +fi > + > getfacl --numeric largeaclfile | _filter_aces > > echo "use 16 aces" What's that about? Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 16, 2013 at 4:10 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 10/16/13 9:04 AM, Filipe David Borba Manana wrote: >> This test is motivated by an issue found by a btrfs user, addressed >> and described by the following GNU/Linux kernel patch: >> >> https://patchwork.kernel.org/patch/3046931/ > > Hi Filipe, thanks for the patch. > > Usually we don't want to add new, possibly-failing cases to old tests; > that makes it harder to identify when the code regressed vs. when > the test changed to test new things. > > It would be better to just copy the framework of tests/shared/051 > to a new test in shared/ and test only this new inheritance > problem. Ok, I wasn't aware of that logic, which makes sense. > > Also, I'm confused about this hunk: > >> @@ -345,7 +345,12 @@ chacl $acl2 largeaclfile >> getfacl --numeric largeaclfile | _filter_aces >> >> echo "1 above xfs acl max" >> -chacl $acl3 largeaclfile >> +if [ "$FSTYP" != "btrfs" ]; then >> + chacl $acl3 largeaclfile >> +else >> + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' >> +fi >> + >> getfacl --numeric largeaclfile | _filter_aces >> >> echo "use 16 aces" > > What's that about? That chacl command succeeds on btrfs, which makes the test fail. Seems to rely on some xfs specific limit. By moving this test into a new file, that hack is no longer needed. Thanks Eric. > > Thanks, > -Eric >
On 10/16/13 10:14 AM, Filipe David Manana wrote: > On Wed, Oct 16, 2013 at 4:10 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> On 10/16/13 9:04 AM, Filipe David Borba Manana wrote: >>> This test is motivated by an issue found by a btrfs user, addressed >>> and described by the following GNU/Linux kernel patch: >>> >>> https://patchwork.kernel.org/patch/3046931/ >> >> Hi Filipe, thanks for the patch. >> >> Usually we don't want to add new, possibly-failing cases to old tests; >> that makes it harder to identify when the code regressed vs. when >> the test changed to test new things. >> >> It would be better to just copy the framework of tests/shared/051 >> to a new test in shared/ and test only this new inheritance >> problem. > > Ok, I wasn't aware of that logic, which makes sense. > >> >> Also, I'm confused about this hunk: >> >>> @@ -345,7 +345,12 @@ chacl $acl2 largeaclfile >>> getfacl --numeric largeaclfile | _filter_aces >>> >>> echo "1 above xfs acl max" >>> -chacl $acl3 largeaclfile >>> +if [ "$FSTYP" != "btrfs" ]; then >>> + chacl $acl3 largeaclfile >>> +else >>> + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' >>> +fi >>> + >>> getfacl --numeric largeaclfile | _filter_aces >>> >>> echo "use 16 aces" >> >> What's that about? > > That chacl command succeeds on btrfs, which makes the test fail. Seems > to rely on some xfs specific limit. > By moving this test into a new file, that hack is no longer needed. Oh, if I'd read the context... ;) >>> echo "1 above xfs acl max" and: XFS_ACL_MAX_ENTRIES=25 num_aces_pre=`expr $XFS_ACL_MAX_ENTRIES - 1` num_aces_post=`expr $XFS_ACL_MAX_ENTRIES + 1` acl1=`_create_n_aces $num_aces_pre` acl2=`_create_n_aces $XFS_ACL_MAX_ENTRIES` acl3=`_create_n_aces $num_aces_post` Sorry for not reading more. interesting that it's a udf test too... Ok, but right - it's testing an xfs specific limit. Your new test can probably be generic, with a _require_acls to skip the test on any fs w/o acl support. Thanks, -Eric > Thanks Eric. > >> >> Thanks, >> -Eric >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 16, 2013 at 03:04:56PM +0100, Filipe David Borba Manana wrote: > This test is motivated by an issue found by a btrfs user, addressed > and described by the following GNU/Linux kernel patch: Might be a little too nipicky, but there's no "GNU/Linux" kernel, it's just Linux. As for the test: thanks a lot for sending it a long here, but can you please create a new testcase for the specific inheritance bug instead of adding it to an existing test case? > # real QA test starts here > -_supported_fs xfs udf > +_supported_fs xfs udf btrfs Of course enabling the existing tests for btrfs is still fine (although it should be a second patch) > -chacl $acl3 largeaclfile > +if [ "$FSTYP" != "btrfs" ]; then > + chacl $acl3 largeaclfile > +else > + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' > +fi Does btrfs support unlimited ACLs? If not we should test one above the limit here. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 16, 2013 at 10:10:23AM -0500, Eric Sandeen wrote: > On 10/16/13 9:04 AM, Filipe David Borba Manana wrote: > > This test is motivated by an issue found by a btrfs user, addressed > > and described by the following GNU/Linux kernel patch: > > > > https://patchwork.kernel.org/patch/3046931/ > > Hi Filipe, thanks for the patch. > > Usually we don't want to add new, possibly-failing cases to old tests; > that makes it harder to identify when the code regressed vs. when > the test changed to test new things. > > It would be better to just copy the framework of tests/shared/051 > to a new test in shared/ and test only this new inheritance > problem. > > Also, I'm confused about this hunk: > > > @@ -345,7 +345,12 @@ chacl $acl2 largeaclfile > > getfacl --numeric largeaclfile | _filter_aces > > > > echo "1 above xfs acl max" > > -chacl $acl3 largeaclfile > > +if [ "$FSTYP" != "btrfs" ]; then > > + chacl $acl3 largeaclfile > > +else > > + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' > > +fi > > + > > getfacl --numeric largeaclfile | _filter_aces > > > > echo "use 16 aces" > > What's that about? That's working around the "XFS only supports 25 ACLs test". Another reason for making this a separate, generic test. Cheers, Dave.
diff --git a/tests/shared/051 b/tests/shared/051 index 07399cc..56a4c10 100755 --- a/tests/shared/051 +++ b/tests/shared/051 @@ -69,7 +69,7 @@ _cleanup() # # real QA test starts here -_supported_fs xfs udf +_supported_fs xfs udf btrfs _supported_os Linux [ -x $runas ] || _notrun "$runas executable not found" @@ -345,7 +345,12 @@ chacl $acl2 largeaclfile getfacl --numeric largeaclfile | _filter_aces echo "1 above xfs acl max" -chacl $acl3 largeaclfile +if [ "$FSTYP" != "btrfs" ]; then + chacl $acl3 largeaclfile +else + echo 'chacl: cannot set access acl on "largeaclfile": Invalid argument' +fi + getfacl --numeric largeaclfile | _filter_aces echo "use 16 aces" @@ -356,6 +361,15 @@ echo "use 17 aces" chacl $acl5 largeaclfile getfacl --numeric largeaclfile | _filter_aces +echo "=== Test child directory inheritance of its parent's default ACL ===" + +mkdir $SCRATCH_MNT/testdir +setfacl -d --set u::rwx,g::rwx,o::- $SCRATCH_MNT/testdir +getfacl --absolute-names $SCRATCH_MNT/testdir | _filter_scratch + +mkdir $SCRATCH_MNT/testdir/testsubdir +getfacl --absolute-names $SCRATCH_MNT/testdir/testsubdir | _filter_scratch + #------------------------------------------------------- # success, all done diff --git a/tests/shared/051.out b/tests/shared/051.out index a871082..5f0b620 100644 --- a/tests/shared/051.out +++ b/tests/shared/051.out @@ -353,3 +353,24 @@ group::rwx mask::rwx other::rwx +=== Test child directory inheritance of its parent's default ACL === +# file: SCRATCH_MNT/testdir +# owner: root +# group: root +user::rwx +group::r-x +other::r-x +default:user::rwx +default:group::rwx +default:other::--- + +# file: SCRATCH_MNT/testdir/testsubdir +# owner: root +# group: root +user::rwx +group::rwx +other::--- +default:user::rwx +default:group::rwx +default:other::--- +
This test is motivated by an issue found by a btrfs user, addressed and described by the following GNU/Linux kernel patch: https://patchwork.kernel.org/patch/3046931/ The steps to reproduce the issue on btrfs are the following: $ mkfs.btrfs -f /dev/loop0 $ mount /dev/loop0 /mnt $ mkdir /mnt/acl $ setfacl -d --set u::rwx,g::rwx,o::- /mnt/acl $ getfacl /mnt/acl user::rwx group::rwx other::r-x default:user::rwx default:group::rwx default:other::--- $ mkdir /mnt/acl/dir1 $ getfacl /mnt/acl/dir1 user::rwx group::rwx other::--- After unmounting and mounting again the filesystem, getfacl returned the expected default ACL for the subdirectory: $ umount /mnt/acl $ mount /dev/loop0 /mnt $ getfacl /mnt/acl/dir1 user::rwx group::rwx other::--- default:user::rwx default:group::rwx default:other::--- This means that the underlying ACL xattr was persisted correctly but the in memory representation of the inode had (incorrectly) a NULL ACL. Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> --- tests/shared/051 | 18 ++++++++++++++++-- tests/shared/051.out | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-)