[2/3] syscalls/setgroups02: Convert to new API

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/syscalls/setgroups/setgroups02.c | 194 +++++-----------------
 1 file changed, 41 insertions(+), 153 deletions(-)

Hi!
> +	TEST(SETGROUPS(gidsetsize, groups_set));

TST_EXP_PASS(SETGROUPS(gidsetsize, groups_set)) ?

> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> -
> -		/*
> -		 * Call setgroups() to set supplimentary group IDs of
> -		 * the calling super-user process to gid of TESTUSER.
> -		 */
> -		TEST(SETGROUPS(cleanup, gidsetsize, groups_list));
> +	if (TST_RET < 0) {
> +		tst_res(TFAIL, "setgroups(%d, groups_set) Failed, "
> +			"errno=%d : %s", gidsetsize,
> +			TST_ERR, strerror(TST_ERR));
> +	}
>  
> -		if (TEST_RETURN == -1) {
> -			tst_resm(TFAIL, "setgroups(%d, groups_list) Failed, "
> -				 "errno=%d : %s", gidsetsize, TEST_ERRNO,
> -				 strerror(TEST_ERRNO));
> -			continue;
> -		}
> +	GETGROUPS(gidsetsize, groups_get);

TST_EXP_VAL(GETGROUPS(gitsetsize, groups_get), 1) ?

Also I suppose that we should make sure that the groups_get[0] value is
not equal to the groups_set[0], expecially with -i 2

So maybe:
	groups_get[0] = ~groups_set[0];

> -		/*
> -		 * Call getgroups(2) to verify that
> -		 * setgroups(2) successfully set the
> -		 * supp. gids of TESTUSER.
> -		 */
> -		groups_list[0] = '\0';
> -		if (GETGROUPS(cleanup, gidsetsize, groups_list) < 0) {
> -			tst_brkm(TFAIL, cleanup, "getgroups() Fails, "
> -				 "error=%d", errno);
> -		}
> -		for (i = 0; i < NGROUPS; i++) {
> -			if (groups_list[i] == user_info->pw_gid) {
> -				tst_resm(TPASS,
> -					 "Functionality of setgroups"
> -					 "(%d, groups_list) successful",
> -					 gidsetsize);
> -				PASS_FLAG = 1;
> -			}
> -		}
> -		if (PASS_FLAG == 0) {
> -			tst_resm(TFAIL, "Supplimentary gid %d not set "
> -				 "for the process", user_info->pw_gid);
> -		}
> +	if (groups_get[0] == groups_set[0]) {
> +		tst_res(TPASS,
> +			"Functionality of setgroups"
> +			"(%d, groups_set) successful",
> +			gidsetsize);
>  	}

TST_EXP_EQ_LI(groups_get[0], groups_set[0]);

At least that will produce TFAIL results if they don't match, which is
missing here.

> -	cleanup();
> -	tst_exit();
>  }
>  
> -/*
> - * setup() - performs all ONE TIME setup for this test.
> - *
> - *  Make sure the test process uid is root.
> - *  Get the supplimentrary group id of test user from /etc/passwd file.
> - */
> -void setup(void)
> +static void setup(void)
>  {
> +	user_info = SAFE_GETPWNAM(TESTUSER);
>  
> -	tst_require_root();
> -
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> -
> -	/* Get the group id info. of TESTUSER from /etc/passwd */
> -	if ((user_info = getpwnam(TESTUSER)) == NULL) {
> -		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed", TESTUSER);
> -	}
> +	GID16_CHECK(user_info->pw_gid, setgroups);
>  
> -	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
> -		tst_brkm(TBROK,
> -			 cleanup,
> -			 "gid returned from getpwnam is too large for testing setgroups16");
> -	}
> -
> -	groups_list[0] = user_info->pw_gid;
> +	groups_set[0] = user_info->pw_gid;

Do we really need this? I suppose that any random value will do, e.g.

	groups_set[0] = 42;

is going to be as good as anything else.

>  }
>  
> -/*
> - * cleanup() - performs all ONE TIME cleanup for this test at
> - *	       completion or premature exit.
> - */
> -void cleanup(void)
> -{
> -
> -}
> +static struct tst_test test = {
> +	.test_all = verify_setgroups,
> +	.setup = setup,
> +	.needs_root = 1,
> +};
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Hi!
Also can we please allocate the buffers passed to setgroups/getgroups as guarded buffers[1] of size 1?

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#131-guarded-buffers

Hi Cyril

