[PATCH v1] xfs/019: extend protofile test

[PATCH v1] xfs/019: extend protofile test

[Catherine Hoang]

This test creates an xfs filesystem and verifies that the filesystem
matches what is specified by the protofile.

This patch extends the current test to check that a protofile can specify
setgid mode on directories. Also, check that the created symlink isn’t
broken.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 tests/xfs/019     |  6 ++++++
 tests/xfs/019.out | 12 +++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/tests/xfs/019 b/tests/xfs/019
index 3dfd5408..535b7af1 100755
--- a/tests/xfs/019
+++ b/tests/xfs/019
@@ -73,6 +73,10 @@ $
 setuid -u-666 0 0 $tempfile
 setgid --g666 0 0 $tempfile
 setugid -ug666 0 0 $tempfile
+directory_setgid d-g755 3 2
+file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
+$
+: back in the root
 block_device b--012 3 1 161 162 
 char_device c--345 3 1 177 178
 pipe p--670 0 0
@@ -114,6 +118,8 @@ _verify_fs()
 		| xargs $here/src/lstat64 | _filter_stat)
 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
 		|| _fail "bigfile corrupted"
+	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
+		|| _fail "symlink broken"
 
 	echo "*** unmount FS"
 	_full "umount"
diff --git a/tests/xfs/019.out b/tests/xfs/019.out
index 19614d9d..8584f593 100644
--- a/tests/xfs/019.out
+++ b/tests/xfs/019.out
@@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
  File: "."
  Size: <DSIZE> Filetype: Directory
  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
-Device: <DEVICE> Inode: <INODE> Links: 3 
+Device: <DEVICE> Inode: <INODE> Links: 4 
 
  File: "./bigfile"
  Size: 2097152 Filetype: Regular File
@@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
 Device: <DEVICE> Inode: <INODE> Links: 1 
 
+ File: "./directory_setgid"
+ Size: <DSIZE> Filetype: Directory
+ Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
+Device: <DEVICE> Inode: <INODE> Links: 2 
+
+ File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
+ Size: 5 Filetype: Regular File
+ Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
+Device: <DEVICE> Inode: <INODE> Links: 1 
+
  File: "./pipe"
  Size: 0 Filetype: Fifo File
  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
-- 
2.25.1

Re: [PATCH v1] xfs/019: extend protofile test

[Zorro Lang]

On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
> This test creates an xfs filesystem and verifies that the filesystem
> matches what is specified by the protofile.
> 
> This patch extends the current test to check that a protofile can specify
> setgid mode on directories. Also, check that the created symlink isn’t
> broken.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---

Any specific reason to add this test? Likes uncovering some one known
bug/fix?

Thanks,
Zorro

>  tests/xfs/019     |  6 ++++++
>  tests/xfs/019.out | 12 +++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/xfs/019 b/tests/xfs/019
> index 3dfd5408..535b7af1 100755
> --- a/tests/xfs/019
> +++ b/tests/xfs/019
> @@ -73,6 +73,10 @@ $
>  setuid -u-666 0 0 $tempfile
>  setgid --g666 0 0 $tempfile
>  setugid -ug666 0 0 $tempfile
> +directory_setgid d-g755 3 2
> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
> +$
> +: back in the root
>  block_device b--012 3 1 161 162 
>  char_device c--345 3 1 177 178
>  pipe p--670 0 0
> @@ -114,6 +118,8 @@ _verify_fs()
>  		| xargs $here/src/lstat64 | _filter_stat)
>  	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
>  		|| _fail "bigfile corrupted"
> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> +		|| _fail "symlink broken"
>  
>  	echo "*** unmount FS"
>  	_full "umount"
> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> index 19614d9d..8584f593 100644
> --- a/tests/xfs/019.out
> +++ b/tests/xfs/019.out
> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
>   File: "."
>   Size: <DSIZE> Filetype: Directory
>   Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> -Device: <DEVICE> Inode: <INODE> Links: 3 
> +Device: <DEVICE> Inode: <INODE> Links: 4 
>  
>   File: "./bigfile"
>   Size: 2097152 Filetype: Regular File
> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
>   Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
>  Device: <DEVICE> Inode: <INODE> Links: 1 
>  
> + File: "./directory_setgid"
> + Size: <DSIZE> Filetype: Directory
> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> +Device: <DEVICE> Inode: <INODE> Links: 2 
> +
> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> + Size: 5 Filetype: Regular File
> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> +Device: <DEVICE> Inode: <INODE> Links: 1 
> +
>   File: "./pipe"
>   Size: 0 Filetype: Fifo File
>   Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> -- 
> 2.25.1
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Catherine Hoang]

