xfstests: add specific test for default ACL inheritance
diff mbox

Message ID 1381932296-14674-1-git-send-email-fdmanana@gmail.com
State Not Applicable
Headers show

Commit Message

Filipe Manana Oct. 16, 2013, 2:04 p.m. UTC
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(-)

Comments

Eric Sandeen Oct. 16, 2013, 3:10 p.m. UTC | #1
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
Filipe Manana Oct. 16, 2013, 3:14 p.m. UTC | #2
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
>
Eric Sandeen Oct. 16, 2013, 3:19 p.m. UTC | #3
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
Christoph Hellwig Oct. 16, 2013, 4:11 p.m. UTC | #4
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
Dave Chinner Oct. 16, 2013, 9:51 p.m. UTC | #5
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.

Patch
diff mbox

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::---
+