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

[Alli <allison.henderson@oracle.com>]

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