Re: [LTP] [PATCH v1] chmod02.c: Block mode changes of symlinks

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

...
>  testcases/kernel/syscalls/chmod/chmod02.c  | 68 ++++++++++++++++++++++
nit: original chmod02.c was removed in eab36ea01, now you are using this
filename again. I don't remember if there is any consensus about reusing test
name (maybe it's ok), but I would prefer to create new file (i.e. chmod08.c),
because reusing names can lead to confusion (people may not notice when looking
into git history that these 2 tests are completely unrelated. If nothing,
different GPL version can be taken by mistake (e.g. original chmod02.c used GPL
2 only license).

...
> +/*\
> + * [Description]
> + *
> + * Test for kernel commit 5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
> + * "attr: block mode changes of symlinks".
nit: 5d1f903f75a8 ("attr: block mode changes of symlinks")
> + *
nit: please remove this blank line above.
> + */
> +
> +#include "lapi/fcntl.h"
> +#include "tst_test.h"
> +
> +#define MODE 0644
> +#define TESTFILE "testfile"
> +#define TESTFILE_SYMLINK "testfile_symlink"
> +
> +static void run(void)
> +{
> +	struct stat stat_file, stat_sym;
> +	int mode = 0;
> +	char fd_path[100];
> +
> +	int fd = SAFE_OPEN(TESTFILE_SYMLINK, O_PATH | O_NOFOLLOW);
> +
> +	sprintf(fd_path, "/proc/self/fd/%d", fd);
> +
> +	TST_EXP_FAIL(chmod(fd_path, mode), ENOTSUP, "chmod(%s, %04o)",
> +			TESTFILE_SYMLINK, mode);
> +
> +	SAFE_STAT(TESTFILE, &stat_file);
> +	SAFE_LSTAT(TESTFILE_SYMLINK, &stat_sym);
> +
> +	stat_file.st_mode &= ~S_IFREG;
> +	stat_sym.st_mode &= ~S_IFLNK;
> +
> +	if (stat_file.st_mode == (unsigned int)mode) {
> +		tst_res(TFAIL, "stat(%s) mode=%04o",
> +				TESTFILE, stat_file.st_mode);
> +	} else {
> +		tst_res(TPASS, "stat(%s) mode=%04o",
> +				TESTFILE, stat_file.st_mode);
> +	}
Maybe using TST_EXP_EXPR() to not repeat yourself?
> +
> +	if (stat_sym.st_mode == (unsigned int)mode) {
> +		tst_res(TFAIL, "stat(%s) mode=%04o",
> +				TESTFILE_SYMLINK, stat_sym.st_mode);
> +	} else {
> +		tst_res(TPASS, "stat(%s) mode=%04o",
> +				TESTFILE_SYMLINK, stat_sym.st_mode);
> +	}
And here as well?

Missing SAFE_CLOSE(fd); causes EMFILE (Too many open files) failure on high -i
(e.g. -i 1100).

> +}
> +
> +static void setup(void)
> +{
> +	SAFE_TOUCH(TESTFILE, MODE, NULL);
> +	SAFE_SYMLINK(TESTFILE, TESTFILE_SYMLINK);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = run,
> +	.needs_tmpdir = 1,
> +	.min_kver = "6.6",
Looking into kernel commit [1] it's in VFS therefore it should be safe to test
it on single filesystem which is in TMPDIR (i.e. not using .all_filesystems).

But "If this causes any regressions" and "It's a simple patch that can be easily
reverted." suggests that we should think twice when not running it with
.all_filesystems (if used, exfat and vfat fails with EPERM, but it works
on NTFS).

Kind regards,
Petr

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956

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