> On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
> 
> On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
>> This test creates an xfs filesystem and verifies that the filesystem
>> matches what is specified by the protofile.
>> 
>> This patch extends the current test to check that a protofile can specify
>> setgid mode on directories. Also, check that the created symlink isn’t
>> broken.
>> 
>> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
>> ---
> 
> Any specific reason to add this test? Likes uncovering some one known
> bug/fix?
> 
> Thanks,
> Zorro

Hi Zorro,

We’ve been exploring alternate uses for protofiles and noticed a few holes
in the testing.

Thanks,
Catherine
> 
>> tests/xfs/019     |  6 ++++++
>> tests/xfs/019.out | 12 +++++++++++-
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/xfs/019 b/tests/xfs/019
>> index 3dfd5408..535b7af1 100755
>> --- a/tests/xfs/019
>> +++ b/tests/xfs/019
>> @@ -73,6 +73,10 @@ $
>> setuid -u-666 0 0 $tempfile
>> setgid --g666 0 0 $tempfile
>> setugid -ug666 0 0 $tempfile
>> +directory_setgid d-g755 3 2
>> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
>> +$
>> +: back in the root
>> block_device b--012 3 1 161 162 
>> char_device c--345 3 1 177 178
>> pipe p--670 0 0
>> @@ -114,6 +118,8 @@ _verify_fs()
>> 		| xargs $here/src/lstat64 | _filter_stat)
>> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
>> 		|| _fail "bigfile corrupted"
>> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
>> +		|| _fail "symlink broken"
>> 
>> 	echo "*** unmount FS"
>> 	_full "umount"
>> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
>> index 19614d9d..8584f593 100644
>> --- a/tests/xfs/019.out
>> +++ b/tests/xfs/019.out
>> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
>>  File: "."
>>  Size: <DSIZE> Filetype: Directory
>>  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
>> -Device: <DEVICE> Inode: <INODE> Links: 3 
>> +Device: <DEVICE> Inode: <INODE> Links: 4 
>> 
>>  File: "./bigfile"
>>  Size: 2097152 Filetype: Regular File
>> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
>>  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
>> Device: <DEVICE> Inode: <INODE> Links: 1 
>> 
>> + File: "./directory_setgid"
>> + Size: <DSIZE> Filetype: Directory
>> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
>> +Device: <DEVICE> Inode: <INODE> Links: 2 
>> +
>> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
>> + Size: 5 Filetype: Regular File
>> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
>> +Device: <DEVICE> Inode: <INODE> Links: 1 
>> +
>>  File: "./pipe"
>>  Size: 0 Filetype: Fifo File
>>  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
>> -- 
>> 2.25.1
>> 
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Zorro Lang]

On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote:
> > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
> > 
> > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
> >> This test creates an xfs filesystem and verifies that the filesystem
> >> matches what is specified by the protofile.
> >> 
> >> This patch extends the current test to check that a protofile can specify
> >> setgid mode on directories. Also, check that the created symlink isn’t
> >> broken.
> >> 
> >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> >> ---
> > 
> > Any specific reason to add this test? Likes uncovering some one known
> > bug/fix?
> > 
> > Thanks,
> > Zorro
> 
> Hi Zorro,
> 
> We’ve been exploring alternate uses for protofiles and noticed a few holes
> in the testing.

That's great, but better to show some details in the patch/commit, likes
a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you
metioned, to help others to know what's this change trying to cover.

Thanks,
Zorro

