mount/mount03.c: cleanup

PATCH Superseded DAN LI 2 participants 6 messages May 13, 2013

lore

Share & tools

lore

DAN LI

author

2013-05-13 02:44 UTC #0 lore raw

PATCH 1/2 mount/mount03.c: cleanup

1. Remove useless comments

2. Revise code to follow ltp-code-style

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 208 +++++++++++++-----------------
 1 file changed, 89 insertions(+), 119 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +89 ~~-119~~

index 04570b9..ea4b0e9 100644
@@ -14,10 +14,8 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
  */
-/**************************************************************************
- *
- *    TEST IDENTIFIER	: mount03
- *
+
+/*
  *    EXECUTED BY	: root / superuser
  *
  *    TEST TITLE	: Test for checking mount(2) flags

@@ -26,10 +24,6 @@
  *
  *    AUTHOR		: Nirmala Devi Dhanasekar <[email protected]>
  *
- *    SIGNALS
- * 	Uses SIGUSR1 to pause before test if option set.
- * 	(See the parse_opts(3) man page).
- *
  *    DESCRIPTION
  *	Check for basic mount(2) system call flags.
  *

@@ -42,13 +36,13 @@
  *	5) MS_REMOUNT - alter flags of a mounted FS.
  *	6) MS_NOSUID - ignore suid and sgid bits.
  *
- * 	Setup:
+ *	Setup:
  *	  Setup signal handling.
  *	  Create a mount point.
  *	  Pause for SIGUSR1 if option specified.
  *
- * 	Test:
- *	 Loop if the proper options are given.
+ *	Test:
+ *	  Loop if the proper options are given.
  *	  Execute mount system call for each flag
  *	  Validate each flag setting. if validation fails
  *		Delete the mount point.

@@ -56,22 +50,10 @@
  *	  Delete the mount point.
  *	  Otherwise, Issue a PASS message.
  *
- * 	Cleanup:
- * 	  Print errno log and/or timing stats if options given
+ *	Cleanup:
+ *	  Print errno log and/or timing stats if options given
  *	  Delete the temporary directory(s)/file(s) created.
  *
- * USAGE:  <for command-line>
- *  mount03 [-T type] -D device [-e] [-i n] [-I x] [-p x] [-t]
- *			where,  -T type : specifies the type of filesystem to
- *					  be mounted. Default ext2.
- *				-D device : device to be mounted.
- *				-e   : Turn on errno logging.
- *				-i n : Execute test n times.
- *				-I x : Execute test for x seconds.
- *				-p   : Pause for SIGUSR1 before starting
- *				-P x : Pause for x seconds between iterations.
- *				-t   : Turn on syscall timing.
- *
  * RESTRICTIONS
  *	test must run with the -D option
  *	test doesn't support -c option to run it in parallel, as mount

@@ -102,30 +84,29 @@ static int setup_uid(void);
 
 char *TCID = "mount03";
 int TST_TOTAL = 6;
-extern char **environ;		/* pointer to this processes env */
 
 #define DEFAULT_FSTYPE	"ext2"
 #define TEMP_FILE	"temp_file"
 #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH)
+#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|\
+			 S_IXGRP|S_IROTH|S_IXOTH)
 #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
 
-static char *Fstype;
+static char *fs_type;
 
 static char mntpoint[20];
 static char *fstype;
 static char *device;
