Re: [LTP] [PATCH v5 2/2] ioctl_fiemap01: New test for fiemap ioctl()

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

...
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Please GPL-2.0-or-later.

> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
Please remove this [Description].
> + *
> + * Verify basic fiemap ioctl functionality, including:
There needs to be an empty line. NOTE: if you add list (numbered or not) blank
line is required otherwise it's everything inline. If you build the docs you
would see it yourself.

> + * - The ioctl returns EBADR if it receives invalid fm_flags.
> + * - 0 extents are reported for an empty file.
> + * - The ioctl correctly retrieves single and multiple extent mappings after writing to the file.
> + */
> +
> +#include <linux/fs.h>
I suppose we need <linux/fs.h>

> +#include <linux/fiemap.h>
> +#include <stdlib.h>
> +#include <sys/statvfs.h>
> +
> +#include "tst_test.h"
> +
> +#define MNTPOINT "mntpoint"
> +#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);
> +	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, ret;
> +	struct fiemap *fiemap;
> +	struct statvfs fs_info;
> +	unsigned long blk_size;
> +
> +	SAFE_CHDIR(MNTPOINT);
IMHO this will not work with -i2, it should be in the setup().

> +	fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644);
> +
> +	SAFE_STATVFS(".", &fs_info);

And probably these two as well.

> +
> +	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 = FIEMAP_FLAG_XATTR;
> +
> +	ret = ioctl(fd, FS_IOC_FIEMAP, fiemap);
> +	if (ret == -1) {
> +		if (errno == ENOTTY)
> +			tst_res(TCONF, "ioctl(FS_IOC_FIEMAP) not implemented");
I wonder if it's safe to put errno == ENOTTY check into SAFE_IOCTL().
We have similar checks in safe_socket() and other.

e.g.:
#define TTYPE (errno == ENOTTY ? TCONF : TBROK)

Maybe it's not safe, ENOTTY might be caused by some test error which deserves
TBROK:

       ENOTTY fd is not associated with a character special device.

       ENOTTY The specified operation does not apply to the kind of object that the file descriptor fd references.


If check used here here, it should be followed by tst_brk(), right?

> +	}
> +
> +	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,
> +		"Empty file should have 0 extends mapped");
> +
> +	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);
> +	SAFE_UNLINK(TESTFILE);
> +}
> +
> +static struct tst_test test = {
> +	.mount_device = 1,
> +	.mntpoint = MNTPOINT,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const[]) {
> +		"exfat", "vfat", "ntfs", "tmpfs", NULL

Is the function unimplemented on these (even on tmpfs)? I would expect that but
better to explain in the commit message why it's skipped.

Kind regards,
Petr

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