| Message ID | 1649333375-2599-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] idmapped-mount: split setgid test from test-core | expand |
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@fujitsu.com> > --- > 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. One note about the test name/numbering though. It seems you haven't added the test using the provided xfstest infrastructure to do that. Instead of manually adding the test you should run the "new" script. You should run: ~/src/git/xfstests$ ./new generic Next test id is 678 Append a name to the ID? Test name will be 678-$name. y,[n]: Creating test file '678' Add to group(s) [auto] (separate by space, ? for list): auto quick attr idmapped mount perms Creating skeletal script for you to edit ... that'll automatically figure out the correct test number etc.
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@fujitsu.com> >> --- >> 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 .. > > One note about the test name/numbering though. It seems you haven't > added the test using the provided xfstest infrastructure to do that. > Instead of manually adding the test you should run the "new" script. > > You should run: > > ~/src/git/xfstests$ ./new generic > > Next test id is 678 > Append a name to the ID? Test name will be 678-$name. y,[n]: > Creating test file '678' > Add to group(s) [auto] (separate by space, ? for list): auto quick attr idmapped mount perms > Creating skeletal script for you to edit ... > > that'll automatically figure out the correct test number etc. Thanks, TBH, I don't know this usage. I don't name to 678 because fstests patchwork has others new case(in reviewing), so I add a biger number. Best Regards Yang Xu
on 2022/4/8 9:20, xuyang2018.jy@fujitsu.com 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@fujitsu.com> >>> --- >>> 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. 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. 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? Best Regards Yang Xu >> >> One note about the test name/numbering though. It seems you haven't >> added the test using the provided xfstest infrastructure to do that. >> Instead of manually adding the test you should run the "new" script. >> >> You should run: >> >> ~/src/git/xfstests$ ./new generic >> >> Next test id is 678 >> Append a name to the ID? Test name will be 678-$name. y,[n]: >> Creating test file '678' >> Add to group(s) [auto] (separate by space, ? for list): auto quick attr idmapped mount perms >> Creating skeletal script for you to edit ... >> >> that'll automatically figure out the correct test number etc. > Thanks, TBH, I don't know this usage. I don't name to 678 because > fstests patchwork has others new case(in reviewing), so I add a biger > number. > > Best Regards > Yang Xu
On Fri, Apr 08, 2022 at 10:17:34AM +0000, xuyang2018.jy@fujitsu.com wrote: > on 2022/4/8 9:20, xuyang2018.jy@fujitsu.com 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@fujitsu.com> > >>> --- > >>> 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
diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c index 4cf6c3bb..dff6820f 100644 --- a/src/idmapped-mounts/idmapped-mounts.c +++ b/src/idmapped-mounts/idmapped-mounts.c @@ -13809,6 +13809,7 @@ static void usage(void) fprintf(stderr, "--test-nested-userns Run nested userns idmapped mount testsuite\n"); fprintf(stderr, "--test-btrfs Run btrfs specific idmapped mount testsuite\n"); fprintf(stderr, "--test-setattr-fix-968219708108 Run setattr regression tests\n"); + fprintf(stderr, "--test-setgid Run setgid create tests\n"); _exit(EXIT_SUCCESS); } @@ -13826,6 +13827,7 @@ static const struct option longopts[] = { {"test-nested-userns", no_argument, 0, 'n'}, {"test-btrfs", no_argument, 0, 'b'}, {"test-setattr-fix-968219708108", no_argument, 0, 'i'}, + {"test-setgid", no_argument, 0, 'j'}, {NULL, 0, 0, 0}, }; @@ -13866,9 +13868,6 @@ struct t_idmapped_mounts { { setattr_truncate, false, "setattr truncate", }, { setattr_truncate_idmapped, true, "setattr truncate on idmapped mounts", }, { setattr_truncate_idmapped_in_userns, true, "setattr truncate on idmapped mounts in user namespace", }, - { setgid_create, false, "create operations in directories with setgid bit set", }, - { setgid_create_idmapped, true, "create operations in directories with setgid bit set on idmapped mounts", }, - { setgid_create_idmapped_in_userns, true, "create operations in directories with setgid bit set on idmapped mounts in user namespace", }, { setid_binaries, false, "setid binaries on regular mounts", }, { setid_binaries_idmapped_mounts, true, "setid binaries on idmapped mounts", }, { setid_binaries_idmapped_mounts_in_userns, true, "setid binaries on idmapped mounts in user namespace", }, @@ -13923,6 +13922,12 @@ struct t_idmapped_mounts t_setattr_fix_968219708108[] = { { setattr_fix_968219708108, true, "test that setattr works correctly", }, }; +struct t_idmapped_mounts t_setgid[] = { + { setgid_create, false, "create operations in directories with setgid bit set", }, + { setgid_create_idmapped, true, "create operations in directories with setgid bit set on idmapped mounts", }, + { setgid_create_idmapped_in_userns, true, "create operations in directories with setgid bit set on idmapped mounts in user namespace", }, +}; + static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size) { int i; @@ -14000,7 +14005,7 @@ int main(int argc, char *argv[]) int index = 0; bool supported = false, test_btrfs = false, test_core = false, test_fscaps_regression = false, test_nested_userns = false, - test_setattr_fix_968219708108 = false; + test_setattr_fix_968219708108 = false, test_setgid = false; while ((ret = getopt_long_only(argc, argv, "", longopts, &index)) != -1) { switch (ret) { @@ -14037,6 +14042,9 @@ int main(int argc, char *argv[]) case 'i': test_setattr_fix_968219708108 = true; break; + case 'j': + test_setgid = true; + break; case 'h': /* fallthrough */ default: @@ -14106,6 +14114,9 @@ int main(int argc, char *argv[]) ARRAY_SIZE(t_setattr_fix_968219708108))) goto out; + if (test_setgid && !run_test(t_setgid, ARRAY_SIZE(t_setgid))) + goto out; + fret = EXIT_SUCCESS; out: diff --git a/tests/generic/999 b/tests/generic/999 new file mode 100755 index 00000000..46a34804 --- /dev/null +++ b/tests/generic/999 @@ -0,0 +1,26 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 FUJITSU LIMITED. All rights reserved +# +# FS QA Test 999 +# +# Test that setgid bit behave correctly. +# +. ./common/preamble +_begin_fstest auto quick cap idmapped mount perms rw + +# Import common functions. +. ./common/filter + +# real QA test starts here + +_supported_fs generic +_require_test + +echo "Silence is golden" + +$here/src/idmapped-mounts/idmapped-mounts --test-setgid --device "$TEST_DEV" \ + --mount "$TEST_DIR" --fstype "$FSTYP" + +status=$? +exit diff --git a/tests/generic/999.out b/tests/generic/999.out new file mode 100644 index 00000000..3b276ca8 --- /dev/null +++ b/tests/generic/999.out @@ -0,0 +1,2 @@ +QA output created by 999 +Silence is golden
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@fujitsu.com> --- src/idmapped-mounts/idmapped-mounts.c | 19 +++++++++++++++---- tests/generic/999 | 26 ++++++++++++++++++++++++++ tests/generic/999.out | 2 ++ 3 files changed, 43 insertions(+), 4 deletions(-) create mode 100755 tests/generic/999 create mode 100644 tests/generic/999.out