-static int Tflag = 0;
-static int Dflag = 0;
-
-static char write_buffer[BUFSIZ];	/* buffer used to write data to file */
-static char read_buffer[BUFSIZ];	/* buffer used to read data from file */
-static int fildes;		/* file descriptor for temporary file */
-static char *Cmd_buffer[3];	/* Buffer to hold command string */
-static char Path_name[PATH_MAX];	/* Buffer to hold command string */
-static char testhome_path[PATH_MAX];	/* Test home Path                */
-static char file[PATH_MAX];	/* Temporary file                */
-static char cmd[] = "cp";
+static int tflag;
+static int dflag;
+
+static char write_buffer[BUFSIZ];
+static char read_buffer[BUFSIZ];
+static int fildes;
+static char path_name[PATH_MAX];
+static char testhome_path[PATH_MAX];
+static char file[PATH_MAX];
+static char *cmd = "cp";
 
 long rwflags[] = {
 	MS_RDONLY,

@@ -136,41 +117,41 @@ long rwflags[] = {
 	MS_NOSUID,
 };
 
-static option_t options[] = {	/* options supported by mount03 test */
-	{"T:", &Tflag, &fstype},	/* -T type of filesystem        */
-	{"D:", &Dflag, &device},	/* -D device used for mounting  */
+static option_t options[] = {
+	{"T:", &tflag, &fstype},
+	{"D:", &dflag, &device},
 	{NULL, NULL, NULL}
 };
 
-int main(int ac, char **av)
+int main(int argc, char *argv[])
 {
 	int lc, i;
 	char *msg;
 
-	if ((msg = parse_opts(ac, av, options, &help)) != NULL)
+	msg = parse_opts(argc, argv, options, &help);
+	if (msg != NULL)
 		tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
 
 	/* Check for mandatory option of the testcase */
-	if (!Dflag) {
+	if (!dflag)
 		tst_brkm(TBROK, NULL,
 			 "you must specify the device used for mounting with -D "
 			 "option");
-	}
 
-	if (Tflag) {
-		Fstype = (char *)malloc(strlen(fstype) + 1);
-		if (Fstype == NULL) {
+	if (tflag) {
+		fs_type = (char *)malloc(strlen(fstype) + 1);
+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		Fstype[strlen(fstype)] = '\0';
-		strncpy(Fstype, fstype, strlen(fstype));
+
+		fs_type[strlen(fstype)] = '\0';
+		strncpy(fs_type, fstype, strlen(fstype));
 	} else {
-		Fstype = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
-		if (Fstype == NULL) {
+		fs_type = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		strncpy(Fstype, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
-		Fstype[strlen(DEFAULT_FSTYPE)] = '\0';
+
+		strncpy(fs_type, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
+		fs_type[strlen(DEFAULT_FSTYPE)] = '\0';
 	}
 
 	if (STD_COPIES != 1) {

@@ -189,7 +170,8 @@ int main(int ac, char **av)
 		for (i = 0; i < TST_TOTAL; ++i) {
 
 			/* Call mount(2) */
-			TEST(mount(device, mntpoint, Fstype, rwflags[i], NULL));
+			TEST(mount(device, mntpoint, fs_type,
+						rwflags[i], NULL));
 
 			/* check return code */
 			if (TEST_RETURN != 0) {

@@ -198,22 +180,20 @@ int main(int ac, char **av)
 			}
 
 			/* Validate the rwflag */
-			if (test_rwflag(i, lc) == 1) {
+			if (test_rwflag(i, lc) == 1)
 				tst_resm(TFAIL, "mount(2) failed while"
 					 " validating %ld", rwflags[i]);
-			} else {
+			else
 				tst_resm(TPASS, "mount(2) passed with "
 					 "rwflag = %ld", rwflags[i]);
-			}
+
 			TEST(umount(mntpoint));
-			if (TEST_RETURN != 0) {
+			if (TEST_RETURN != 0)
 				tst_brkm(TBROK | TTERRNO, cleanup,
 					 "umount(2) failed for %s", mntpoint);
-			}
 		}
 	}
 
-	/* cleanup and exit */
 	cleanup();
 
 	tst_exit();

@@ -234,8 +214,9 @@ int test_rwflag(int i, int cnt)
 	case 0:
 		/* Validate MS_RDONLY flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			if (errno == EROFS) {
 				return 0;
 			} else {

@@ -249,10 +230,11 @@ int test_rwflag(int i, int cnt)
 	case 1:
 		/* Validate MS_NODEV flag of mount call */
 
-		snprintf(file, PATH_MAX, "%smynod_%d_%d", Path_name, getpid(),
+		snprintf(file, PATH_MAX, "%smynod_%d_%d", path_name, getpid(),
 			 cnt);
 		if (mknod(file, S_IFBLK | 0777, 0) == 0) {
-			if ((fd = open(file, O_RDWR, S_IRWXU)) == -1) {
+			fd = open(file, O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				if (errno == EACCES) {
 					return 0;
 				} else {

@@ -271,8 +253,9 @@ int test_rwflag(int i, int cnt)
 	case 2:
 		/* Validate MS_NOEXEC flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp1", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp1", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);

@@ -288,9 +271,9 @@ int test_rwflag(int i, int cnt)
 		strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz");
 
 		/* Creat a temporary file under above directory */
-		snprintf(file, PATH_MAX, "%s%s", Path_name, TEMP_FILE);
-		if ((fildes = open(file, O_RDWR | O_CREAT, FILE_MODE))
-		    == -1) {
+		snprintf(file, PATH_MAX, "%s%s", path_name, TEMP_FILE);
+		fildes = open(file, O_RDWR | O_CREAT, FILE_MODE);
+		if (fildes == -1) {
 			tst_resm(TWARN | TERRNO,
 				 "open(%s, O_RDWR|O_CREAT, %#o) failed",
 				 file, FILE_MODE);

@@ -333,14 +316,14 @@ int test_rwflag(int i, int cnt)
 	case 4:
 		/* Validate MS_REMOUNT flag of mount call */
 
-		TEST(mount(device, mntpoint, Fstype, MS_REMOUNT, NULL));
+		TEST(mount(device, mntpoint, fs_type, MS_REMOUNT, NULL));
 		if (TEST_RETURN != 0) {
 			tst_resm(TWARN | TTERRNO, "mount(2) failed to remount");
 			return 1;
 		} else {
-			snprintf(file, PATH_MAX, "%stmp2", Path_name);
-			if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU))
-			    == -1) {
+			snprintf(file, PATH_MAX, "%stmp2", path_name);
+			fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				tst_resm(TWARN, "open(%s) on readonly "
 					 "filesystem passed", file);
 				return 1;

@@ -356,23 +339,23 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TBROK | TERRNO, "setup_uid failed");
 			return 1;
 		}
-		switch (pid = fork()) {
+		pid = fork();
+		switch (pid) {
 		case -1:
 			tst_resm(TBROK | TERRNO, "fork failed");
 			return 1;
 		case 0:
-			snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
-			if (chmod(file, SUID_MODE) != 0) {
+			snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+			if (chmod(file, SUID_MODE) != 0)
 				tst_resm(TWARN, "chmod(%s, %#o) failed",
 					 file, SUID_MODE);
-			}
 
 			ltpuser = getpwnam(nobody_uid);
-			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1) {
+			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1)
 				tst_resm(TWARN | TERRNO,
 					 "seteuid() failed to change euid to %d",
 					 ltpuser->pw_uid);
-			}
+
 			execlp(file, basename(file), NULL);
 			exit(1);
 		default:

@@ -395,24 +378,21 @@ int setup_uid()
 	int pid, status;
 	char command[PATH_MAX];
 
-	switch (pid = fork()) {
+	pid = fork();
+	switch (pid) {
 	case -1:
 		tst_resm(TWARN | TERRNO, "fork failed");
 		return 1;
 	case 0:
-		Cmd_buffer[0] = cmd;
-		Cmd_buffer[1] = testhome_path;
-		Cmd_buffer[2] = Path_name;
-
 		/* Put command into string */
-		sprintf(command, "%s %s %s", cmd, testhome_path, Path_name);
+		sprintf(command, "%s %s %s", cmd, testhome_path, path_name);
 
 		/* Run command to cp file to right spot */
-		if (system(command) == 0) {
+		if (system(command) == 0)
 			execlp(file, basename(file), NULL);
-		} else {
+		else
 			printf("call to %s failed\n", command);
-		}
+
 		exit(1);
 	default:
 		waitpid(pid, &status, 0);

@@ -428,17 +408,16 @@ int setup_uid()
 	}
 }
 
-/* setup() - performs all ONE TIME setup for this test */
 void setup()
 {
-	char *test_home;	/* variable to hold TESTHOME env */
+	char *test_home;
 	struct stat setuid_test_stat;
 
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 
 	/* Check whether we are root */
 	if (geteuid() != 0) {
-		free(Fstype);
+		free(fs_type);
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}

@@ -451,38 +430,37 @@ void setup()
 	/* Unique mount point */
 	(void)sprintf(mntpoint, "mnt_%d", getpid());
 
-	if (mkdir(mntpoint, DIR_MODE)) {
+	if (mkdir(mntpoint, DIR_MODE))
 		tst_brkm(TBROK | TERRNO, cleanup, "mkdir(%s, %#o) failed",
 			 mntpoint, DIR_MODE);
-	}
+
 	/* Get the current working directory of the process */
-	if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
+	if (getcwd(path_name, sizeof(path_name)) == NULL)
 		tst_brkm(TBROK, cleanup, "getcwd failed");
-	}
-	if (chmod(Path_name, DIR_MODE) != 0) {
+
+	if (chmod(path_name, DIR_MODE) != 0)
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
-			 Path_name, DIR_MODE);
-	}
-	snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
+			 path_name, DIR_MODE);
+
+	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {
 		if ((setuid_test_stat.st_uid || setuid_test_stat.st_gid) &&
-		    chown(file, 0, 0) < 0) {
+		    chown(file, 0, 0) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "chown for setuid_test failed");
-		}
+
 		if (setuid_test_stat.st_mode != SUID_MODE &&
-		    chmod(file, SUID_MODE) < 0) {
+		    chmod(file, SUID_MODE) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "setuid for setuid_test failed");
-		}
 	}
 
 	/*
 	 * under temporary directory
 	 */
-	snprintf(Path_name, PATH_MAX, "%s/%s/", Path_name, mntpoint);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
 
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

@@ -491,18 +469,10 @@ void setup()
 
 }
 
-/*
- *cleanup() -  performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- */
 void cleanup()
 {
-	free(Fstype);
+	free(fs_type);
 
-	/*
-	 * print timing stats if that option was specified.
-	 * print errno log if that option was specified.
-	 */
 	TEST_CLEANUP;
 
 	tst_rmdir();

@@ -514,6 +484,6 @@ void cleanup()
 void help()
 {
 	printf("-T type	  : specifies the type of filesystem to be mounted."
-	       " Default ext2. \n");
-	printf("-D device : device used for mounting \n");
+	       "Default ext2.\n");
+	printf("-D device : device used for mounting\n");
 }

Signature · 12 lines

1.8.3

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

DAN LI

author

2013-05-13 03:14 UTC #1 lore raw

PATCH 2/2 mount/mount03.c: fix several issues

Make following fixes:
1. In setup():
Create the file before calling stat(file). Call get_current_dir_name() after tst_tmpdir(). Fix incorrect using of snprintf().

2. For MS_NOEXEC test, change judgement method to
If errno is set to EACCES after execlp(exec-file), this case passes.

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +17 -8

index ea4b0e9..1dc39f2 100644
@@ -206,7 +206,7 @@ int main(int argc, char *argv[])
 
 int test_rwflag(int i, int cnt)
 {
-	int fd, pid, status;
+	int ret, fd, pid, status;
 	char nobody_uid[] = "nobody";
 	struct passwd *ltpuser;

@@ -259,7 +259,9 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);
-			execlp(file, basename(file), NULL);
+			ret = execlp(file, basename(file), NULL);
+			if ((ret == -1) && (errno == EACCES))
+				return 0;
 		}
 		return 1;
 	case 3:

@@ -410,6 +412,8 @@ int setup_uid()
 
 void setup()
 {
+	int fd;
+	char path[PATH_MAX];
 	char *test_home;
 	struct stat setuid_test_stat;

@@ -421,12 +425,12 @@ void setup()
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}
 
-	/* Test home directory */
-	test_home = get_current_dir_name();
-
 	/* make a temp directory */
 	tst_tmpdir();
 
+	/* Test home directory */
+	test_home = get_current_dir_name();
+
 	/* Unique mount point */
 	(void)sprintf(mntpoint, "mnt_%d", getpid());

@@ -442,7 +446,12 @@ void setup()
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
 			 path_name, DIR_MODE);
 
-	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+	snprintf(file, PATH_MAX, "%s/setuid_test", path_name);
+	fd = open(file, O_CREAT | O_TRUNC);
+	if (fd == -1)
+		tst_brkm(TBROK, cleanup, "open file failed");
+	close(fd);
+
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {

@@ -460,8 +469,8 @@ void setup()
 	/*
 	 * under temporary directory
 	 */
-	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
-
+	strncpy(path, path_name, PATH_MAX);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

Signature · 13 lines

1.8.3


------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Eryu Guan

2013-05-13 08:41 UTC #2 lore raw

Re: [LTP] [PATCH 2/2]mount/mount03.c: fix several issues

Hi,

For patch summary
[LTP] [PATCH 2/2]mount/mount03.c: fix several issues
^^ ^^ one extra space
^^ one space is needed here

Quoted 8 lines · DAN LI

Make following fixes:
1. In setup():
Create the file before calling stat(file). Call get_current_dir_name() after tst_tmpdir(). Fix incorrect using of snprintf().
2. For MS_NOEXEC test, change judgement method to
If errno is set to EACCES after execlp(exec-file), this case passes.

I don't think you need to indent commit message like this.

Quoted 20 lines

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +1 -1

index ea4b0e9..1dc39f2 100644
@@ -206,7 +206,7 @@ int main(int argc, char *argv[])
 
 int test_rwflag(int i, int cnt)
 {
-	int fd, pid, status;
+	int ret, fd, pid, status;
 	char nobody_uid[] = "nobody";
 	struct passwd *ltpuser;

testcases/kernel/syscalls/mount/mount03.c int test_rwflag(int i, int cnt) · line 264

Quoted 7 lines

testcases/kernel/syscalls/mount/mount03.c +3 -1

@@ -259,7 +259,9 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);
-			execlp(file, basename(file), NULL);
+			ret = execlp(file, basename(file), NULL);
+			if ((ret == -1) && (errno == EACCES))
+				return 0;

Should we give out a TWARN or TBROK when execlp failed?

Quoted 13 lines

testcases/kernel/syscalls/mount/mount03.c +2 -0

 		}
 		return 1;
 	case 3:

@@ -410,6 +412,8 @@ int setup_uid()
 
 void setup()
 {
+	int fd;
+	char path[PATH_MAX];
 	char *test_home;
 	struct stat setuid_test_stat;

testcases/kernel/syscalls/mount/mount03.c void setup() · line 431

Quoted 10 lines

testcases/kernel/syscalls/mount/mount03.c +1 -3

@@ -421,12 +425,12 @@ void setup()
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}
 
-	/* Test home directory */
-	test_home = get_current_dir_name();
-
 	/* make a temp directory */
 	tst_tmpdir();
 
+	/* Test home directory */

Unwanted comment I think

testcases/kernel/syscalls/mount/mount03.c void setup() · line 434

Quoted 3 lines

testcases/kernel/syscalls/mount/mount03.c +2 -0
+	test_home = get_current_dir_name();
+
 	/* Unique mount point */

here too

testcases/kernel/syscalls/mount/mount03.c void setup() · line 435

Quoted 1 line

testcases/kernel/syscalls/mount/mount03.c +0 -0
 	(void)sprintf(mntpoint, "mnt_%d", getpid());

        ^^^^^^ this is useless

All the three changes should go in the first cleanup patch.

Thanks,
Eryu Guan

Quoted 40 lines

testcases/kernel/syscalls/mount/mount03.c +8 -3

@@ -442,7 +446,12 @@ void setup()
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
 			 path_name, DIR_MODE);
 
-	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+	snprintf(file, PATH_MAX, "%s/setuid_test", path_name);
+	fd = open(file, O_CREAT | O_TRUNC);
+	if (fd == -1)
+		tst_brkm(TBROK, cleanup, "open file failed");
+	close(fd);
+
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {

@@ -460,8 +469,8 @@ void setup()
 	/*
 	 * under temporary directory
 	 */
-	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
-
+	strncpy(path, path_name, PATH_MAX);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

Signature · 13 lines

1.8.3


------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book "Graph Databases" is the definitive new guide to graph databases and their applications. This 200-page book is written by three acclaimed leaders in the field. The early access version is available now. Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

DAN LI

author

2013-05-13 11:01 UTC #3 lore raw

Re: [LTP] [PATCH 2/2]mount/mount03.c: fix several issues

Quoted 6 lines · Eryu Guan

Hi,
For patch summary
[LTP] [PATCH 2/2]mount/mount03.c: fix several issues
^^ ^^ one extra space
^^ one space is needed here

Ok.

Quoted 11 lines · DAN LI

Quoted 8 lines
Make following fixes:
1. In setup():
Create the file before calling stat(file). Call get_current_dir_name() after tst_tmpdir(). Fix incorrect using of snprintf().
2. For MS_NOEXEC test, change judgement method to
If errno is set to EACCES after execlp(exec-file), this case passes.
I don't think you need to indent commit message like this.

Agreed.

Quoted 19 lines

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +1 -1

index ea4b0e9..1dc39f2 100644
@@ -206,7 +206,7 @@ int main(int argc, char *argv[])
 
 int test_rwflag(int i, int cnt)
 {
-	int fd, pid, status;
+	int ret, fd, pid, status;
 	char nobody_uid[] = "nobody";
 	struct passwd *ltpuser;

Quoted 9 lines

testcases/kernel/syscalls/mount/mount03.c int test_rwflag(int i, int cnt) · line 264
Quoted 8 lines
testcases/kernel/syscalls/mount/mount03.c +3 -1
@@ -259,7 +259,9 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);
-			execlp(file, basename(file), NULL);
+			ret = execlp(file, basename(file), NULL);
+			if ((ret == -1) && (errno == EACCES))
+				return 0;
Should we give out a TWARN or TBROK when execlp failed?

This failure is the expected result. So, an error message is may not necessary.

Quoted 17 lines

testcases/kernel/syscalls/mount/mount03.c +2 -0

 		}
 		return 1;
 	case 3:

@@ -410,6 +412,8 @@ int setup_uid()
 
 void setup()
 {
+	int fd;
+	char path[PATH_MAX];
 	char *test_home;
 	struct stat setuid_test_stat;

@@ -421,12 +425,12 @@ void setup()
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}

Quoted 8 lines

testcases/kernel/syscalls/mount/mount03.c void setup() · line 431
Quoted 7 lines
testcases/kernel/syscalls/mount/mount03.c +1 -3
-	/* Test home directory */
-	test_home = get_current_dir_name();
-
 	/* make a temp directory */
 	tst_tmpdir();
 
+	/* Test home directory */
Unwanted comment I think

Agreed.

Quoted 5 lines

testcases/kernel/syscalls/mount/mount03.c void setup() · line 434
Quoted 4 lines
testcases/kernel/syscalls/mount/mount03.c +2 -0
+	test_home = get_current_dir_name();
+
 	/* Unique mount point */
here too

Agreed.

Quoted 3 lines

testcases/kernel/syscalls/mount/mount03.c void setup() · line 435
Quoted 2 lines
testcases/kernel/syscalls/mount/mount03.c +0 -0
 	(void)sprintf(mntpoint, "mnt_%d", getpid());
        ^^^^^^ this is useless

Agreed.

Quoted 2 lines

All the three changes should go in the first cleanup patch.

Ok.

I'll fix all of them in coming V2.

Thanks again for reviewing.
DAN LI

Quoted 33 lines

Thanks,
Eryu Guan

Quoted 30 lines

testcases/kernel/syscalls/mount/mount03.c +8 -3

@@ -442,7 +446,12 @@ void setup()
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
 			 path_name, DIR_MODE);
 
-	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+	snprintf(file, PATH_MAX, "%s/setuid_test", path_name);
+	fd = open(file, O_CREAT | O_TRUNC);
+	if (fd == -1)
+		tst_brkm(TBROK, cleanup, "open file failed");
+	close(fd);
+
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {

@@ -460,8 +469,8 @@ void setup()
 	/*
 	 * under temporary directory
 	 */
-	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
-
+	strncpy(path, path_name, PATH_MAX);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

Signature · 1 line

1.8.3

Quoted 11 lines

Quoted 10 lines
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book "Graph Databases" is the definitive new guide to graph databases and their applications. This 200-page book is written by three acclaimed leaders in the field. The early access version is available now. Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Eryu Guan

2013-05-13 08:26 UTC #4 lore raw

In reply to DAN LI

Quoted 81 lines · DAN LI

1. Remove useless comments

2. Revise code to follow ltp-code-style

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 208 +++++++++++++-----------------
 1 file changed, 89 insertions(+), 119 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +7 ~~-25~~

index 04570b9..ea4b0e9 100644
@@ -14,10 +14,8 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
  */
-/**************************************************************************
- *
- *    TEST IDENTIFIER	: mount03
- *
+
+/*
  *    EXECUTED BY	: root / superuser
  *
  *    TEST TITLE	: Test for checking mount(2) flags

@@ -26,10 +24,6 @@
  *
  *    AUTHOR		: Nirmala Devi Dhanasekar <[email protected]>
  *
- *    SIGNALS
- * 	Uses SIGUSR1 to pause before test if option set.
- * 	(See the parse_opts(3) man page).
- *
  *    DESCRIPTION
  *	Check for basic mount(2) system call flags.
  *

@@ -42,13 +36,13 @@
  *	5) MS_REMOUNT - alter flags of a mounted FS.
  *	6) MS_NOSUID - ignore suid and sgid bits.
  *
- * 	Setup:
+ *	Setup:
  *	  Setup signal handling.
  *	  Create a mount point.
  *	  Pause for SIGUSR1 if option specified.
  *
- * 	Test:
- *	 Loop if the proper options are given.
+ *	Test:
+ *	  Loop if the proper options are given.
  *	  Execute mount system call for each flag
  *	  Validate each flag setting. if validation fails
  *		Delete the mount point.

@@ -56,22 +50,10 @@
  *	  Delete the mount point.
  *	  Otherwise, Issue a PASS message.
  *
- * 	Cleanup:
- * 	  Print errno log and/or timing stats if options given
+ *	Cleanup:
+ *	  Print errno log and/or timing stats if options given
  *	  Delete the temporary directory(s)/file(s) created.
  *
- * USAGE:  <for command-line>
- *  mount03 [-T type] -D device [-e] [-i n] [-I x] [-p x] [-t]
- *			where,  -T type : specifies the type of filesystem to
- *					  be mounted. Default ext2.
- *				-D device : device to be mounted.
- *				-e   : Turn on errno logging.
- *				-i n : Execute test n times.
- *				-I x : Execute test for x seconds.
- *				-p   : Pause for SIGUSR1 before starting
- *				-P x : Pause for x seconds between iterations.
- *				-t   : Turn on syscall timing.
- *
  * RESTRICTIONS
  *	test must run with the -D option
  *	test doesn't support -c option to run it in parallel, as mount

testcases/kernel/syscalls/mount/mount03.c static int setup_uid(void); · line 91

Quoted 10 lines

testcases/kernel/syscalls/mount/mount03.c +1 -2

@@ -102,30 +84,29 @@ static int setup_uid(void);
 
 char *TCID = "mount03";
 int TST_TOTAL = 6;
-extern char **environ;		/* pointer to this processes env */
 
 #define DEFAULT_FSTYPE	"ext2"
 #define TEMP_FILE	"temp_file"
 #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH)
+#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|\

						       ^^^^ I'd like a space here, | \

Quoted 16 lines

testcases/kernel/syscalls/mount/mount03.c +2 -8

+			 S_IXGRP|S_IROTH|S_IXOTH)
 #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
 
-static char *Fstype;
+static char *fs_type;
 
 static char mntpoint[20];
 static char *fstype;
 static char *device;
-static int Tflag = 0;
-static int Dflag = 0;
-
-static char write_buffer[BUFSIZ];	/* buffer used to write data to file */
-static char read_buffer[BUFSIZ];	/* buffer used to read data from file */
-static int fildes;		/* file descriptor for temporary file */
-static char *Cmd_buffer[3];	/* Buffer to hold command string */

testcases/kernel/syscalls/mount/mount03.c static int setup_uid(void); · line 105

Quoted 10 lines

testcases/kernel/syscalls/mount/mount03.c +6 -4

-static char Path_name[PATH_MAX];	/* Buffer to hold command string */
-static char testhome_path[PATH_MAX];	/* Test home Path                */
-static char file[PATH_MAX];	/* Temporary file                */
-static char cmd[] = "cp";
+static int tflag;
+static int dflag;
+
+static char write_buffer[BUFSIZ];
+static char read_buffer[BUFSIZ];
+static int fildes;

move this up and put all int vars together?

Quoted 34 lines

testcases/kernel/syscalls/mount/mount03.c +11 -6

+static char path_name[PATH_MAX];
+static char testhome_path[PATH_MAX];
+static char file[PATH_MAX];
+static char *cmd = "cp";
 
 long rwflags[] = {
 	MS_RDONLY,

@@ -136,41 +117,41 @@ long rwflags[] = {
 	MS_NOSUID,
 };
 
-static option_t options[] = {	/* options supported by mount03 test */
-	{"T:", &Tflag, &fstype},	/* -T type of filesystem        */
-	{"D:", &Dflag, &device},	/* -D device used for mounting  */
+static option_t options[] = {
+	{"T:", &tflag, &fstype},
+	{"D:", &dflag, &device},
 	{NULL, NULL, NULL}
 };
 
-int main(int ac, char **av)
+int main(int argc, char *argv[])
 {
 	int lc, i;
 	char *msg;
 
-	if ((msg = parse_opts(ac, av, options, &help)) != NULL)
+	msg = parse_opts(argc, argv, options, &help);
+	if (msg != NULL)
 		tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
 
 	/* Check for mandatory option of the testcase */
-	if (!Dflag) {
+	if (!dflag)

testcases/kernel/syscalls/mount/mount03.c long rwflags[] = { · line 142

Quoted 10 lines

testcases/kernel/syscalls/mount/mount03.c +2 -4

 		tst_brkm(TBROK, NULL,
 			 "you must specify the device used for mounting with -D "
 			 "option");
-	}
 
-	if (Tflag) {
-		Fstype = (char *)malloc(strlen(fstype) + 1);
-		if (Fstype == NULL) {
+	if (tflag) {
+		fs_type = (char *)malloc(strlen(fstype) + 1);

^^^^^^^^ not sure if we really need a cast for malloc

Quoted 24 lines

testcases/kernel/syscalls/mount/mount03.c +9 -8

+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		Fstype[strlen(fstype)] = '\0';
-		strncpy(Fstype, fstype, strlen(fstype));
+
+		fs_type[strlen(fstype)] = '\0';
+		strncpy(fs_type, fstype, strlen(fstype));
 	} else {
-		Fstype = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
-		if (Fstype == NULL) {
+		fs_type = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		strncpy(Fstype, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
-		Fstype[strlen(DEFAULT_FSTYPE)] = '\0';
+
+		strncpy(fs_type, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
+		fs_type[strlen(DEFAULT_FSTYPE)] = '\0';
 	}
 
 	if (STD_COPIES != 1) {

testcases/kernel/syscalls/mount/mount03.c int main(int ac, char **av) · line 174

Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c +2 -1

@@ -189,7 +170,8 @@ int main(int ac, char **av)
 		for (i = 0; i < TST_TOTAL; ++i) {
 
 			/* Call mount(2) */
-			TEST(mount(device, mntpoint, Fstype, rwflags[i], NULL));
+			TEST(mount(device, mntpoint, fs_type,
+						rwflags[i], NULL));

indent like this?
TEST(mount(device, mntpoint, fs_type, rwflags[i],

				   NULL));

Quoted 253 lines

testcases/kernel/syscalls/mount/mount03.c +47 ~~-59~~

 			/* check return code */
 			if (TEST_RETURN != 0) {

@@ -198,22 +180,20 @@ int main(int ac, char **av)
 			}
 
 			/* Validate the rwflag */
-			if (test_rwflag(i, lc) == 1) {
+			if (test_rwflag(i, lc) == 1)
 				tst_resm(TFAIL, "mount(2) failed while"
 					 " validating %ld", rwflags[i]);
-			} else {
+			else
 				tst_resm(TPASS, "mount(2) passed with "
 					 "rwflag = %ld", rwflags[i]);
-			}
+
 			TEST(umount(mntpoint));
-			if (TEST_RETURN != 0) {
+			if (TEST_RETURN != 0)
 				tst_brkm(TBROK | TTERRNO, cleanup,
 					 "umount(2) failed for %s", mntpoint);
-			}
 		}
 	}
 
-	/* cleanup and exit */
 	cleanup();
 
 	tst_exit();

@@ -234,8 +214,9 @@ int test_rwflag(int i, int cnt)
 	case 0:
 		/* Validate MS_RDONLY flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			if (errno == EROFS) {
 				return 0;
 			} else {

@@ -249,10 +230,11 @@ int test_rwflag(int i, int cnt)
 	case 1:
 		/* Validate MS_NODEV flag of mount call */
 
-		snprintf(file, PATH_MAX, "%smynod_%d_%d", Path_name, getpid(),
+		snprintf(file, PATH_MAX, "%smynod_%d_%d", path_name, getpid(),
 			 cnt);
 		if (mknod(file, S_IFBLK | 0777, 0) == 0) {
-			if ((fd = open(file, O_RDWR, S_IRWXU)) == -1) {
+			fd = open(file, O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				if (errno == EACCES) {
 					return 0;
 				} else {

@@ -271,8 +253,9 @@ int test_rwflag(int i, int cnt)
 	case 2:
 		/* Validate MS_NOEXEC flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp1", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp1", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);

@@ -288,9 +271,9 @@ int test_rwflag(int i, int cnt)
 		strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz");
 
 		/* Creat a temporary file under above directory */
-		snprintf(file, PATH_MAX, "%s%s", Path_name, TEMP_FILE);
-		if ((fildes = open(file, O_RDWR | O_CREAT, FILE_MODE))
-		    == -1) {
+		snprintf(file, PATH_MAX, "%s%s", path_name, TEMP_FILE);
+		fildes = open(file, O_RDWR | O_CREAT, FILE_MODE);
+		if (fildes == -1) {
 			tst_resm(TWARN | TERRNO,
 				 "open(%s, O_RDWR|O_CREAT, %#o) failed",
 				 file, FILE_MODE);

@@ -333,14 +316,14 @@ int test_rwflag(int i, int cnt)
 	case 4:
 		/* Validate MS_REMOUNT flag of mount call */
 
-		TEST(mount(device, mntpoint, Fstype, MS_REMOUNT, NULL));
+		TEST(mount(device, mntpoint, fs_type, MS_REMOUNT, NULL));
 		if (TEST_RETURN != 0) {
 			tst_resm(TWARN | TTERRNO, "mount(2) failed to remount");
 			return 1;
 		} else {
-			snprintf(file, PATH_MAX, "%stmp2", Path_name);
-			if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU))
-			    == -1) {
+			snprintf(file, PATH_MAX, "%stmp2", path_name);
+			fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				tst_resm(TWARN, "open(%s) on readonly "
 					 "filesystem passed", file);
 				return 1;

@@ -356,23 +339,23 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TBROK | TERRNO, "setup_uid failed");
 			return 1;
 		}
-		switch (pid = fork()) {
+		pid = fork();
+		switch (pid) {
 		case -1:
 			tst_resm(TBROK | TERRNO, "fork failed");
 			return 1;
 		case 0:
-			snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
-			if (chmod(file, SUID_MODE) != 0) {
+			snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+			if (chmod(file, SUID_MODE) != 0)
 				tst_resm(TWARN, "chmod(%s, %#o) failed",
 					 file, SUID_MODE);
-			}
 
 			ltpuser = getpwnam(nobody_uid);
-			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1) {
+			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1)
 				tst_resm(TWARN | TERRNO,
 					 "seteuid() failed to change euid to %d",
 					 ltpuser->pw_uid);
-			}
+
 			execlp(file, basename(file), NULL);
 			exit(1);
 		default:

@@ -395,24 +378,21 @@ int setup_uid()
 	int pid, status;
 	char command[PATH_MAX];
 
-	switch (pid = fork()) {
+	pid = fork();
+	switch (pid) {
 	case -1:
 		tst_resm(TWARN | TERRNO, "fork failed");
 		return 1;
 	case 0:
-		Cmd_buffer[0] = cmd;
-		Cmd_buffer[1] = testhome_path;
-		Cmd_buffer[2] = Path_name;
-
 		/* Put command into string */
-		sprintf(command, "%s %s %s", cmd, testhome_path, Path_name);
+		sprintf(command, "%s %s %s", cmd, testhome_path, path_name);
 
 		/* Run command to cp file to right spot */
-		if (system(command) == 0) {
+		if (system(command) == 0)
 			execlp(file, basename(file), NULL);
-		} else {
+		else
 			printf("call to %s failed\n", command);
-		}
+
 		exit(1);
 	default:
 		waitpid(pid, &status, 0);

@@ -428,17 +408,16 @@ int setup_uid()
 	}
 }
 
-/* setup() - performs all ONE TIME setup for this test */
 void setup()
 {
-	char *test_home;	/* variable to hold TESTHOME env */
+	char *test_home;
 	struct stat setuid_test_stat;
 
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 
 	/* Check whether we are root */
 	if (geteuid() != 0) {
-		free(Fstype);
+		free(fs_type);
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}

@@ -451,38 +430,37 @@ void setup()
 	/* Unique mount point */
 	(void)sprintf(mntpoint, "mnt_%d", getpid());
 
-	if (mkdir(mntpoint, DIR_MODE)) {
+	if (mkdir(mntpoint, DIR_MODE))
 		tst_brkm(TBROK | TERRNO, cleanup, "mkdir(%s, %#o) failed",
 			 mntpoint, DIR_MODE);
-	}
+
 	/* Get the current working directory of the process */
-	if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
+	if (getcwd(path_name, sizeof(path_name)) == NULL)
 		tst_brkm(TBROK, cleanup, "getcwd failed");
-	}
-	if (chmod(Path_name, DIR_MODE) != 0) {
+
+	if (chmod(path_name, DIR_MODE) != 0)
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
-			 Path_name, DIR_MODE);
-	}
-	snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
+			 path_name, DIR_MODE);
+
+	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {
 		if ((setuid_test_stat.st_uid || setuid_test_stat.st_gid) &&
-		    chown(file, 0, 0) < 0) {
+		    chown(file, 0, 0) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "chown for setuid_test failed");
-		}
+
 		if (setuid_test_stat.st_mode != SUID_MODE &&
-		    chmod(file, SUID_MODE) < 0) {
+		    chmod(file, SUID_MODE) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "setuid for setuid_test failed");
-		}
 	}
 
 	/*
 	 * under temporary directory
 	 */
-	snprintf(Path_name, PATH_MAX, "%s/%s/", Path_name, mntpoint);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
 
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

@@ -491,18 +469,10 @@ void setup()
 
 }
 
-/*
- *cleanup() -  performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- */
 void cleanup()
 {
-	free(Fstype);
+	free(fs_type);
 
-	/*
-	 * print timing stats if that option was specified.
-	 * print errno log if that option was specified.
-	 */
 	TEST_CLEANUP;
 
 	tst_rmdir();

testcases/kernel/syscalls/mount/mount03.c void cleanup() · line 487

Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c +1 -2

@@ -514,6 +484,6 @@ void cleanup()
 void help()
 {
 	printf("-T type	  : specifies the type of filesystem to be mounted."
-	       " Default ext2. \n");
-	printf("-D device : device used for mounting \n");
+	       "Default ext2.\n");

^^ we need a space here otherwise the output will be:
"... to be mounted.Default ext2."
^^^^^ no space

Thanks for the cleanup!

Eryu Guan

Quoted 15 lines

testcases/kernel/syscalls/mount/mount03.c +1 -0

+	printf("-D device : device used for mounting\n");
 }

Signature · 12 lines

1.8.3

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

DAN LI

author

2013-05-13 10:39 UTC #5 lore raw

Quoted 87 lines · Eryu Guan

Quoted 86 lines · DAN LI

1. Remove useless comments

2. Revise code to follow ltp-code-style

Signed-off-by DAN LI <[email protected]>

---
 testcases/kernel/syscalls/mount/mount03.c | 208 +++++++++++++-----------------
 1 file changed, 89 insertions(+), 119 deletions(-)

testcases/kernel/syscalls/mount/mount03.c +7 ~~-26~~

index 04570b9..ea4b0e9 100644
@@ -14,10 +14,8 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
  */
-/**************************************************************************
- *
- *    TEST IDENTIFIER	: mount03
- *
+
+/*
  *    EXECUTED BY	: root / superuser
  *
  *    TEST TITLE	: Test for checking mount(2) flags

@@ -26,10 +24,6 @@
  *
  *    AUTHOR		: Nirmala Devi Dhanasekar <[email protected]>
  *
- *    SIGNALS
- * 	Uses SIGUSR1 to pause before test if option set.
- * 	(See the parse_opts(3) man page).
- *
  *    DESCRIPTION
  *	Check for basic mount(2) system call flags.
  *

@@ -42,13 +36,13 @@
  *	5) MS_REMOUNT - alter flags of a mounted FS.
  *	6) MS_NOSUID - ignore suid and sgid bits.
  *
- * 	Setup:
+ *	Setup:
  *	  Setup signal handling.
  *	  Create a mount point.
  *	  Pause for SIGUSR1 if option specified.
  *
- * 	Test:
- *	 Loop if the proper options are given.
+ *	Test:
+ *	  Loop if the proper options are given.
  *	  Execute mount system call for each flag
  *	  Validate each flag setting. if validation fails
  *		Delete the mount point.

@@ -56,22 +50,10 @@
  *	  Delete the mount point.
  *	  Otherwise, Issue a PASS message.
  *
- * 	Cleanup:
- * 	  Print errno log and/or timing stats if options given
+ *	Cleanup:
+ *	  Print errno log and/or timing stats if options given
  *	  Delete the temporary directory(s)/file(s) created.
  *
- * USAGE:  <for command-line>
- *  mount03 [-T type] -D device [-e] [-i n] [-I x] [-p x] [-t]
- *			where,  -T type : specifies the type of filesystem to
- *					  be mounted. Default ext2.
- *				-D device : device to be mounted.
- *				-e   : Turn on errno logging.
- *				-i n : Execute test n times.
- *				-I x : Execute test for x seconds.
- *				-p   : Pause for SIGUSR1 before starting
- *				-P x : Pause for x seconds between iterations.
- *				-t   : Turn on syscall timing.
- *
  * RESTRICTIONS
  *	test must run with the -D option
  *	test doesn't support -c option to run it in parallel, as mount

@@ -102,30 +84,29 @@ static int setup_uid(void);
 
 char *TCID = "mount03";
 int TST_TOTAL = 6;
-extern char **environ;		/* pointer to this processes env */

Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c static int setup_uid(void); · line 91

Quoted 5 lines

testcases/kernel/syscalls/mount/mount03.c +1 -1

 #define DEFAULT_FSTYPE	"ext2"
 #define TEMP_FILE	"temp_file"
 #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH)
+#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|\

						       ^^^^ I'd like a space here, | \

Agreed.

Quoted 20 lines

testcases/kernel/syscalls/mount/mount03.c +2 ~~-12~~

+			 S_IXGRP|S_IROTH|S_IXOTH)
 #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
 
-static char *Fstype;
+static char *fs_type;
 
 static char mntpoint[20];
 static char *fstype;
 static char *device;
-static int Tflag = 0;
-static int Dflag = 0;
-
-static char write_buffer[BUFSIZ];	/* buffer used to write data to file */
-static char read_buffer[BUFSIZ];	/* buffer used to read data from file */
-static int fildes;		/* file descriptor for temporary file */
-static char *Cmd_buffer[3];	/* Buffer to hold command string */
-static char Path_name[PATH_MAX];	/* Buffer to hold command string */
-static char testhome_path[PATH_MAX];	/* Test home Path                */
-static char file[PATH_MAX];	/* Temporary file                */
-static char cmd[] = "cp";

Quoted 8 lines

testcases/kernel/syscalls/mount/mount03.c static int setup_uid(void); · line 105
Quoted 6 lines
testcases/kernel/syscalls/mount/mount03.c +6 -0
+static int tflag;
+static int dflag;
+
+static char write_buffer[BUFSIZ];
+static char read_buffer[BUFSIZ];
+static int fildes;
move this up and put all int vars together?

Agreed.

Quoted 39 lines

testcases/kernel/syscalls/mount/mount03.c +11 -7

+static char path_name[PATH_MAX];
+static char testhome_path[PATH_MAX];
+static char file[PATH_MAX];
+static char *cmd = "cp";
 
 long rwflags[] = {
 	MS_RDONLY,

@@ -136,41 +117,41 @@ long rwflags[] = {
 	MS_NOSUID,
 };
 
-static option_t options[] = {	/* options supported by mount03 test */
-	{"T:", &Tflag, &fstype},	/* -T type of filesystem        */
-	{"D:", &Dflag, &device},	/* -D device used for mounting  */
+static option_t options[] = {
+	{"T:", &tflag, &fstype},
+	{"D:", &dflag, &device},
 	{NULL, NULL, NULL}
 };
 
-int main(int ac, char **av)
+int main(int argc, char *argv[])
 {
 	int lc, i;
 	char *msg;
 
-	if ((msg = parse_opts(ac, av, options, &help)) != NULL)
+	msg = parse_opts(argc, argv, options, &help);
+	if (msg != NULL)
 		tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
 
 	/* Check for mandatory option of the testcase */
-	if (!Dflag) {
+	if (!dflag)
 		tst_brkm(TBROK, NULL,
 			 "you must specify the device used for mounting with -D "
 			 "option");
-	}

Quoted 7 lines

testcases/kernel/syscalls/mount/mount03.c long rwflags[] = { · line 142
Quoted 5 lines
testcases/kernel/syscalls/mount/mount03.c +2 -3
-	if (Tflag) {
-		Fstype = (char *)malloc(strlen(fstype) + 1);
-		if (Fstype == NULL) {
+	if (tflag) {
+		fs_type = (char *)malloc(strlen(fstype) + 1);
^^^^^^^^ not sure if we really need a cast for malloc

It's a watch point in C++, but for C, maybe better to cut the cast.

Quoted 26 lines

testcases/kernel/syscalls/mount/mount03.c +9 -8

+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		Fstype[strlen(fstype)] = '\0';
-		strncpy(Fstype, fstype, strlen(fstype));
+
+		fs_type[strlen(fstype)] = '\0';
+		strncpy(fs_type, fstype, strlen(fstype));
 	} else {
-		Fstype = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
-		if (Fstype == NULL) {
+		fs_type = (char *)malloc(strlen(DEFAULT_FSTYPE) + 1);
+		if (fs_type == NULL)
 			tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
-		}
-		strncpy(Fstype, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
-		Fstype[strlen(DEFAULT_FSTYPE)] = '\0';
+
+		strncpy(fs_type, DEFAULT_FSTYPE, strlen(DEFAULT_FSTYPE));
+		fs_type[strlen(DEFAULT_FSTYPE)] = '\0';
 	}
 
 	if (STD_COPIES != 1) {

@@ -189,7 +170,8 @@ int main(int ac, char **av)
 		for (i = 0; i < TST_TOTAL; ++i) {

Quoted 7 lines

testcases/kernel/syscalls/mount/mount03.c int main(int ac, char **av) · line 174
Quoted 4 lines
testcases/kernel/syscalls/mount/mount03.c +2 -1
 			/* Call mount(2) */
-			TEST(mount(device, mntpoint, Fstype, rwflags[i], NULL));
+			TEST(mount(device, mntpoint, fs_type,
+						rwflags[i], NULL));
indent like this?
TEST(mount(device, mntpoint, fs_type, rwflags[i],
				   NULL));

Agreed.

Quoted 251 lines

testcases/kernel/syscalls/mount/mount03.c +47 ~~-59~~

 			/* check return code */
 			if (TEST_RETURN != 0) {

@@ -198,22 +180,20 @@ int main(int ac, char **av)
 			}
 
 			/* Validate the rwflag */
-			if (test_rwflag(i, lc) == 1) {
+			if (test_rwflag(i, lc) == 1)
 				tst_resm(TFAIL, "mount(2) failed while"
 					 " validating %ld", rwflags[i]);
-			} else {
+			else
 				tst_resm(TPASS, "mount(2) passed with "
 					 "rwflag = %ld", rwflags[i]);
-			}
+
 			TEST(umount(mntpoint));
-			if (TEST_RETURN != 0) {
+			if (TEST_RETURN != 0)
 				tst_brkm(TBROK | TTERRNO, cleanup,
 					 "umount(2) failed for %s", mntpoint);
-			}
 		}
 	}
 
-	/* cleanup and exit */
 	cleanup();
 
 	tst_exit();

@@ -234,8 +214,9 @@ int test_rwflag(int i, int cnt)
 	case 0:
 		/* Validate MS_RDONLY flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			if (errno == EROFS) {
 				return 0;
 			} else {

@@ -249,10 +230,11 @@ int test_rwflag(int i, int cnt)
 	case 1:
 		/* Validate MS_NODEV flag of mount call */
 
-		snprintf(file, PATH_MAX, "%smynod_%d_%d", Path_name, getpid(),
+		snprintf(file, PATH_MAX, "%smynod_%d_%d", path_name, getpid(),
 			 cnt);
 		if (mknod(file, S_IFBLK | 0777, 0) == 0) {
-			if ((fd = open(file, O_RDWR, S_IRWXU)) == -1) {
+			fd = open(file, O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				if (errno == EACCES) {
 					return 0;
 				} else {

@@ -271,8 +253,9 @@ int test_rwflag(int i, int cnt)
 	case 2:
 		/* Validate MS_NOEXEC flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp1", Path_name);
-		if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU)) == -1) {
+		snprintf(file, PATH_MAX, "%stmp1", path_name);
+		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+		if (fd == -1) {
 			tst_resm(TWARN | TERRNO, "opening %s failed", file);
 		} else {
 			close(fd);

@@ -288,9 +271,9 @@ int test_rwflag(int i, int cnt)
 		strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz");
 
 		/* Creat a temporary file under above directory */
-		snprintf(file, PATH_MAX, "%s%s", Path_name, TEMP_FILE);
-		if ((fildes = open(file, O_RDWR | O_CREAT, FILE_MODE))
-		    == -1) {
+		snprintf(file, PATH_MAX, "%s%s", path_name, TEMP_FILE);
+		fildes = open(file, O_RDWR | O_CREAT, FILE_MODE);
+		if (fildes == -1) {
 			tst_resm(TWARN | TERRNO,
 				 "open(%s, O_RDWR|O_CREAT, %#o) failed",
 				 file, FILE_MODE);

@@ -333,14 +316,14 @@ int test_rwflag(int i, int cnt)
 	case 4:
 		/* Validate MS_REMOUNT flag of mount call */
 
-		TEST(mount(device, mntpoint, Fstype, MS_REMOUNT, NULL));
+		TEST(mount(device, mntpoint, fs_type, MS_REMOUNT, NULL));
 		if (TEST_RETURN != 0) {
 			tst_resm(TWARN | TTERRNO, "mount(2) failed to remount");
 			return 1;
 		} else {
-			snprintf(file, PATH_MAX, "%stmp2", Path_name);
-			if ((fd = open(file, O_CREAT | O_RDWR, S_IRWXU))
-			    == -1) {
+			snprintf(file, PATH_MAX, "%stmp2", path_name);
+			fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
+			if (fd == -1) {
 				tst_resm(TWARN, "open(%s) on readonly "
 					 "filesystem passed", file);
 				return 1;

@@ -356,23 +339,23 @@ int test_rwflag(int i, int cnt)
 			tst_resm(TBROK | TERRNO, "setup_uid failed");
 			return 1;
 		}
-		switch (pid = fork()) {
+		pid = fork();
+		switch (pid) {
 		case -1:
 			tst_resm(TBROK | TERRNO, "fork failed");
 			return 1;
 		case 0:
-			snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
-			if (chmod(file, SUID_MODE) != 0) {
+			snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
+			if (chmod(file, SUID_MODE) != 0)
 				tst_resm(TWARN, "chmod(%s, %#o) failed",
 					 file, SUID_MODE);
-			}
 
 			ltpuser = getpwnam(nobody_uid);
-			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1) {
+			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1)
 				tst_resm(TWARN | TERRNO,
 					 "seteuid() failed to change euid to %d",
 					 ltpuser->pw_uid);
-			}
+
 			execlp(file, basename(file), NULL);
 			exit(1);
 		default:

@@ -395,24 +378,21 @@ int setup_uid()
 	int pid, status;
 	char command[PATH_MAX];
 
-	switch (pid = fork()) {
+	pid = fork();
+	switch (pid) {
 	case -1:
 		tst_resm(TWARN | TERRNO, "fork failed");
 		return 1;
 	case 0:
-		Cmd_buffer[0] = cmd;
-		Cmd_buffer[1] = testhome_path;
-		Cmd_buffer[2] = Path_name;
-
 		/* Put command into string */
-		sprintf(command, "%s %s %s", cmd, testhome_path, Path_name);
+		sprintf(command, "%s %s %s", cmd, testhome_path, path_name);
 
 		/* Run command to cp file to right spot */
-		if (system(command) == 0) {
+		if (system(command) == 0)
 			execlp(file, basename(file), NULL);
-		} else {
+		else
 			printf("call to %s failed\n", command);
-		}
+
 		exit(1);
 	default:
 		waitpid(pid, &status, 0);

@@ -428,17 +408,16 @@ int setup_uid()
 	}
 }
 
-/* setup() - performs all ONE TIME setup for this test */
 void setup()
 {
-	char *test_home;	/* variable to hold TESTHOME env */
+	char *test_home;
 	struct stat setuid_test_stat;
 
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 
 	/* Check whether we are root */
 	if (geteuid() != 0) {
-		free(Fstype);
+		free(fs_type);
 		tst_brkm(TBROK, NULL, "Test must be run as root");
 	}

@@ -451,38 +430,37 @@ void setup()
 	/* Unique mount point */
 	(void)sprintf(mntpoint, "mnt_%d", getpid());
 
-	if (mkdir(mntpoint, DIR_MODE)) {
+	if (mkdir(mntpoint, DIR_MODE))
 		tst_brkm(TBROK | TERRNO, cleanup, "mkdir(%s, %#o) failed",
 			 mntpoint, DIR_MODE);
-	}
+
 	/* Get the current working directory of the process */
-	if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
+	if (getcwd(path_name, sizeof(path_name)) == NULL)
 		tst_brkm(TBROK, cleanup, "getcwd failed");
-	}
-	if (chmod(Path_name, DIR_MODE) != 0) {
+
+	if (chmod(path_name, DIR_MODE) != 0)
 		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
-			 Path_name, DIR_MODE);
-	}
-	snprintf(file, PATH_MAX, "%ssetuid_test", Path_name);
+			 path_name, DIR_MODE);
+
+	snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
 	if (stat(file, &setuid_test_stat) < 0) {
 		tst_brkm(TBROK, cleanup, "stat for setuid_test failed");
 	} else {
 		if ((setuid_test_stat.st_uid || setuid_test_stat.st_gid) &&
-		    chown(file, 0, 0) < 0) {
+		    chown(file, 0, 0) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "chown for setuid_test failed");
-		}
+
 		if (setuid_test_stat.st_mode != SUID_MODE &&
-		    chmod(file, SUID_MODE) < 0) {
+		    chmod(file, SUID_MODE) < 0)
 			tst_brkm(TBROK, cleanup,
 				 "setuid for setuid_test failed");
-		}
 	}
 
 	/*
 	 * under temporary directory
 	 */
-	snprintf(Path_name, PATH_MAX, "%s/%s/", Path_name, mntpoint);
+	snprintf(path_name, PATH_MAX, "%s/%s/", path_name, mntpoint);
 
 	strcpy(testhome_path, test_home);
 	strcat(testhome_path, "/setuid_test");

@@ -491,18 +469,10 @@ void setup()
 
 }
 
-/*
- *cleanup() -  performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- */
 void cleanup()
 {
-	free(Fstype);
+	free(fs_type);
 
-	/*
-	 * print timing stats if that option was specified.
-	 * print errno log if that option was specified.
-	 */
 	TEST_CLEANUP;

Quoted 12 lines

testcases/kernel/syscalls/mount/mount03.c void cleanup() · line 487
Quoted 8 lines
testcases/kernel/syscalls/mount/mount03.c +1 -2
 	tst_rmdir();
@@ -514,6 +484,6 @@ void cleanup()
 void help()
 {
 	printf("-T type	  : specifies the type of filesystem to be mounted."
-	       " Default ext2. \n");
-	printf("-D device : device used for mounting \n");
+	       "Default ext2.\n");
^^ we need a space here otherwise the output will be:
"... to be mounted.Default ext2."
^^^^^ no space

But it seems that checkpatch.pl do not agree it.
I'will check it.

Thanks for reviewing.
DAN LI

Quoted 8 lines

Thanks for the cleanup!
Eryu Guan
Quoted 5 lines
testcases/kernel/syscalls/mount/mount03.c +1 -0
+	printf("-D device : device used for mounting\n");
 }
Signature · 1 line
1.8.3

Quoted 11 lines

Quoted 10 lines
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book "Graph Databases" is the definitive new guide to graph databases and their applications. This 200-page book is written by three acclaimed leaders in the field. The early access version is available now. Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list