[2/2] syscalls/chmod05: Use new ltp library

Message ID 20181106161126.251484-3-sspatil@google.com
State New
Headers show
Series
  • Misc cleanup (new lib) and fixes for Android
Related show

Commit Message

Sandeep Patil Nov. 6, 2018, 4:11 p.m.
Use SPDX-Licence-Identifier and delete dead comments / code
while at it.

Signed-off-by: Sandeep Patil <sspatil@google.com>
---

v1->v2:
------
- Fix SPDX identifier to make it GPLv2 or later.
- Fix build errors (missed cause 'make install' doesn't build things again)
- Add cleanup function to reset the effective uid / gid.

 testcases/kernel/syscalls/chmod/chmod05.c | 191 +++++-----------------
 1 file changed, 39 insertions(+), 152 deletions(-)

Comments

Sandeep Patil Nov. 6, 2018, 4:14 p.m. | #1
On Tue, Nov 06, 2018 at 08:11:26AM -0800, Sandeep Patil wrote:
> Use SPDX-Licence-Identifier and delete dead comments / code
> while at it.
> 
> Signed-off-by: Sandeep Patil <sspatil@google.com>
> ---
> 
> v1->v2:
> ------
> - Fix SPDX identifier to make it GPLv2 or later.
> - Fix build errors (missed cause 'make install' doesn't build things again)
> - Add cleanup function to reset the effective uid / gid.
> 
>  testcases/kernel/syscalls/chmod/chmod05.c | 191 +++++-----------------
>  1 file changed, 39 insertions(+), 152 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/chmod/chmod05.c b/testcases/kernel/syscalls/chmod/chmod05.c
> index 3cf4db5d0..0e86f0691 100644
> --- a/testcases/kernel/syscalls/chmod/chmod05.c
> +++ b/testcases/kernel/syscalls/chmod/chmod05.c
> @@ -1,30 +1,12 @@
> +/// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - *
>   *   Copyright (c) International Business Machines  Corp., 2001
> - *
> - *   This program is free software;  you can redistribute it and/or modify
> - *   it under the terms of the GNU General Public License as published by
> - *   the Free Software Foundation; either version 2 of the License, or
> - *   (at your option) any later version.
> - *
> - *   This program is distributed in the hope that it will be useful,
> - *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> - *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> - *   the GNU General Public License for more details.
> - *
> - *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
>  /*
>   * Test Name: chmod05
>   *
>   * Test Description:
> -//wjh
> -//This test seems to be invalid see comments below
> -//The man page for chmod doesn't seem to indicate that the setgid bit can
> -//only be set by root
>   *  Verify that, chmod(2) will succeed to change the mode of a directory
>   *  but fails to set the setgid bit on it if invoked by non-root (uid != 0)
>   *  process with the following constraints,
> @@ -36,48 +18,6 @@
>   *  chmod() should return value 0 on success and though succeeds to change
>   *  the mode of a directory but fails to set setgid bit on it.
>   *
> - * Algorithm:
> - *  Setup:
> - *   Setup signal handling.
> - *   Create temporary directory.
> - *   Pause for SIGUSR1 if option specified.
> - *
> - *  Test:
> - *   Loop if the proper options are given.
> - *   Execute system call
> - *   Check return code, if system call failed (return=-1)
> - *	Log the errno and Issue a FAIL message.
> - *   Otherwise,
> - *	Verify the Functionality of system call
> - *      if successful,
> - *		Issue Functionality-Pass message.
> - *      Otherwise,
> - *		Issue Functionality-Fail message.
> - *  Cleanup:
> - *   Print errno log and/or timing stats if options given
> - *   Delete the temporary directory created.
> - *
> - * Usage:  <for command-line>
> - *  chmod05 [-c n] [-e] [-f] [-i n] [-I x] [-P x] [-t]
> - *     where,  -c n : Run n copies concurrently.
> - *             -e   : Turn on errno logging.
> - *             -f   : Turn off functionality Testing.
> - *	       -i n : Execute test n times.
> - *	       -I x : Execute test for x seconds.
> - *	       -P x : Pause for x seconds between iterations.
> - *	       -t   : Turn on syscall timing.
> - *
> - * HISTORY
> - *	07/2001 Ported by Wayne Boyer
> - *	05/2002 Fixes by wjhuie and Ferhan Shareef
> - *                  changed conditional to check for valid tests only and
> - *                  in a more logically understandable way
> - *	07/2002 Additional fixes to #defines to allow comparisions to work.
> - *		-Robbie Williamson
> - *
> - * RESTRICTIONS:
> - *  This test should be run by root.
> - *
>   */
>  
>  #ifndef _GNU_SOURCE
> @@ -96,126 +36,73 @@
>  #include <grp.h>
>  #include <pwd.h>
>  
> -#include "test.h"
> -#include "safe_macros.h"
> -
> -#define DEBUG 0
> +#include "tst_test.h"
>  
>  #define MODE_RWX	(mode_t)(S_IRWXU | S_IRWXG | S_IRWXO)
>  #define DIR_MODE	(mode_t)(S_ISVTX | S_ISGID | S_IFDIR)
>  #define PERMS		(mode_t)(MODE_RWX | DIR_MODE)
>  #define TESTDIR		"testdir"
>  
> -char *TCID = "chmod05";
> -int TST_TOTAL = 1;
> -
> -void setup();			/* Main setup function for test */
> -void cleanup();			/* Main cleanup function for test */
> -
> -int main(int ac, char **av)
> +void test_chmod(void)
>  {
>  	struct stat stat_buf;	/* stat struct */
> -	int lc;
>  	mode_t dir_mode;	/* mode permissions set on test directory */
>  
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> -
> -		TEST(chmod(TESTDIR, PERMS));
> -
> -		if (TEST_RETURN == -1) {
> -			tst_resm(TFAIL, "chmod(%s, %#o) failed",
> -				 TESTDIR, PERMS);
> -			continue;
> -		}
> -		/*
> -		 * Get the directory information using
> -		 * stat(2).
> -		 */
> -		if (stat(TESTDIR, &stat_buf) < 0) {
> -			tst_brkm(TFAIL | TERRNO, cleanup,
> -				 "stat(%s) failed", TESTDIR);
> -		}
> -		dir_mode = stat_buf.st_mode;
> -#if DEBUG
> -		printf("DIR_MODE = 0%03o\n", DIR_MODE);
> -		printf("MODE_RWX = 0%03o\n", MODE_RWX);
> -		printf("PERMS = 0%03o\n", PERMS);
> -		printf("dir_mode = 0%03o\n", dir_mode);
> -#endif
> -		if ((PERMS & ~S_ISGID) != dir_mode)
> -			tst_resm(TFAIL, "%s: Incorrect modes 0%03o, "
> -				 "Expected 0%03o", TESTDIR, dir_mode,
> -				 PERMS & ~S_ISGID);
> -		else
> -			tst_resm(TPASS,
> -				 "Functionality of chmod(%s, %#o) successful",
> -					 TESTDIR, PERMS);
> +	TEST(chmod(TESTDIR, PERMS));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL, "chmod(%s, %#o) failed", TESTDIR, PERMS);
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	/* Get the directory information using stat(2) and compare with
> +	 * expected results.
> +	 */
> +	SAFE_STAT(TESTDIR, &stat_buf);
> +	dir_mode = stat_buf.st_mode;
> +	if ((PERMS & ~S_ISGID) != dir_mode) {
> +		tst_res(TFAIL, "%s: Incorrect modes 0%03o, "
> +				"Expected 0%03o", TESTDIR, dir_mode,
> +				PERMS & ~S_ISGID);
> +	} else {
> +		tst_res(TPASS, "Functionality of chmod(%s, %#o) successful",
> +				TESTDIR, PERMS);
> +	}
>  }
>  
> -/*
> - * void
> - * setup() - performs all ONE TIME setup for this test.
> - *  Create a temporary directory and cd to it.
> - *  Create a test directory under temporary directory.
> -//wjh we are root so do we really need this kluged helper program?
> - *  Invoke setuid to root program to modify group ownership
> - *  on test directory.
> - */
>  void setup(void)
>  {
>  	struct passwd *nobody_u;
> -	struct group *bin_group;
> +	struct group *bin_gr;
>  
> -	tst_require_root();
> -
> -	TEST_PAUSE;
> -
> -	tst_tmpdir();
> -
> -	nobody_u = getpwnam("nobody");
> -	if (nobody_u == NULL)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "getpwnam(\"nobody\") failed");
> -
> -	bin_group = getgrnam("bin");
> -	if (bin_group == NULL)
> -		tst_brkm(TBROK | TERRNO, cleanup, "getgrnam(\"bin\") failed");
> +	nobody_u = SAFE_GETPWNAM("nobody");
> +	bin_gr = SAFE_GETGRNAM("bin");
>  
>  	/*
>  	 * Create a test directory under temporary directory with specified
>  	 * mode permissions and change the gid of test directory to nobody's
>  	 * gid.
>  	 */
> -	SAFE_MKDIR(cleanup, TESTDIR, MODE_RWX);
> -
> +	SAFE_MKDIR(TESTDIR, MODE_RWX);
>  	if (setgroups(1, &nobody_u->pw_gid) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "setgroups to nobody's gid failed");
> +		tst_brk(TBROK | TERRNO, "setgroups to nobody's gid failed");
>  
> -	SAFE_CHOWN(cleanup, TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
> +	SAFE_CHOWN(TESTDIR, nobody_u->pw_uid, bin_gr->gr_gid);
>  
>  	/* change to nobody:nobody */
> -	if (setegid(nobody_u->pw_gid) == -1 || seteuid(nobody_u->pw_uid) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "Couldn't switch to nobody:nobody");
> +	SAFE_SETEGID(nobody_u->pw_gid);
> +	SAFE_SETEUID(nobody_u->pw_uid);
>  }
>  
> -void cleanup(void)
> +void cleanup_chmod(void)
>  {
> -	if (setegid(0) == -1)
> -		tst_resm(TWARN | TERRNO, "setegid(0) failed");
> -	if (seteuid(0) == -1)
> -		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
> -
> -	tst_rmdir();
> +	/* Reset the uid / gid to root */
> +	SAFE_SETEGID(0);
> +	SAFE_SETEUID(0);
>  }

This was added, but I don't think it hurts anything, if at all it resets what
the test does in the setup (even though its probably not needed)?. I'll leave
it upto you to keep / delete this.

- ssp

> +
> +static struct tst_test test = {
> +	.needs_root 	= 1,
> +	.needs_tmpdir 	= 1,
> +	.setup 		= setup,
> +	.test_all 	= test_chmod,
> +	.cleanup 	= cleanup_chmod,
> +};
> -- 
> 2.19.1.930.g4563a0d9d0-goog
>
Cyril Hrubis Nov. 6, 2018, 4:33 p.m. | #2
Hi!
> This was added, but I don't think it hurts anything, if at all it resets what
> the test does in the setup (even though its probably not needed)?. I'll leave
> it upto you to keep / delete this.

I've removed the cleanup and did a few minor changes as well and pushed,
thanks.

* Deleted /* stat struct */ and other comments commenting obvious

* Added return; in the if (TST_RET == -1) statement, there is no point
  in checking the directory permissions if the call has failed

* Removed spaces before tabs in the tst_test structure
Sandeep Patil Nov. 6, 2018, 5:01 p.m. | #3
On Tue, Nov 06, 2018 at 05:33:05PM +0100, Cyril Hrubis wrote:
> Hi!
> > This was added, but I don't think it hurts anything, if at all it resets what
> > the test does in the setup (even though its probably not needed)?. I'll leave
> > it upto you to keep / delete this.
> 
> I've removed the cleanup and did a few minor changes as well and pushed,
> thanks.
> 
> * Deleted /* stat struct */ and other comments commenting obvious
> 
> * Added return; in the if (TST_RET == -1) statement, there is no point
>   in checking the directory permissions if the call has failed
> 
> * Removed spaces before tabs in the tst_test structure

That was done to align the fields, I will be more mindful next time.
Thanks for making the changes.

- ssp

> 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

Patch

diff --git a/testcases/kernel/syscalls/chmod/chmod05.c b/testcases/kernel/syscalls/chmod/chmod05.c
index 3cf4db5d0..0e86f0691 100644
--- a/testcases/kernel/syscalls/chmod/chmod05.c
+++ b/testcases/kernel/syscalls/chmod/chmod05.c
@@ -1,30 +1,12 @@ 
+/// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *
  *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 /*
  * Test Name: chmod05
  *
  * Test Description:
-//wjh
-//This test seems to be invalid see comments below
-//The man page for chmod doesn't seem to indicate that the setgid bit can
-//only be set by root
  *  Verify that, chmod(2) will succeed to change the mode of a directory
  *  but fails to set the setgid bit on it if invoked by non-root (uid != 0)
  *  process with the following constraints,
@@ -36,48 +18,6 @@ 
  *  chmod() should return value 0 on success and though succeeds to change
  *  the mode of a directory but fails to set setgid bit on it.
  *
- * Algorithm:
- *  Setup:
- *   Setup signal handling.
- *   Create temporary directory.
- *   Pause for SIGUSR1 if option specified.
- *
- *  Test:
- *   Loop if the proper options are given.
- *   Execute system call
- *   Check return code, if system call failed (return=-1)
- *	Log the errno and Issue a FAIL message.
- *   Otherwise,
- *	Verify the Functionality of system call
- *      if successful,
- *		Issue Functionality-Pass message.
- *      Otherwise,
- *		Issue Functionality-Fail message.
- *  Cleanup:
- *   Print errno log and/or timing stats if options given
- *   Delete the temporary directory created.
- *
- * Usage:  <for command-line>
- *  chmod05 [-c n] [-e] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -e   : Turn on errno logging.
- *             -f   : Turn off functionality Testing.
- *	       -i n : Execute test n times.
- *	       -I x : Execute test for x seconds.
- *	       -P x : Pause for x seconds between iterations.
- *	       -t   : Turn on syscall timing.
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *	05/2002 Fixes by wjhuie and Ferhan Shareef
- *                  changed conditional to check for valid tests only and
- *                  in a more logically understandable way
- *	07/2002 Additional fixes to #defines to allow comparisions to work.
- *		-Robbie Williamson
- *
- * RESTRICTIONS:
- *  This test should be run by root.
- *
  */
 
 #ifndef _GNU_SOURCE
@@ -96,126 +36,73 @@ 
 #include <grp.h>
 #include <pwd.h>
 
-#include "test.h"
-#include "safe_macros.h"
-
-#define DEBUG 0
+#include "tst_test.h"
 
 #define MODE_RWX	(mode_t)(S_IRWXU | S_IRWXG | S_IRWXO)
 #define DIR_MODE	(mode_t)(S_ISVTX | S_ISGID | S_IFDIR)
 #define PERMS		(mode_t)(MODE_RWX | DIR_MODE)
 #define TESTDIR		"testdir"
 
-char *TCID = "chmod05";
-int TST_TOTAL = 1;
-
-void setup();			/* Main setup function for test */
-void cleanup();			/* Main cleanup function for test */
-
-int main(int ac, char **av)
+void test_chmod(void)
 {
 	struct stat stat_buf;	/* stat struct */
-	int lc;
 	mode_t dir_mode;	/* mode permissions set on test directory */
 
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		TEST(chmod(TESTDIR, PERMS));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL, "chmod(%s, %#o) failed",
-				 TESTDIR, PERMS);
-			continue;
-		}
-		/*
-		 * Get the directory information using
-		 * stat(2).
-		 */
-		if (stat(TESTDIR, &stat_buf) < 0) {
-			tst_brkm(TFAIL | TERRNO, cleanup,
-				 "stat(%s) failed", TESTDIR);
-		}
-		dir_mode = stat_buf.st_mode;
-#if DEBUG
-		printf("DIR_MODE = 0%03o\n", DIR_MODE);
-		printf("MODE_RWX = 0%03o\n", MODE_RWX);
-		printf("PERMS = 0%03o\n", PERMS);
-		printf("dir_mode = 0%03o\n", dir_mode);
-#endif
-		if ((PERMS & ~S_ISGID) != dir_mode)
-			tst_resm(TFAIL, "%s: Incorrect modes 0%03o, "
-				 "Expected 0%03o", TESTDIR, dir_mode,
-				 PERMS & ~S_ISGID);
-		else
-			tst_resm(TPASS,
-				 "Functionality of chmod(%s, %#o) successful",
-					 TESTDIR, PERMS);
+	TEST(chmod(TESTDIR, PERMS));
+	if (TST_RET == -1) {
+		tst_res(TFAIL, "chmod(%s, %#o) failed", TESTDIR, PERMS);
 	}
 
