Re: [LTP] [PATCH v3] ioctl_fiemap01: New test for fiemap ioctl()

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> +#define TMPDIR "mntdir"
very nit: We usually use MNTPOINT for .mntpoint
#define MNTPOINT   "mntpoint"

(I even tried to get rid of these defines:
https://patchwork.ozlabs.org/project/ltp/list/?series=393028 .)

> +#define TESTFILE "testfile"
> +#define NUM_EXTENT 3
> +
> +static void print_extens(struct fiemap *fiemap)
> +{
> +	tst_res(TDEBUG, "File extent count: %u", fiemap->fm_mapped_extents);
> +
> +	for (unsigned int i = 0; i < fiemap->fm_mapped_extents; ++i) {
> +		tst_res(TDEBUG, "Extent %u: Logical offset: %llu, Physical offset: %llu, flags: %u, Length: %llu",
> +				i + 1,
> +				fiemap->fm_extents[i].fe_logical,
> +				fiemap->fm_extents[i].fe_physical,
> +				fiemap->fm_extents[i].fe_flags,
> +				fiemap->fm_extents[i].fe_length);
> +	}
> +}
> +
> +static void check_extent(struct fiemap *fiemap, unsigned int fm_mapped_extents, int index_extents, int fe_flags, unsigned int min_fe_physical, unsigned int fe_length)
> +{
> +	TST_EXP_EXPR(fiemap->fm_mapped_extents == fm_mapped_extents,
> +		"Check extent fm_mapped_extents is %d", fm_mapped_extents);
nit: space here (and below) would be more readable for me.
> +	TST_EXP_EXPR(fiemap->fm_extents[index_extents].fe_flags & fe_flags,
> +		"Check fe_flags is %d", fe_flags);
> +	TST_EXP_EXPR(fiemap->fm_extents[index_extents].fe_physical >= min_fe_physical,
> +		"Check fe_physical > %d", min_fe_physical);
> +	TST_EXP_EXPR(fiemap->fm_extents[index_extents].fe_length == fe_length,
> +		"Check fe_length is %d", fe_length);
> +}
> +
> +static void verify_ioctl(void)
> +{
> +	int fd;
> +	struct fiemap *fiemap;
> +	struct statvfs fs_info;
> +	unsigned long blk_size;
> +
> +	SAFE_CHDIR(TMPDIR);
> +	fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644);
> +
> +	TST_EXP_PASS(statvfs(".", &fs_info));

I'd here jut use:
if (statvfs(".", &fs_info) != 0)
	tst_brk(TBROK, "statvfs failed");

Why? IMHO all TEST() and TST_EXP_PASS() should be used for the subject of
testing.

(We don't have safe_statvfs(), but it'd be needed just on 3 places.)

> +	blk_size = fs_info.f_bsize;
> +
> +	fiemap = SAFE_MALLOC(sizeof(struct fiemap) + sizeof(struct fiemap_extent) * NUM_EXTENT);
> +	fiemap->fm_start = 0;
> +	fiemap->fm_length = ~0ULL;
> +	fiemap->fm_extent_count = 1;
> +
> +	fiemap->fm_flags =  -1;
nit: fiemap->fm_flags = -1;

> +	TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR);
> +
> +	fiemap->fm_flags =  0;
> +	TST_EXP_PASS(ioctl(fd, FS_IOC_FIEMAP, fiemap));
> +	print_extens(fiemap);
> +	TST_EXP_EXPR(fiemap->fm_mapped_extents == 0,
> +		"Check extent fm_mapped_extents is 0");
> +
> +	char *buf = SAFE_MALLOC(blk_size);
> +
> +	SAFE_WRITE(SAFE_WRITE_ANY, fd, buf, blk_size);
> +	fiemap->fm_flags = FIEMAP_FLAG_SYNC;
> +	TST_EXP_PASS(ioctl(fd, FS_IOC_FIEMAP, fiemap));
> +	print_extens(fiemap);
> +	check_extent(fiemap, 1, 0, FIEMAP_EXTENT_LAST, 1, blk_size);
> +
> +	fiemap->fm_extent_count = NUM_EXTENT;
> +	SAFE_LSEEK(fd, 2 * blk_size, SEEK_SET);
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, blk_size);
> +	SAFE_LSEEK(fd, 4 * blk_size, SEEK_SET);
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, blk_size);
> +	TST_EXP_PASS(ioctl(fd, FS_IOC_FIEMAP, fiemap));
> +	print_extens(fiemap);
> +	check_extent(fiemap, NUM_EXTENT, NUM_EXTENT - 1, FIEMAP_EXTENT_LAST, 1, blk_size);
> +
> +	free(buf);
> +	free(fiemap);
> +	SAFE_CLOSE(fd);
> +	unlink(TESTFILE);

SAFE_UNLINK(TESTFILE);

> +}
> +
> +static struct tst_test test = {
> +	.mount_device = 1,
> +	.mntpoint = TMPDIR,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const[]) {
> +		"exfat", "vfat", "fuse", "ntfs", "tmpfs", NULL

Why do you whitelist fuse? Which filesystem under fuse does not work?

> +	},
> +	.test_all = verify_ioctl,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
needs_tmpdir is not needed (you use .all_filesystems). You would find that when
generating docparse.

Kind regards,
Petr

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