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

Petr Vorel
2024-02-28T17:07:54Z

Hi Wei,

> Fixes: #535

> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c
> new file mode 100644
> index 000000000..a626bb03c
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify basic fiemap ioctl
> + *
nit: missing space and dot at the end.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/fiemap.h>
> +#include <stdlib.h>
> +
> +#include "tst_test.h"
> +
> +#define TESTFILE "testfile"
> +#define NUM_EXTENT 2
> +#define FILE_OFFSET ((rand() % 8 + 2) * getpagesize())
> +
> +static char *buf;
> +
> +static void print_extens(struct fiemap *fiemap)
> +{
> +
nit: please no space above.
> + tst_res(TDEBUG, "File extent count: %u", fiemap->fm_mapped_extents);
nit: please add space here (readability)
> + 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 verify_ioctl(void)
> +{
> + int fd;
> + struct fiemap *fiemap;
> +
> + fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644);
> +
> + 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;
> + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR);

I get on Tumbleweed with 6.8.0-rc4-1.g9b23bf2-default EOPNOTSUPP:
ioctl_fiemap01.c:52: TFAIL: ioctl(fd, FS_IOC_FIEMAP, fiemap) expected EBADR: EOPNOTSUPP (95)
What is the missing dependency for FS_IOC_FIEMAP? Or is there wrong fiemap use
which causes that?

> +
> + fiemap->fm_flags = 0;
> + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap);
And on the same kernel another EOPNOTSUPP:

ioctl_fiemap01.c:55: TBROK: ioctl(3,((((2U|1U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((11)) << 0) | ((((sizeof(struct fiemap)))) << ((0+8)+8)))),...) failed: EOPNOTSUPP (95)
> + print_extens(fiemap);
> + if (fiemap->fm_mapped_extents == 0)
> + tst_res(TPASS, "Check fiemap iotct zero fm_mapped_extents pass");
s/iotct/ioctl/

> + else
> + tst_res(TFAIL, "Check fiemap iotct zero fm_mapped_extents failed");
s/iotct/ioctl/
> +
> + SAFE_WRITE(SAFE_WRITE_ANY, fd, buf, getpagesize());
> + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap);
> + print_extens(fiemap);
Maybe print only on failure?
> + if ((fiemap->fm_mapped_extents == 1) && (fiemap->fm_extents[0].fe_physical == 0))
NOTE: brackets are not needed (== has higher precedence than &&), thus:
if (fiemap->fm_mapped_extents == 1 && fiemap->fm_extents[0].fe_physical == 0)

> + tst_res(TPASS, "Check fiemap iotct one fm_mapped_extents pass");
s/iotct/ioctl/
> + else
> + tst_res(TFAIL, "Check fiemap iotct one fm_mapped_extents failed");
s/iotct/ioctl/
> +
> + fiemap->fm_flags = FIEMAP_FLAG_SYNC;
> + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap);
> + print_extens(fiemap);
> + if ((fiemap->fm_mapped_extents == 1) &&
nit: again == does not need to be in ()
> + (fiemap->fm_extents[0].fe_flags == FIEMAP_EXTENT_LAST) &&
> + (fiemap->fm_extents[0].fe_physical > 0) &&
> + (fiemap->fm_extents[0].fe_length == (__u64)getpagesize()))
> + tst_res(TPASS, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags pass");
s/iotct/ioctl/
> + else
> + tst_res(TFAIL, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags failed");
s/iotct/ioctl/
There are 4x checks, I guess instead it would be worth to write what exactly failed.

> +
> + fiemap->fm_extent_count = NUM_EXTENT;
> + srand(time(NULL));
> + SAFE_LSEEK(fd, FILE_OFFSET, SEEK_SET);
> + SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, getpagesize());
> + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap);
> + print_extens(fiemap);
> + if ((fiemap->fm_mapped_extents == NUM_EXTENT) &&
nit: If this check was repeated more than twice, I would put it into separate
function.
> + (fiemap->fm_extents[NUM_EXTENT - 1].fe_flags == FIEMAP_EXTENT_LAST) &&
> + (fiemap->fm_extents[NUM_EXTENT - 1].fe_physical > 0) &&
> + (fiemap->fm_extents[NUM_EXTENT - 1].fe_length == (__u64)getpagesize()))
> + tst_res(TPASS, "Check fiemap iotct multiple fm_mapped_extents pass");
> + else
> + tst_res(TFAIL, "Check fiemap iotct multiple fm_mapped_extents failed");
> +
> + free(fiemap);
> + SAFE_CLOSE(fd);
> + unlink(TESTFILE);
> +}
...

Kind regards,
Petr