> 
> Thanks,
> Catherine
> > 
> >> tests/xfs/019     |  6 ++++++
> >> tests/xfs/019.out | 12 +++++++++++-
> >> 2 files changed, 17 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tests/xfs/019 b/tests/xfs/019
> >> index 3dfd5408..535b7af1 100755
> >> --- a/tests/xfs/019
> >> +++ b/tests/xfs/019
> >> @@ -73,6 +73,10 @@ $
> >> setuid -u-666 0 0 $tempfile
> >> setgid --g666 0 0 $tempfile
> >> setugid -ug666 0 0 $tempfile
> >> +directory_setgid d-g755 3 2
> >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
> >> +$
> >> +: back in the root
> >> block_device b--012 3 1 161 162 
> >> char_device c--345 3 1 177 178
> >> pipe p--670 0 0
> >> @@ -114,6 +118,8 @@ _verify_fs()
> >> 		| xargs $here/src/lstat64 | _filter_stat)
> >> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
> >> 		|| _fail "bigfile corrupted"
> >> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> >> +		|| _fail "symlink broken"
> >> 
> >> 	echo "*** unmount FS"
> >> 	_full "umount"
> >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> >> index 19614d9d..8584f593 100644
> >> --- a/tests/xfs/019.out
> >> +++ b/tests/xfs/019.out
> >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
> >>  File: "."
> >>  Size: <DSIZE> Filetype: Directory
> >>  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> >> -Device: <DEVICE> Inode: <INODE> Links: 3 
> >> +Device: <DEVICE> Inode: <INODE> Links: 4 
> >> 
> >>  File: "./bigfile"
> >>  Size: 2097152 Filetype: Regular File
> >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
> >>  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> >> Device: <DEVICE> Inode: <INODE> Links: 1 
> >> 
> >> + File: "./directory_setgid"
> >> + Size: <DSIZE> Filetype: Directory
> >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> >> +Device: <DEVICE> Inode: <INODE> Links: 2 
> >> +
> >> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> >> + Size: 5 Filetype: Regular File
> >> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> >> +Device: <DEVICE> Inode: <INODE> Links: 1 
> >> +
> >>  File: "./pipe"
> >>  Size: 0 Filetype: Fifo File
> >>  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> >> -- 
> >> 2.25.1
> >> 
> > 
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Darrick J. Wong]

On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote:
> On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote:
> > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
> > > 
> > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
> > >> This test creates an xfs filesystem and verifies that the filesystem
> > >> matches what is specified by the protofile.
> > >> 
> > >> This patch extends the current test to check that a protofile can specify
> > >> setgid mode on directories. Also, check that the created symlink isn’t
> > >> broken.
> > >> 
> > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > >> ---
> > > 
> > > Any specific reason to add this test? Likes uncovering some one known
> > > bug/fix?
> > > 
> > > Thanks,
> > > Zorro
> > 
> > Hi Zorro,
> > 
> > We’ve been exploring alternate uses for protofiles and noticed a few holes
> > in the testing.
> 
> That's great, but better to show some details in the patch/commit, likes
> a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you
> metioned, to help others to know what's this change trying to cover.

I think this patch is adding a check that protofile lines are actually
being honored (in the case of the symlink file) and checking that setgid
on a directory is not carried over into new children unless the
protofile explicitly marks the children setgid.

IOWs, this isn't adding a regression test for a fix in xfsprogs, it's
increasing test coverage.

--D