> Hi!
> Also can we please allocate the buffers passed to setgroups/getgroups as guarded buffers[1] of size 1?
>
> [1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#131-guarded-buffers

Yes,I will update it as you suggested.

Best Regards

Yang Xu

Hi Cyril

Hi!


+       TEST(SETGROUPS(gidsetsize, groups_set));



TST_EXP_PASS(SETGROUPS(gidsetsize, groups_set)) ?


OK.

-       setup();
-
-       for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-               tst_count = 0;
-
-               /*
-                * Call setgroups() to set supplimentary group IDs of
-                * the calling super-user process to gid of TESTUSER.
-                */
-               TEST(SETGROUPS(cleanup, gidsetsize, groups_list));
+       if (TST_RET < 0) {
+               tst_res(TFAIL, "setgroups(%d, groups_set) Failed, "
+                       "errno=%d : %s", gidsetsize,
+                       TST_ERR, strerror(TST_ERR));
+       }

-               if (TEST_RETURN == -1) {
-                       tst_resm(TFAIL, "setgroups(%d, groups_list) Failed, "
-                                "errno=%d : %s", gidsetsize, TEST_ERRNO,
-                                strerror(TEST_ERRNO));
-                       continue;
-               }
+       GETGROUPS(gidsetsize, groups_get);



TST_EXP_VAL(GETGROUPS(gitsetsize, groups_get), 1) ?

Also I suppose that we should make sure that the groups_get[0] value is
not equal to the groups_set[0], expecially with -i 2

So maybe:
        groups_get[0] = ~groups_set[0];


I will update it, but the results we assume are equal.

-               /*
-                * Call getgroups(2) to verify that
-                * setgroups(2) successfully set the
-                * supp. gids of TESTUSER.
-                */
-               groups_list[0] = '\0';
-               if (GETGROUPS(cleanup, gidsetsize, groups_list) < 0) {
-                       tst_brkm(TFAIL, cleanup, "getgroups() Fails, "
-                                "error=%d", errno);
-               }
-               for (i = 0; i < NGROUPS; i++) {
-                       if (groups_list[i] == user_info->pw_gid) {
-                               tst_resm(TPASS,
-                                        "Functionality of setgroups"
-                                        "(%d, groups_list) successful",
-                                        gidsetsize);
-                               PASS_FLAG = 1;
-                       }
-               }
-               if (PASS_FLAG == 0) {
-                       tst_resm(TFAIL, "Supplimentary gid %d not set "
-                                "for the process", user_info->pw_gid);
-               }
+       if (groups_get[0] == groups_set[0]) {
+               tst_res(TPASS,
+                       "Functionality of setgroups"
+                       "(%d, groups_set) successful",
+                       gidsetsize);
        }



TST_EXP_EQ_LI(groups_get[0], groups_set[0]);

At least that will produce TFAIL results if they don't match, which is
missing here.


Sound reasonable.

-       cleanup();
-       tst_exit();
 }

-/*
- * setup() - performs all ONE TIME setup for this test.
- *
- *  Make sure the test process uid is root.
- *  Get the supplimentrary group id of test user from /etc/passwd file.
- */
-void setup(void)
+static void setup(void)
 {
+       user_info = SAFE_GETPWNAM(TESTUSER);

-       tst_require_root();
-
-       tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-       TEST_PAUSE;
-
-       /* Get the group id info. of TESTUSER from /etc/passwd */
-       if ((user_info = getpwnam(TESTUSER)) == NULL) {
-               tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed", TESTUSER);
-       }
+       GID16_CHECK(user_info->pw_gid, setgroups);

-       if (!GID_SIZE_CHECK(user_info->pw_gid)) {
-               tst_brkm(TBROK,
-                        cleanup,
-                        "gid returned from getpwnam is too large for testing setgroups16");
-       }
-
-       groups_list[0] = user_info->pw_gid;
+       groups_set[0] = user_info->pw_gid;



Do we really need this? I suppose that any random value will do, e.g.

        groups_set[0] = 42;

is going to be as good as anything else.


Agree.

 }

-/*
- * cleanup() - performs all ONE TIME cleanup for this test at
- *            completion or premature exit.
- */
-void cleanup(void)
-{
-
-}
+static struct tst_test test = {
+       .test_all = verify_setgroups,
+       .setup = setup,
+       .needs_root = 1,
+};
--
1.8.3.1


--
Mailing list info: https://lists.linux.it/listinfo/ltp


Best Regards

Yang Xu