[LTP] [PATCH] mount03: Convert to new API

[Petr Vorel]

Hi Chen,

quite nice work, but more cleanup is needed.

>  testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++--------------
...
>  #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 SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)

@Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
like permission warnings. Do we want to follow? Or should we remove these from
our checkpatch.pl fork (we use constants in many places)?

$ make check-mount03
mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'.
mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'.
mount03.c:50: WARNING: static char array declaration should probably be static const char
mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.

...
> +#define MNTPOINT        "mntpoint"
...
> +static char nobody_uid[] = "nobody";
I'd remove constant and use "nobody" in the setup (the same like creat08.c).
If we want to have it defined, I'd use #define (like you use it in MNTPOINT).

Also your code produces warning:
mount03.c:50: WARNING: static char array declaration should probably be static const char

...
> +static int test_ms_nosuid(void)
...
> +	int pid, status;
pid_t pid; // don't use int for pid please.

This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need
switch.

For a readability, your code (most of inherited from the original) is:
static int test_ms_nosuid(void)
{
	int pid, status;

	switch (pid = SAFE_FORK()) {
	case -1:
		tst_brk(TFAIL | TERRNO, "fork failed");
		break;
	case 0:
		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);

		execlp(file, basename(file), NULL);
		exit(1);
	default:
		waitpid(pid, &status, 0);
		if (WIFEXITED(status)) {
			/* reset the setup_uid */
			if (status)
				return 0;
		}
		return 1;
	}
	return 0;
}

* I'm not sure why exit(1),
* we have SAFE_EXECLP. Therefore something like:


static void test_ms_nosuid(void)
{
	int status;
	pid_t pid;

	pid = SAFE_FORK();

	if (!pid) {
		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
		SAFE_EXECLP(file, basename(file), NULL);
	}

	SAFE_WAITPID(pid, &status, 0);
}

But SAFE_WAITPID() would not allow to print TPASS/TFAIL.

> -/*
> - * test_rwflag(int i, int cnt)
> - * Validate the mount system call for rwflags.
> - */
> -int test_rwflag(int i, int cnt)
> +static void test_rwflag(int i)
Instead of a crazy long switch which is in the original code, we prefer to
use functions which are put into struct tcase via function pointer. See e.g.
mq_open01.c for example.


...
> +		snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
> +		TEST(test_ms_nosuid());
Please don't reuse TEST() macro for normal function. Just call it:

> +		if (TST_RET == 0)
> +			tst_res(TPASS, "mount(2) passed with flag MS_NOSUID");
> +		else
> +			tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID");
There is TST_EXP_PASS() macro:
TST_EXP_PASS(test_ms_nosuid())
if (!TST_PASS)
		return;

But, please don't reuse TEST() macro for normal function. These are meant to be
for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make it
void.

...
> +static void setup(void)
> +{
> +	struct stat file_stat = {0};

> +	ltpuser = getpwnam(nobody_uid);
ltpuser = SAFE_GETPWNAM("nobody");
(or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c)

> +	if (ltpuser == NULL)
> +		tst_res(TFAIL, "getpwnam failed");
> +	snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);

> +	SAFE_STAT(MNTPOINT, &file_stat);
>  	if (file_stat.st_mode != SUID_MODE &&
> -	    chmod(file, SUID_MODE) < 0)
> -		tst_brkm(TBROK, cleanup,
> -			 "setuid for setuid_test failed");
> -	SAFE_UMOUNT(cleanup, mntpoint);
> -
> -	TEST_PAUSE;
> +	    chmod(MNTPOINT, SUID_MODE) < 0)
> +		tst_res(TFAIL, "setuid for setuid_test failed");
Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a
reason [1]:
1) tst_brk quits testing, which is usually required in setup function
2) TBROK is usually used instead of TFAIL in setup function

TBROK: Something has failed in test preparation phase.
TFAIL: Test has failed.

(This description should have been in LTP Test Writing Guidelines [2],
because it applies to both C and shell API.)


...
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.format_device = 1,
> +	.resource_files = resource_files,

Please inline resource files like:
	.resource_files = (const char *const[]) {
		"mount03_setuid_test",
		NULL,
	},

It produces "mount03_setuid_test" in doc [3]:

| .resource_files | mount03_setuid_test |

But using .resource_files = resource_files produces just "resource_files":
| .resource_files | resource_files |

(to make doc locally: cd metadata && make # result is in ../docparse/metadata.html)

> +	.forks_child = 1,
> +	.mntpoint = MNTPOINT,
> +	.skip_filesystems = (const char *const []){"fuse", NULL},
Why you introduced skipping on FUSE? I don't see it in the old code, nothing
mentioned in commit message).

Also when I test it for all filesystems (which could be a good idea for mount
test) it fails for vfat, exfat and ntfs:

	.skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL},
	.all_filesystems = 1,

Skipping just fuse does not help in my case:

tst_test.c:1599: TINFO: Testing on ntfs
tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts=''
The partition start sector was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
The number of sectors per track was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
The number of heads was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
To boot from a device, Windows needs the 'partition start sector', the 'sectors per track' and the 'number of heads' to be set.
Windows will not be able to boot from this device.
mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected succeeded
tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL
mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22)

tst_test.c:1599: TINFO: Testing on exfat
tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts=''
mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30)
mount03.c:112: TBROK: mknod() failed: EPERM (1)

tst_test.c:1599: TINFO: Testing on vfat
tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts=''
mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30)
mount03.c:112: TBROK: mknod() failed: EPERM (1)

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interface
[2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
[3] http://linux-test-project.github.io/metadata/metadata.stable.html