> Thanks,
> Zorro
> 
> > 
> > Thanks,
> > Catherine
> > > 
> > >> tests/xfs/019     |  6 ++++++
> > >> tests/xfs/019.out | 12 +++++++++++-
> > >> 2 files changed, 17 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/tests/xfs/019 b/tests/xfs/019
> > >> index 3dfd5408..535b7af1 100755
> > >> --- a/tests/xfs/019
> > >> +++ b/tests/xfs/019
> > >> @@ -73,6 +73,10 @@ $
> > >> setuid -u-666 0 0 $tempfile
> > >> setgid --g666 0 0 $tempfile
> > >> setugid -ug666 0 0 $tempfile
> > >> +directory_setgid d-g755 3 2
> > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
> > >> +$
> > >> +: back in the root
> > >> block_device b--012 3 1 161 162 
> > >> char_device c--345 3 1 177 178
> > >> pipe p--670 0 0
> > >> @@ -114,6 +118,8 @@ _verify_fs()
> > >> 		| xargs $here/src/lstat64 | _filter_stat)
> > >> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
> > >> 		|| _fail "bigfile corrupted"
> > >> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> > >> +		|| _fail "symlink broken"
> > >> 
> > >> 	echo "*** unmount FS"
> > >> 	_full "umount"
> > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > >> index 19614d9d..8584f593 100644
> > >> --- a/tests/xfs/019.out
> > >> +++ b/tests/xfs/019.out
> > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
> > >>  File: "."
> > >>  Size: <DSIZE> Filetype: Directory
> > >>  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> > >> -Device: <DEVICE> Inode: <INODE> Links: 3 
> > >> +Device: <DEVICE> Inode: <INODE> Links: 4 
> > >> 
> > >>  File: "./bigfile"
> > >>  Size: 2097152 Filetype: Regular File
> > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
> > >>  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> > >> Device: <DEVICE> Inode: <INODE> Links: 1 
> > >> 
> > >> + File: "./directory_setgid"
> > >> + Size: <DSIZE> Filetype: Directory
> > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> > >> +Device: <DEVICE> Inode: <INODE> Links: 2 
> > >> +
> > >> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> > >> + Size: 5 Filetype: Regular File
> > >> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > >> +Device: <DEVICE> Inode: <INODE> Links: 1 
> > >> +
> > >>  File: "./pipe"
> > >>  Size: 0 Filetype: Fifo File
> > >>  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> > >> -- 
> > >> 2.25.1
> > >> 
> > > 
> > 
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Zorro Lang]

On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote:
> On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote:
> > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote:
> > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
> > > > 
> > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
> > > >> This test creates an xfs filesystem and verifies that the filesystem
> > > >> matches what is specified by the protofile.
> > > >> 
> > > >> This patch extends the current test to check that a protofile can specify
> > > >> setgid mode on directories. Also, check that the created symlink isn’t
> > > >> broken.
> > > >> 
> > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > >> ---
> > > > 
> > > > Any specific reason to add this test? Likes uncovering some one known
> > > > bug/fix?
> > > > 
> > > > Thanks,
> > > > Zorro
> > > 
> > > Hi Zorro,
> > > 
> > > We’ve been exploring alternate uses for protofiles and noticed a few holes
> > > in the testing.
> > 
> > That's great, but better to show some details in the patch/commit, likes
> > a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you
> > metioned, to help others to know what's this change trying to cover.
> 
> I think this patch is adding a check that protofile lines are actually
> being honored (in the case of the symlink file) and checking that setgid
> on a directory is not carried over into new children unless the
> protofile explicitly marks the children setgid.
> 
> IOWs, this isn't adding a regression test for a fix in xfsprogs, it's
> increasing test coverage.

Oh, understand, I have no objection with this patch, just thought it covers
a known bug :) If it's good to you too, let's ACK it.

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > Thanks,
> > > Catherine
> > > > 
> > > >> tests/xfs/019     |  6 ++++++
> > > >> tests/xfs/019.out | 12 +++++++++++-
> > > >> 2 files changed, 17 insertions(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/tests/xfs/019 b/tests/xfs/019
> > > >> index 3dfd5408..535b7af1 100755
> > > >> --- a/tests/xfs/019
> > > >> +++ b/tests/xfs/019
> > > >> @@ -73,6 +73,10 @@ $
> > > >> setuid -u-666 0 0 $tempfile
> > > >> setgid --g666 0 0 $tempfile
> > > >> setugid -ug666 0 0 $tempfile
> > > >> +directory_setgid d-g755 3 2
> > > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
> > > >> +$
> > > >> +: back in the root
> > > >> block_device b--012 3 1 161 162 
> > > >> char_device c--345 3 1 177 178
> > > >> pipe p--670 0 0
> > > >> @@ -114,6 +118,8 @@ _verify_fs()
> > > >> 		| xargs $here/src/lstat64 | _filter_stat)
> > > >> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
> > > >> 		|| _fail "bigfile corrupted"
> > > >> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> > > >> +		|| _fail "symlink broken"
> > > >> 
> > > >> 	echo "*** unmount FS"
> > > >> 	_full "umount"
> > > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > > >> index 19614d9d..8584f593 100644
> > > >> --- a/tests/xfs/019.out
> > > >> +++ b/tests/xfs/019.out
> > > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
> > > >>  File: "."
> > > >>  Size: <DSIZE> Filetype: Directory
> > > >>  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> > > >> -Device: <DEVICE> Inode: <INODE> Links: 3 
> > > >> +Device: <DEVICE> Inode: <INODE> Links: 4 
> > > >> 
> > > >>  File: "./bigfile"
> > > >>  Size: 2097152 Filetype: Regular File
> > > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
> > > >>  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> > > >> Device: <DEVICE> Inode: <INODE> Links: 1 
> > > >> 
> > > >> + File: "./directory_setgid"
> > > >> + Size: <DSIZE> Filetype: Directory
> > > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> > > >> +Device: <DEVICE> Inode: <INODE> Links: 2 
> > > >> +
> > > >> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> > > >> + Size: 5 Filetype: Regular File
> > > >> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > > >> +Device: <DEVICE> Inode: <INODE> Links: 1 
> > > >> +
> > > >>  File: "./pipe"
> > > >>  Size: 0 Filetype: Fifo File
> > > >>  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> > > >> -- 
> > > >> 2.25.1
> > > >> 
> > > > 
> > > 
> > 
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Darrick J. Wong]

