Re: [LTP] [PATCH 1/2] mount/mount03.c: clean up

chrubis
2013-07-09T14:40:41Z

Hi!
> Clean up mount03.c:
> xxx func() -> xxx func(void)
>
> clean up setup()
>
>
> Signed-off-by: DAN LI <li.dan@cn.fujitsu.com>

Sorry it took longer than usuall.

> testcases/kernel/syscalls/mount/mount03.c | 47 ++++++++++---------------------
> 1 file changed, 15 insertions(+), 32 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/mount/mount03.c b/testcases/kernel/syscalls/mount/mount03.c
> index b1aa71f..8b2d3e4 100644
> --- a/testcases/kernel/syscalls/mount/mount03.c
> +++ b/testcases/kernel/syscalls/mount/mount03.c
> @@ -85,6 +85,7 @@ static int setup_uid(void);
> char *TCID = "mount03";
> int TST_TOTAL = 6;
>
> +#define EXEC_COMMAND "cp"
> #define DEFAULT_FSTYPE "ext2"
> #define TEMP_FILE "temp_file"
> #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
> @@ -106,7 +107,6 @@ static char read_buffer[BUFSIZ];
> 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,
> @@ -375,7 +375,7 @@ int test_rwflag(int i, int cnt)
> }
>
> /* setup_uid() - performs setup for NOUID test */
> -int setup_uid()
> +int setup_uid(void)
> {
> int pid, status;
> char command[PATH_MAX];
> @@ -387,7 +387,8 @@ int setup_uid()
> return 1;
> case 0:
> /* Put command into string */
> - sprintf(command, "%s %s %s", cmd, testhome_path, path_name);
> + sprintf(command, "%s %s %s",
> + EXEC_COMMAND, testhome_path, path_name);
>
> /* Run command to cp file to right spot */
> if (system(command) == 0)

What about using SAFE_CP() from safe_file_ops.h?

> @@ -410,25 +411,17 @@ int setup_uid()
> }
> }
>
> -void setup()
> +void setup(void)
> {
> - int fd;
> char path[PATH_MAX];
> - char *test_home;
> struct stat setuid_test_stat;
>
> tst_sig(FORK, DEF_HANDLER, cleanup);
>
> - /* Check whether we are root */
> - if (geteuid() != 0) {
> - free(fs_type);
> - tst_brkm(TBROK, NULL, "Test must be run as root");
> - }
> + tst_require_root(NULL);
>
> tst_tmpdir();
>
> - test_home = get_current_dir_name();
> -
> sprintf(mntpoint, "mnt_%d", getpid());

This getpid() shouldn't be needed as we are in the test tmpdir anyway.

> if (mkdir(mntpoint, DIR_MODE))
> @@ -444,38 +437,28 @@ void setup()
> path_name, DIR_MODE);
>
> snprintf(file, PATH_MAX, "%s/setuid_test", path_name);
> - fd = open(file, O_CREAT | O_TRUNC, S_IRWXU);
> - if (fd == -1)
> + if (open(file, O_CREAT | O_TRUNC, S_IRWXU) == -1)
> tst_brkm(TBROK, cleanup, "open file failed");
> - close(fd);

Why removing the close(fd)?

> - if (stat(file, &setuid_test_stat) < 0) {
> + 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)
> - tst_brkm(TBROK, cleanup,
> - "chown for setuid_test failed");
> -
> - if (setuid_test_stat.st_mode != SUID_MODE &&
> - chmod(file, SUID_MODE) < 0)
> - tst_brkm(TBROK, cleanup,
> - "setuid for setuid_test failed");
> - }
> +
> + if (setuid_test_stat.st_mode != SUID_MODE && chmod(file, SUID_MODE) < 0)
> + tst_brkm(TBROK, cleanup,
> + "setuid for setuid_test failed");
^
This could now be one line statement if I'm
not mistaken

> /*
> * under temporary directory
> */
> + strcpy(testhome_path, file);
> 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");
>
> TEST_PAUSE;
>
> }
>
> -void cleanup()
> +void cleanup(void)
> {
> free(fs_type);
>
> @@ -487,7 +470,7 @@ void cleanup()
> /*
> * issue a help message
> */
> -void help()
> +void help(void)
> {
> printf("-T type : specifies the type of filesystem to be mounted."
> " Default ext2.\n");
> --

Also please make all internal functions static.

And pretty please remove the absurd fs_type flag allocation.

--
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!