* [PATCH] xfstests: add specific test for default ACL inheritance
@ 2013-10-16 14:04 Filipe David Borba Manana
2013-10-16 15:10 ` Eric Sandeen
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Filipe David Borba Manana @ 2013-10-16 14:04 UTC (permalink / raw)
To: linux-btrfs, xfs; +Cc: jbacik, dsterba, Filipe David Borba Manana
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(-)
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::---
+
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfstests: add specific test for default ACL inheritance
2013-10-16 14:04 [PATCH] xfstests: add specific test for default ACL inheritance Filipe David Borba Manana
@ 2013-10-16 15:10 ` Eric Sandeen
2013-10-16 15:14 ` Filipe David Manana
2013-10-16 21:51 ` Dave Chinner
2013-10-16 15:52 ` [PATCH v2] " Filipe David Borba Manana
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Eric Sandeen @ 2013-10-16 15:10 UTC (permalink / raw)
To: Filipe David Borba Manana; +Cc: linux-btrfs, xfs, dsterba, jbacik
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfstests: add specific test for default ACL inheritance
2013-10-16 15:10 ` Eric Sandeen
@ 2013-10-16 15:14 ` Filipe David Manana
2013-10-16 15:19 ` Eric Sandeen
2013-10-16 21:51 ` Dave Chinner
1 sibling, 1 reply; 13+ messages in thread
From: Filipe David Manana @ 2013-10-16 15:14 UTC (permalink / raw)
To: Eric Sandeen
Cc: linux-btrfs@vger.kernel.org, xfs, dsterba@suse.cz, Josef Bacik
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
>
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfstests: add specific test for default ACL inheritance
2013-10-16 15:14 ` Filipe David Manana
@ 2013-10-16 15:19 ` Eric Sandeen
0 siblings, 0 replies; 13+ messages in thread
From: Eric Sandeen @ 2013-10-16 15:19 UTC (permalink / raw)
To: fdmanana; +Cc: Josef Bacik, dsterba@suse.cz, linux-btrfs@vger.kernel.org, xfs
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
>>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] xfstests: add specific test for default ACL inheritance
2013-10-16 14:04 [PATCH] xfstests: add specific test for default ACL inheritance Filipe David Borba Manana
2013-10-16 15:10 ` Eric Sandeen
@ 2013-10-16 15:52 ` Filipe David Borba Manana
2013-10-16 16:09 ` Eric Sandeen
2013-10-16 16:11 ` [PATCH] " Christoph Hellwig
2013-10-16 16:25 ` [PATCH v3] " Filipe David Borba Manana
3 siblings, 1 reply; 13+ messages in thread
From: Filipe David Borba Manana @ 2013-10-16 15:52 UTC (permalink / raw)
To: linux-btrfs, xfs; +Cc: jbacik, dsterba, sandeen, Filipe David Borba Manana
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>
---
V2: Moved the regression test into a dedicated and new file, as suggested
by Eric Sandeen.
tests/shared/052 | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
tests/shared/052.out | 21 +++++++++++++++
tests/shared/group | 1 +
3 files changed, 92 insertions(+)
create mode 100755 tests/shared/052
create mode 100644 tests/shared/052.out
diff --git a/tests/shared/052 b/tests/shared/052
new file mode 100755
index 0000000..ee08eda
--- /dev/null
+++ b/tests/shared/052
@@ -0,0 +1,70 @@
+#! /bin/bash
+# FS QA Test No. shared/052
+#
+# Regression test to make sure a directory inherits the default ACL from
+# its parent directory. This test was motivated by an issue reported by
+# a btrfs user. That issue is fixed and described by the following btrfs
+# kernel patch:
+#
+# https://patchwork.kernel.org/patch/3046931/
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2013 Filipe Manana. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+#
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # FAILure is the default!
+
+_cleanup()
+{
+ rm -f $tmp.*
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+_supported_os Linux
+_require_acls
+_require_scratch
+_need_to_be_root
+
+rm -f $seqres.full
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount
+
+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
+status=0
+exit
diff --git a/tests/shared/052.out b/tests/shared/052.out
new file mode 100644
index 0000000..d453175
--- /dev/null
+++ b/tests/shared/052.out
@@ -0,0 +1,21 @@
+QA output created by 052
+# 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::---
+
diff --git a/tests/shared/group b/tests/shared/group
index 0ad640b..91cb049 100644
--- a/tests/shared/group
+++ b/tests/shared/group
@@ -5,6 +5,7 @@
#
032 mkfs auto quick
051 acl udf auto quick
+052 acl auto quick
218 auto fsr quick
243 auto quick prealloc
272 auto enospc rw
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfstests: add specific test for default ACL inheritance
2013-10-16 15:52 ` [PATCH v2] " Filipe David Borba Manana
@ 2013-10-16 16:09 ` Eric Sandeen
2013-10-16 16:11 ` Filipe David Manana
0 siblings, 1 reply; 13+ messages in thread
From: Eric Sandeen @ 2013-10-16 16:09 UTC (permalink / raw)
To: Filipe David Borba Manana; +Cc: linux-btrfs, xfs, jbacik, dsterba
On 10/16/13 10:52 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/
>
> 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>
> ---
>
> V2: Moved the regression test into a dedicated and new file, as suggested
> by Eric Sandeen.
Great, thanks. Verified that it succeeds on xfs & ext3 as well.
It also fails properly when mounting ext3 -o noacl:
shared/052 1s ... [not run] ACLs not supported by this filesystem type: ext3
...
> +# real QA test starts here
> +_supported_os Linux
Technically this should have a:
+_supported_fs generic
here. And then it can move to tests/generic/xxx
(I guess that's a little odd and redundant, and it does
run today w/o the _supported_fs, I guess, but still
best to be consistent).
Sorry for the runaround :)
If you don't mind a V3, we'll be done, I think!
-Eric
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfstests: add specific test for default ACL inheritance
2013-10-16 16:09 ` Eric Sandeen
@ 2013-10-16 16:11 ` Filipe David Manana
2013-10-16 16:14 ` Eric Sandeen
0 siblings, 1 reply; 13+ messages in thread
From: Filipe David Manana @ 2013-10-16 16:11 UTC (permalink / raw)
To: Eric Sandeen
Cc: linux-btrfs@vger.kernel.org, xfs, Josef Bacik, dsterba@suse.cz
On Wed, Oct 16, 2013 at 5:09 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 10/16/13 10:52 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/
>>
>> 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>
>> ---
>>
>> V2: Moved the regression test into a dedicated and new file, as suggested
>> by Eric Sandeen.
>
> Great, thanks. Verified that it succeeds on xfs & ext3 as well.
>
> It also fails properly when mounting ext3 -o noacl:
>
> shared/052 1s ... [not run] ACLs not supported by this filesystem type: ext3
>
> ...
>
>> +# real QA test starts here
>> +_supported_os Linux
>
> Technically this should have a:
>
> +_supported_fs generic
>
> here. And then it can move to tests/generic/xxx
>
> (I guess that's a little odd and redundant, and it does
> run today w/o the _supported_fs, I guess, but still
> best to be consistent).
>
> Sorry for the runaround :)
>
> If you don't mind a V3, we'll be done, I think!
Np.
Is there any rule as for which name (number) to pick for the test case
file name?
>
> -Eric
>
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfstests: add specific test for default ACL inheritance
2013-10-16 14:04 [PATCH] xfstests: add specific test for default ACL inheritance Filipe David Borba Manana
2013-10-16 15:10 ` Eric Sandeen
2013-10-16 15:52 ` [PATCH v2] " Filipe David Borba Manana
@ 2013-10-16 16:11 ` Christoph Hellwig
2013-10-16 16:25 ` [PATCH v3] " Filipe David Borba Manana
3 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2013-10-16 16:11 UTC (permalink / raw)
To: Filipe David Borba Manana; +Cc: linux-btrfs, xfs, dsterba, jbacik
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfstests: add specific test for default ACL inheritance
2013-10-16 16:11 ` Filipe David Manana
@ 2013-10-16 16:14 ` Eric Sandeen
0 siblings, 0 replies; 13+ messages in thread
From: Eric Sandeen @ 2013-10-16 16:14 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs@vger.kernel.org, xfs, Josef Bacik, dsterba@suse.cz
On 10/16/13 11:11 AM, Filipe David Manana wrote:
> On Wed, Oct 16, 2013 at 5:09 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> On 10/16/13 10:52 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/
>>>
>>> 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>
>>> ---
>>>
>>> V2: Moved the regression test into a dedicated and new file, as suggested
>>> by Eric Sandeen.
>>
>> Great, thanks. Verified that it succeeds on xfs & ext3 as well.
>>
>> It also fails properly when mounting ext3 -o noacl:
>>
>> shared/052 1s ... [not run] ACLs not supported by this filesystem type: ext3
>>
>> ...
>>
>>> +# real QA test starts here
>>> +_supported_os Linux
>>
>> Technically this should have a:
>>
>> +_supported_fs generic
>>
>> here. And then it can move to tests/generic/xxx
>>
>> (I guess that's a little odd and redundant, and it does
>> run today w/o the _supported_fs, I guess, but still
>> best to be consistent).
>>
>> Sorry for the runaround :)
>>
>> If you don't mind a V3, we'll be done, I think!
>
> Np.
> Is there any rule as for which name (number) to pick for the test case
> file name?
just pick a free slot. SGI is behind on merging, so they may need to move
it to avoid a conflict. Wish we had a little better way to do this...
hch just chimed in, maybe we can tweak the original 051 test to do the
same testing on other filesystems, if we can set the appropriate max
acl counts... but that's another patch.
-Eric
>>
>> -Eric
>>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] xfstests: add specific test for default ACL inheritance
2013-10-16 14:04 [PATCH] xfstests: add specific test for default ACL inheritance Filipe David Borba Manana
` (2 preceding siblings ...)
2013-10-16 16:11 ` [PATCH] " Christoph Hellwig
@ 2013-10-16 16:25 ` Filipe David Borba Manana
2013-10-16 16:30 ` Eric Sandeen
2013-10-16 21:24 ` Rich Johnston
3 siblings, 2 replies; 13+ messages in thread
From: Filipe David Borba Manana @ 2013-10-16 16:25 UTC (permalink / raw)
To: linux-btrfs, xfs; +Cc: jbacik, dsterba, sandeen, Filipe David Borba Manana
This test is motivated by an issue found by a btrfs user, addressed
and described by the following 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>
---
V2: Moved the regression test into a dedicated and new file, as suggested
by Eric Sandeen.
V3: Moved the test to the generic group and added "_supported_fs generic",
as suggested by Eric Sandeen. Also replaced GNU/Linux with Linux (hope
rms doesn't get mad at me).
tests/generic/106 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
tests/generic/106.out | 21 +++++++++++++++
tests/generic/group | 1 +
3 files changed, 93 insertions(+)
create mode 100755 tests/generic/106
create mode 100644 tests/generic/106.out
diff --git a/tests/generic/106 b/tests/generic/106
new file mode 100755
index 0000000..76cea80
--- /dev/null
+++ b/tests/generic/106
@@ -0,0 +1,71 @@
+#! /bin/bash
+# FS QA Test No. generic/106
+#
+# Regression test to make sure a directory inherits the default ACL from
+# its parent directory. This test was motivated by an issue reported by
+# a btrfs user. That issue is fixed and described by the following btrfs
+# kernel patch:
+#
+# https://patchwork.kernel.org/patch/3046931/
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2013 Filipe Manana. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+#
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # FAILure is the default!
+
+_cleanup()
+{
+ rm -f $tmp.*
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+_supported_os Linux
+_supported_fs generic
+_require_acls
+_require_scratch
+_need_to_be_root
+
+rm -f $seqres.full
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount
+
+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
+status=0
+exit
diff --git a/tests/generic/106.out b/tests/generic/106.out
new file mode 100644
index 0000000..5755cf9
--- /dev/null
+++ b/tests/generic/106.out
@@ -0,0 +1,21 @@
+QA output created by 106
+# 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::---
+
diff --git a/tests/generic/group b/tests/generic/group
index 1aee03c..e93233a 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -33,6 +33,7 @@
099 udf auto
100 udf auto
105 acl auto quick
+106 acl auto quick
112 rw aio auto quick
113 rw aio auto quick
117 attr auto quick
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] xfstests: add specific test for default ACL inheritance
2013-10-16 16:25 ` [PATCH v3] " Filipe David Borba Manana
@ 2013-10-16 16:30 ` Eric Sandeen
2013-10-16 21:24 ` Rich Johnston
1 sibling, 0 replies; 13+ messages in thread
From: Eric Sandeen @ 2013-10-16 16:30 UTC (permalink / raw)
To: Filipe David Borba Manana; +Cc: linux-btrfs, xfs, jbacik, dsterba
On 10/16/13 11:25 AM, Filipe David Borba Manana wrote:
> This test is motivated by an issue found by a btrfs user, addressed
> and described by the following Linux kernel patch:
>
> https://patchwork.kernel.org/patch/3046931/
>
> The steps to reproduce the issue on btrfs are the following:
Thanks!
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> $ 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>
> ---
>
> V2: Moved the regression test into a dedicated and new file, as suggested
> by Eric Sandeen.
> V3: Moved the test to the generic group and added "_supported_fs generic",
> as suggested by Eric Sandeen. Also replaced GNU/Linux with Linux (hope
> rms doesn't get mad at me).
>
> tests/generic/106 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/106.out | 21 +++++++++++++++
> tests/generic/group | 1 +
> 3 files changed, 93 insertions(+)
> create mode 100755 tests/generic/106
> create mode 100644 tests/generic/106.out
>
> diff --git a/tests/generic/106 b/tests/generic/106
> new file mode 100755
> index 0000000..76cea80
> --- /dev/null
> +++ b/tests/generic/106
> @@ -0,0 +1,71 @@
> +#! /bin/bash
> +# FS QA Test No. generic/106
> +#
> +# Regression test to make sure a directory inherits the default ACL from
> +# its parent directory. This test was motivated by an issue reported by
> +# a btrfs user. That issue is fixed and described by the following btrfs
> +# kernel patch:
> +#
> +# https://patchwork.kernel.org/patch/3046931/
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2013 Filipe Manana. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # FAILure is the default!
> +
> +_cleanup()
> +{
> + rm -f $tmp.*
> +}
> +
> +trap "_cleanup ; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +_supported_os Linux
> +_supported_fs generic
> +_require_acls
> +_require_scratch
> +_need_to_be_root
> +
> +rm -f $seqres.full
> +
> +_scratch_mkfs > /dev/null 2>&1
> +_scratch_mount
> +
> +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
> +status=0
> +exit
> diff --git a/tests/generic/106.out b/tests/generic/106.out
> new file mode 100644
> index 0000000..5755cf9
> --- /dev/null
> +++ b/tests/generic/106.out
> @@ -0,0 +1,21 @@
> +QA output created by 106
> +# 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::---
> +
> diff --git a/tests/generic/group b/tests/generic/group
> index 1aee03c..e93233a 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -33,6 +33,7 @@
> 099 udf auto
> 100 udf auto
> 105 acl auto quick
> +106 acl auto quick
> 112 rw aio auto quick
> 113 rw aio auto quick
> 117 attr auto quick
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] xfstests: add specific test for default ACL inheritance
2013-10-16 16:25 ` [PATCH v3] " Filipe David Borba Manana
2013-10-16 16:30 ` Eric Sandeen
@ 2013-10-16 21:24 ` Rich Johnston
1 sibling, 0 replies; 13+ messages in thread
From: Rich Johnston @ 2013-10-16 21:24 UTC (permalink / raw)
To: Filipe David Borba Manana, linux-btrfs, xfs; +Cc: sandeen, dsterba, jbacik
Thanks for the patch Filipe.
This has been committed.
Thanks
--Rich
commit 8bab8b31bb288b9d1b077ff8d06b6491715e8da7
Author: Filipe David Borba Manana <fdmanana@gmail.com>
Date: Wed Oct 16 16:25:18 2013 +0000
xfstests: add specific test for default ACL inheritance
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfstests: add specific test for default ACL inheritance
2013-10-16 15:10 ` Eric Sandeen
2013-10-16 15:14 ` Filipe David Manana
@ 2013-10-16 21:51 ` Dave Chinner
1 sibling, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2013-10-16 21:51 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Filipe David Borba Manana, jbacik, dsterba, linux-btrfs, xfs
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-10-16 21:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-16 14:04 [PATCH] xfstests: add specific test for default ACL inheritance Filipe David Borba Manana
2013-10-16 15:10 ` Eric Sandeen
2013-10-16 15:14 ` Filipe David Manana
2013-10-16 15:19 ` Eric Sandeen
2013-10-16 21:51 ` Dave Chinner
2013-10-16 15:52 ` [PATCH v2] " Filipe David Borba Manana
2013-10-16 16:09 ` Eric Sandeen
2013-10-16 16:11 ` Filipe David Manana
2013-10-16 16:14 ` Eric Sandeen
2013-10-16 16:11 ` [PATCH] " Christoph Hellwig
2013-10-16 16:25 ` [PATCH v3] " Filipe David Borba Manana
2013-10-16 16:30 ` Eric Sandeen
2013-10-16 21:24 ` Rich Johnston
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).