-	cleanup();
-	tst_exit();
+	/* Get the directory information using stat(2) and compare with
+	 * expected results.
+	 */
+	SAFE_STAT(TESTDIR, &stat_buf);
+	dir_mode = stat_buf.st_mode;
+	if ((PERMS & ~S_ISGID) != dir_mode) {
+		tst_res(TFAIL, "%s: Incorrect modes 0%03o, "
+				"Expected 0%03o", TESTDIR, dir_mode,
+				PERMS & ~S_ISGID);
+	} else {
+		tst_res(TPASS, "Functionality of chmod(%s, %#o) successful",
+				TESTDIR, PERMS);
+	}
 }
 
-/*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and cd to it.
- *  Create a test directory under temporary directory.
-//wjh we are root so do we really need this kluged helper program?
- *  Invoke setuid to root program to modify group ownership
- *  on test directory.
- */
 void setup(void)
 {
 	struct passwd *nobody_u;
-	struct group *bin_group;
+	struct group *bin_gr;
 
-	tst_require_root();
-
-	TEST_PAUSE;
-
-	tst_tmpdir();
-
-	nobody_u = getpwnam("nobody");
-	if (nobody_u == NULL)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "getpwnam(\"nobody\") failed");
-
-	bin_group = getgrnam("bin");
-	if (bin_group == NULL)
-		tst_brkm(TBROK | TERRNO, cleanup, "getgrnam(\"bin\") failed");
+	nobody_u = SAFE_GETPWNAM("nobody");
+	bin_gr = SAFE_GETGRNAM("bin");
 
 	/*
 	 * Create a test directory under temporary directory with specified
 	 * mode permissions and change the gid of test directory to nobody's
 	 * gid.
 	 */