On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote:
> On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote:
> > On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote:
> > > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote:
> > > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
> > > > > 
> > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
> > > > >> This test creates an xfs filesystem and verifies that the filesystem
> > > > >> matches what is specified by the protofile.
> > > > >> 
> > > > >> This patch extends the current test to check that a protofile can specify
> > > > >> setgid mode on directories. Also, check that the created symlink isn’t
> > > > >> broken.
> > > > >> 
> > > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > > >> ---
> > > > > 
> > > > > Any specific reason to add this test? Likes uncovering some one known
> > > > > bug/fix?
> > > > > 
> > > > > Thanks,
> > > > > Zorro
> > > > 
> > > > Hi Zorro,
> > > > 
> > > > We’ve been exploring alternate uses for protofiles and noticed a few holes
> > > > in the testing.
> > > 
> > > That's great, but better to show some details in the patch/commit, likes
> > > a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you
> > > metioned, to help others to know what's this change trying to cover.
> > 
> > I think this patch is adding a check that protofile lines are actually
> > being honored (in the case of the symlink file) and checking that setgid
> > on a directory is not carried over into new children unless the
> > protofile explicitly marks the children setgid.
> > 
> > IOWs, this isn't adding a regression test for a fix in xfsprogs, it's
> > increasing test coverage.
> 
> Oh, understand, I have no objection with this patch, just thought it covers
> a known bug :) If it's good to you too, let's ACK it.

Yes!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Thanks,
> Zorro
> 
> > 
> > --D
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > Thanks,
> > > > Catherine
> > > > > 
> > > > >> tests/xfs/019     |  6 ++++++
> > > > >> tests/xfs/019.out | 12 +++++++++++-
> > > > >> 2 files changed, 17 insertions(+), 1 deletion(-)
> > > > >> 
> > > > >> diff --git a/tests/xfs/019 b/tests/xfs/019
> > > > >> index 3dfd5408..535b7af1 100755
> > > > >> --- a/tests/xfs/019
> > > > >> +++ b/tests/xfs/019
> > > > >> @@ -73,6 +73,10 @@ $
> > > > >> setuid -u-666 0 0 $tempfile
> > > > >> setgid --g666 0 0 $tempfile
> > > > >> setugid -ug666 0 0 $tempfile
> > > > >> +directory_setgid d-g755 3 2
> > > > >> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
> > > > >> +$
> > > > >> +: back in the root
> > > > >> block_device b--012 3 1 161 162 
> > > > >> char_device c--345 3 1 177 178
> > > > >> pipe p--670 0 0
> > > > >> @@ -114,6 +118,8 @@ _verify_fs()
> > > > >> 		| xargs $here/src/lstat64 | _filter_stat)
> > > > >> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
> > > > >> 		|| _fail "bigfile corrupted"
> > > > >> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> > > > >> +		|| _fail "symlink broken"
> > > > >> 
> > > > >> 	echo "*** unmount FS"
> > > > >> 	_full "umount"
> > > > >> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > > > >> index 19614d9d..8584f593 100644
> > > > >> --- a/tests/xfs/019.out
> > > > >> +++ b/tests/xfs/019.out
> > > > >> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
> > > > >>  File: "."
> > > > >>  Size: <DSIZE> Filetype: Directory
> > > > >>  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> > > > >> -Device: <DEVICE> Inode: <INODE> Links: 3 
> > > > >> +Device: <DEVICE> Inode: <INODE> Links: 4 
> > > > >> 
> > > > >>  File: "./bigfile"
> > > > >>  Size: 2097152 Filetype: Regular File
> > > > >> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
> > > > >>  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> > > > >> Device: <DEVICE> Inode: <INODE> Links: 1 
> > > > >> 
> > > > >> + File: "./directory_setgid"
> > > > >> + Size: <DSIZE> Filetype: Directory
> > > > >> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> > > > >> +Device: <DEVICE> Inode: <INODE> Links: 2 
> > > > >> +
> > > > >> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
> > > > >> + Size: 5 Filetype: Regular File
> > > > >> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > > > >> +Device: <DEVICE> Inode: <INODE> Links: 1 
> > > > >> +
> > > > >>  File: "./pipe"
> > > > >>  Size: 0 Filetype: Fifo File
> > > > >>  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> > > > >> -- 
> > > > >> 2.25.1
> > > > >> 
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH v1] xfs/019: extend protofile test

