Re: [PATCH v2 1/6] idmapped-mount: split setgid test from test-core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 08, 2022 at 10:17:34AM +0000, xuyang2018.jy@xxxxxxxxxxx wrote:
> on 2022/4/8 9:20, xuyang2018.jy@xxxxxxxxxxx wrote:
> > on 2022/4/7 20:55, Christian Brauner wrote:
> >> On Thu, Apr 07, 2022 at 08:09:30PM +0800, Yang Xu wrote:
> >>> Since we plan to increase setgid test covertage, it will find new bug
> >>> , so add a new test group test-setgid is better.
> >>>
> >>> Also add a new test case to test test-setgid instead of miss it.
> >>>
> >>> Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxx>
> >>> ---
> >>>    src/idmapped-mounts/idmapped-mounts.c | 19 +++++++++++++++----
> >>>    tests/generic/999                     | 26 ++++++++++++++++++++++++++
> >>>    tests/generic/999.out                 |  2 ++
> >>
> >> I actually didn't mean to split out the existing setgid tests. I mean
> >> adding new ones for the test-cases you're adding. But how you did it
> >> works for me too and is a bit nicer. I don't have a strong opinion so as
> >> long as Dave and Darrick are fine with it then this seems good to me.
> > Ok, let's listen ..
> When I write v3, I add mknodat patch as 1st patch and tmpfile as 2nd 
> patch(by using a file doesn't under DIR1 directory, so I don't need to 
> concern about xfs_irix_sgid_inherit_enabled), errno reset to 0 as 3st 
> patch. It seems this way can't introduce the new failure for generic/633.

Sure.

> 
> So I will add a new group for umask and acl and add new case for them 
> instead of split setgid case from test-core group.

Yes. What I'd expect to see is something like:

struct t_idmapped_mounts t_setgid_umask[] = {
	{ setgid_create_umask,						false,	"blabla",     },
	{ setgid_create_umask_idmapped,					true,	"blabla",     },
	{ setgid_create_umask_idmapped_in_userns,			true,	"blabla",     },
};

struct t_idmapped_mounts t_setgid_acl[] = {
	{ setgid_create_acl,						false,	"blabla",	},
	{ setgid_create_acl_idmapped,					true,	"blabla",	},
	{ setgid_create_acl_idmapped_in_userns,				true,	"blabla",	},
};

and two command line switches:

--test-setgid-umask
--test-setgid-acl

and two tests:

generic/<idx>
generic/<idx + 1>

where one of them calls:

--test-setgid-umask

and the other one:

--test-setgid-acl

That's roughly how I think it should work. But if you have a better
approach, please propose it.

> 
> ps: I doubt whether I need to send two patch sets(one is about 
> mknodat,tmpfile,errno, the another is about umask,acl,new case).
> What do you think about this?

I think a single patch _series_ with multiple patches:
1. errno bugfix
2. mknodat()
3. tmpfile()
4. umask & new test case for it as sm like generic/<idx>
4. acl & new test case for it as sm like generic/<idx + 1>

Christian



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux