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