[Catherine Hoang]

> On Mar 25, 2022, at 10:59 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote:
>> On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote:
>>> On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote:
>>>> On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang wrote:
>>>>>> On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com> wrote:
>>>>>> 
>>>>>> On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang wrote:
>>>>>>> This test creates an xfs filesystem and verifies that the filesystem
>>>>>>> matches what is specified by the protofile.
>>>>>>> 
>>>>>>> This patch extends the current test to check that a protofile can specify
>>>>>>> setgid mode on directories. Also, check that the created symlink isn’t
>>>>>>> broken.
>>>>>>> 
>>>>>>> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
>>>>>>> ---
>>>>>> 
>>>>>> Any specific reason to add this test? Likes uncovering some one known
>>>>>> bug/fix?
>>>>>> 
>>>>>> Thanks,
>>>>>> Zorro
>>>>> 
>>>>> Hi Zorro,
>>>>> 
>>>>> We’ve been exploring alternate uses for protofiles and noticed a few holes
>>>>> in the testing.
>>>> 
>>>> That's great, but better to show some details in the patch/commit, likes
>>>> a commit id of xfsprogs?/kernel? (if there's one) which fix the bug you
>>>> metioned, to help others to know what's this change trying to cover.
>>> 
>>> I think this patch is adding a check that protofile lines are actually
>>> being honored (in the case of the symlink file) and checking that setgid
>>> on a directory is not carried over into new children unless the
>>> protofile explicitly marks the children setgid.
>>> 
>>> IOWs, this isn't adding a regression test for a fix in xfsprogs, it's
>>> increasing test coverage.
>> 
>> Oh, understand, I have no objection with this patch, just thought it covers
>> a known bug :) If it's good to you too, let's ACK it.
> 
> Yes!
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D

Thanks Darrick!
> 
>> 
>> Thanks,
>> Zorro
>> 
>>> 
>>> --D
>>> 
>>>> Thanks,
>>>> Zorro
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Catherine
>>>>>> 
>>>>>>> tests/xfs/019     |  6 ++++++
>>>>>>> tests/xfs/019.out | 12 +++++++++++-
>>>>>>> 2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>> 
>>>>>>> diff --git a/tests/xfs/019 b/tests/xfs/019
>>>>>>> index 3dfd5408..535b7af1 100755
>>>>>>> --- a/tests/xfs/019
>>>>>>> +++ b/tests/xfs/019
>>>>>>> @@ -73,6 +73,10 @@ $
>>>>>>> setuid -u-666 0 0 $tempfile
>>>>>>> setgid --g666 0 0 $tempfile
>>>>>>> setugid -ug666 0 0 $tempfile
>>>>>>> +directory_setgid d-g755 3 2
>>>>>>> +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 ---755 3 1 $tempfile
>>>>>>> +$
>>>>>>> +: back in the root
>>>>>>> block_device b--012 3 1 161 162 
>>>>>>> char_device c--345 3 1 177 178
>>>>>>> pipe p--670 0 0
>>>>>>> @@ -114,6 +118,8 @@ _verify_fs()
>>>>>>> 		| xargs $here/src/lstat64 | _filter_stat)
>>>>>>> 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
>>>>>>> 		|| _fail "bigfile corrupted"
>>>>>>> +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
>>>>>>> +		|| _fail "symlink broken"
>>>>>>> 
>>>>>>> 	echo "*** unmount FS"
>>>>>>> 	_full "umount"
>>>>>>> diff --git a/tests/xfs/019.out b/tests/xfs/019.out
>>>>>>> index 19614d9d..8584f593 100644
>>>>>>> --- a/tests/xfs/019.out
>>>>>>> +++ b/tests/xfs/019.out
>>>>>>> @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
>>>>>>> File: "."
>>>>>>> Size: <DSIZE> Filetype: Directory
>>>>>>> Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
>>>>>>> -Device: <DEVICE> Inode: <INODE> Links: 3 
>>>>>>> +Device: <DEVICE> Inode: <INODE> Links: 4 
>>>>>>> 
>>>>>>> File: "./bigfile"
>>>>>>> Size: 2097152 Filetype: Regular File
>>>>>>> @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links: 1
>>>>>>> Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
>>>>>>> Device: <DEVICE> Inode: <INODE> Links: 1 
>>>>>>> 
>>>>>>> + File: "./directory_setgid"
>>>>>>> + Size: <DSIZE> Filetype: Directory
>>>>>>> + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
>>>>>>> +Device: <DEVICE> Inode: <INODE> Links: 2 
>>>>>>> +
>>>>>>> + File: "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5"
>>>>>>> + Size: 5 Filetype: Regular File
>>>>>>> + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
>>>>>>> +Device: <DEVICE> Inode: <INODE> Links: 1 
>>>>>>> +
>>>>>>> File: "./pipe"
>>>>>>> Size: 0 Filetype: Fifo File
>>>>>>> Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
>>>>>>> -- 
>>>>>>> 2.25.1
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 

Re: [PATCH v1] xfs/019: extend protofile test

[Alli]

On Fri, 2022-03-25 at 10:59 -0700, Darrick J. Wong wrote:
> On Fri, Mar 25, 2022 at 09:33:56PM +0800, Zorro Lang wrote:
> > On Thu, Mar 24, 2022 at 01:17:30PM -0700, Darrick J. Wong wrote:
> > > On Fri, Mar 25, 2022 at 03:26:00AM +0800, Zorro Lang wrote:
> > > > On Thu, Mar 24, 2022 at 03:44:00PM +0000, Catherine Hoang
> > > > wrote:
> > > > > > On Mar 22, 2022, at 6:36 PM, Zorro Lang <zlang@redhat.com>
> > > > > > wrote:
> > > > > > 
> > > > > > On Thu, Mar 17, 2022 at 11:24:08PM +0000, Catherine Hoang
> > > > > > wrote:
> > > > > > > This test creates an xfs filesystem and verifies that the
> > > > > > > filesystem
> > > > > > > matches what is specified by the protofile.
> > > > > > > 
> > > > > > > This patch extends the current test to check that a
> > > > > > > protofile can specify
> > > > > > > setgid mode on directories. Also, check that the created
> > > > > > > symlink isn’t
> > > > > > > broken.
> > > > > > > 
> > > > > > > Signed-off-by: Catherine Hoang <
> > > > > > > catherine.hoang@oracle.com>
> > > > > > > ---
> > > > > > 
> > > > > > Any specific reason to add this test? Likes uncovering some
> > > > > > one known
> > > > > > bug/fix?
> > > > > > 
> > > > > > Thanks,
> > > > > > Zorro
> > > > > 
> > > > > Hi Zorro,
> > > > > 
> > > > > We’ve been exploring alternate uses for protofiles and
> > > > > noticed a few holes
> > > > > in the testing.
> > > > 
> > > > That's great, but better to show some details in the
> > > > patch/commit, likes
> > > > a commit id of xfsprogs?/kernel? (if there's one) which fix the
> > > > bug you
> > > > metioned, to help others to know what's this change trying to
> > > > cover.
> > > 
> > > I think this patch is adding a check that protofile lines are
> > > actually
> > > being honored (in the case of the symlink file) and checking that
> > > setgid
> > > on a directory is not carried over into new children unless the
> > > protofile explicitly marks the children setgid.
> > > 
> > > IOWs, this isn't adding a regression test for a fix in xfsprogs,
> > > it's
> > > increasing test coverage.
> > 
> > Oh, understand, I have no objection with this patch, just thought
> > it covers
> > a known bug :) If it's good to you too, let's ACK it.
> 
> Yes!
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 

