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

Petr Vorel
2024-04-03T09:28:48Z

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