Re: [LTP] [PATCH v3] unlinkat: Add negative tests for unlinkat

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
Can we move the three negative tests from the unlikat01.c here as well,
so that we have all the negative tests in a single place?

> +static void setup(void)
> +{
> +	int attr;
> +
> +	pw = SAFE_GETPWNAM("nobody");
> +
> +	SAFE_MKDIR(DIR_EACCES_NOWRITE, 0777);
> +	SAFE_TOUCH(DIR_EACCES_NOWRITE "/" TEST_EACCES, 0777, NULL);
> +	SAFE_CHMOD(DIR_EACCES_NOWRITE, 0555);

Can't we pass the 0555 directly to the SAFE_TOUCH()?

> +	SAFE_MKDIR(DIR_EACCES_NOSEARCH, 0777);
> +	SAFE_TOUCH(DIR_EACCES_NOSEARCH "/" TEST_EACCES, 0777, NULL);
> +	SAFE_CHMOD(DIR_EACCES_NOSEARCH, 0666);

Here as well?

> +	SAFE_MKDIR(DIR_NORMAL, 0777);
> +	SAFE_TOUCH(DIR_NORMAL "/" TEST_NORMAL, 0777, NULL);
> +	SAFE_TOUCH(DIR_NORMAL "/" TEST_EFAULT, 0777, NULL);
> +
> +	SAFE_MKDIR(DIR_NORMAL "/" DIR_EISDIR, 0777);
> +
> +	memset(longfilename, '1', PATH_MAX + 1);
> +
> +	SAFE_TOUCH(DIR_NORMAL "/" DIR_ENOTDIR, 0777, NULL);
> +
> +	fd_immutable = SAFE_OPEN(DIR_NORMAL "/" TEST_EPERM_IMMUTABLE,
> +			O_CREAT, 0777);
> +	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> +	attr |= FS_IMMUTABLE_FL;
> +	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> +	SAFE_CLOSE(fd_immutable);

This should be put into a function

static void set_fs_flags(int fd, int flags); and possibly put into the
test library, because this pattern is repeated in several tests.

> +	fd_append_only = SAFE_OPEN(DIR_NORMAL "/" TEST_EPERM_APPEND_ONLY,
> +			O_CREAT, 0777);
> +	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> +	attr |= FS_APPEND_FL;
> +	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> +	SAFE_CLOSE(fd_append_only);
> +
> +	SAFE_TOUCH(DIR_ENOTDIR2, 0777, NULL);
> +}
> +
> +static void cleanup(void)
> +{
> +	int attr;
> +
> +	fd_immutable = SAFE_OPEN(DIR_NORMAL "/" TEST_EPERM_IMMUTABLE,
> +			O_RDONLY, 0777);
> +	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> +	attr &= ~FS_IMMUTABLE_FL;
> +	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> +	SAFE_CLOSE(fd_immutable);

Same here, this should be put into clear_fs_flags(int fd, int flags)
function.

Or maybe just one function for both:

tst_change_fs_flags(int fd, int flags, bool set);

Where the set will switch between set and clear operations.

> +	fd_append_only = SAFE_OPEN(DIR_NORMAL "/" TEST_EPERM_APPEND_ONLY,
> +			O_RDONLY, 0777);
> +	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> +	attr &= ~FS_APPEND_FL;
> +	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> +	SAFE_CLOSE(fd_append_only);
> +}
> +
> +static void do_unlinkat(struct test_case_t *tc)
> +{
> +	int attr;
> +	char fullpath[PATH_MAX];
> +	int dirfd = open(tc->dirname, O_DIRECTORY);
> +
> +	if (dirfd < 0) {
> +		if (tc->expected_errno != EBADF) {
> +			/* Special situation: dirfd refers to a file */
> +			if (errno == ENOTDIR)
> +				dirfd = SAFE_OPEN(tc->dirname, O_APPEND);
> +			else {
> +				tst_res(TFAIL | TERRNO, "Cannot open dirfd");
> +				return;
> +			}
> +		}
> +	}

Can't we pass the flags in the testcase structure as well? That way we
would do just:

	dirfd = open(tc->dirname, tc->open_flags);

> +	TST_EXP_FAIL(unlinkat(dirfd, tc->filename, tc->flags),
> +		tc->expected_errno,
> +		"%s", tc->desc);
> +
> +	/* If unlinkat() succeeded unexpectedly, test file should be restored */
> +	if (!TST_RET) {
> +		snprintf(fullpath, sizeof(fullpath), "%s/%s", tc->dirname,
> +			tc->filename);
> +		if (tc->fd) {
> +			*(tc->fd) = SAFE_OPEN(fullpath, O_CREAT, 0600);
> +			if (tc->ioctl_flag) {
> +				SAFE_IOCTL(*(tc->fd), FS_IOC_GETFLAGS, &attr);
> +				attr |= tc->ioctl_flag;
> +				SAFE_IOCTL(*(tc->fd), FS_IOC_SETFLAGS, &attr);
> +			}
> +			SAFE_CLOSE(*(tc->fd));
> +		} else {
> +			SAFE_TOUCH(fullpath, 0777, 0);
> +		}
> +	}

Hmm, I'm not sure if this recovery is worth the extra code.

> +	if (dirfd > 0)
> +		SAFE_CLOSE(dirfd);
> +}
> +
> +static void verify_unlinkat(unsigned int i)
> +{
> +	struct test_case_t *tc = &tcases[i];
> +	pid_t pid;
> +
> +	if (tc->user) {
> +		pid = SAFE_FORK();
> +		if (!pid) {
> +			SAFE_SETUID(pw->pw_uid);
> +			do_unlinkat(tc);
> +			exit(0);
> +		}
> +		SAFE_WAITPID(pid, NULL, 0);
> +	} else {
> +		do_unlinkat(tc);
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.cleanup = cleanup,
> +	.test = verify_unlinkat,
> +	.needs_rofs = 1,
> +	.mntpoint = DIR_EROFS,
> +	.needs_root = 1,
> +	.forks_child = 1,
> +};
> -- 
> 2.39.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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