This looks good to me as well.  Feel free to add my rvb:
Reviewed-by: Allison Henderson <allison.henderson@oracle.org>

Thanks!
Allison

> > Thanks,
> > Zorro
> > 
> > > --D
> > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > > Thanks,
> > > > > Catherine
> > > > > > > tests/xfs/019     |  6 ++++++
> > > > > > > tests/xfs/019.out | 12 +++++++++++-
> > > > > > > 2 files changed, 17 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/tests/xfs/019 b/tests/xfs/019
> > > > > > > index 3dfd5408..535b7af1 100755
> > > > > > > --- a/tests/xfs/019
> > > > > > > +++ b/tests/xfs/019
> > > > > > > @@ -73,6 +73,10 @@ $
> > > > > > > setuid -u-666 0 0 $tempfile
> > > > > > > setgid --g666 0 0 $tempfile
> > > > > > > setugid -ug666 0 0 $tempfile
> > > > > > > +directory_setgid d-g755 3 2
> > > > > > > +file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_5 -
> > > > > > > --755 3 1 $tempfile
> > > > > > > +$
> > > > > > > +: back in the root
> > > > > > > block_device b--012 3 1 161 162 
> > > > > > > char_device c--345 3 1 177 178
> > > > > > > pipe p--670 0 0
> > > > > > > @@ -114,6 +118,8 @@ _verify_fs()
> > > > > > > 		| xargs $here/src/lstat64 | _filter_stat)
> > > > > > > 	diff -q $SCRATCH_MNT/bigfile $tempfile.2 \
> > > > > > > 		|| _fail "bigfile corrupted"
> > > > > > > +	diff -q $SCRATCH_MNT/symlink $tempfile.2 \
> > > > > > > +		|| _fail "symlink broken"
> > > > > > > 
> > > > > > > 	echo "*** unmount FS"
> > > > > > > 	_full "umount"
> > > > > > > diff --git a/tests/xfs/019.out b/tests/xfs/019.out
> > > > > > > index 19614d9d..8584f593 100644
> > > > > > > --- a/tests/xfs/019.out
> > > > > > > +++ b/tests/xfs/019.out
> > > > > > > @@ -7,7 +7,7 @@ Wrote 2048.00Kb (value 0x2c)
> > > > > > >  File: "."
> > > > > > >  Size: <DSIZE> Filetype: Directory
> > > > > > >  Mode: (0777/drwxrwxrwx) Uid: (3) Gid: (1)
> > > > > > > -Device: <DEVICE> Inode: <INODE> Links: 3 
> > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 4 
> > > > > > > 
> > > > > > >  File: "./bigfile"
> > > > > > >  Size: 2097152 Filetype: Regular File
> > > > > > > @@ -54,6 +54,16 @@ Device: <DEVICE> Inode: <INODE> Links:
> > > > > > > 1
> > > > > > >  Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (1)
> > > > > > > Device: <DEVICE> Inode: <INODE> Links: 1 
> > > > > > > 
> > > > > > > + File: "./directory_setgid"
> > > > > > > + Size: <DSIZE> Filetype: Directory
> > > > > > > + Mode: (2755/drwxr-sr-x) Uid: (3) Gid: (2)
> > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 2 
> > > > > > > +
> > > > > > > + File:
> > > > > > > "./directory_setgid/file_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > > > xxxxxxxxxxx_5"
> > > > > > > + Size: 5 Filetype: Regular File
> > > > > > > + Mode: (0755/-rwxr-xr-x) Uid: (3) Gid: (2)
> > > > > > > +Device: <DEVICE> Inode: <INODE> Links: 1 
> > > > > > > +
> > > > > > >  File: "./pipe"
> > > > > > >  Size: 0 Filetype: Fifo File
> > > > > > >  Mode: (0670/frw-rwx---) Uid: (0) Gid: (0)
> > > > > > > -- 
> > > > > > > 2.25.1
> > > > > > >