-	SAFE_MKDIR(cleanup, TESTDIR, MODE_RWX);
-
+	SAFE_MKDIR(TESTDIR, MODE_RWX);
 	if (setgroups(1, &nobody_u->pw_gid) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "setgroups to nobody's gid failed");
+		tst_brk(TBROK | TERRNO, "setgroups to nobody's gid failed");
 
-	SAFE_CHOWN(cleanup, TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
+	SAFE_CHOWN(TESTDIR, nobody_u->pw_uid, bin_gr->gr_gid);
 
 	/* change to nobody:nobody */
-	if (setegid(nobody_u->pw_gid) == -1 || seteuid(nobody_u->pw_uid) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "Couldn't switch to nobody:nobody");
+	SAFE_SETEGID(nobody_u->pw_gid);
+	SAFE_SETEUID(nobody_u->pw_uid);
 }
 
-void cleanup(void)
+void cleanup_chmod(void)
 {
-	if (setegid(0) == -1)
-		tst_resm(TWARN | TERRNO, "setegid(0) failed");
-	if (seteuid(0) == -1)
-		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
-
-	tst_rmdir();
+	/* Reset the uid / gid to root */
+	SAFE_SETEGID(0);
+	SAFE_SETEUID(0);
 }
+
+static struct tst_test test = {
+	.needs_root 	= 1,
+	.needs_tmpdir 	= 1,
+	.setup 		= setup,
+	.test_all 	= test_chmod,
+	.cleanup 	= cleanup_chmod,
+};