From: Petr Vorel <pvorel@suse.cz>
To: Wei Gao <wegao@suse.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v3] ioctl_fiemap01: New test for fiemap ioctl()
Date: Wed, 3 Apr 2024 11:28:27 +0200 [thread overview]
Message-ID: <20240403092827.GA419563@pevik> (raw)
In-Reply-To: <20240331021720.9527-1-wegao@suse.com>
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
next prev parent reply other threads:[~2024-04-03 9:28 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 5:43 [LTP] [PATCH v1] ioctl_fiemap01: New test for fiemap ioctl() Wei Gao via ltp
2024-01-15 15:23 ` Cyril Hrubis
2024-01-18 7:32 ` [LTP] [PATCH v2] " Wei Gao via ltp
2024-02-28 17:07 ` Petr Vorel
2024-03-29 8:28 ` Wei Gao via ltp
2024-03-29 21:32 ` Petr Vorel
2024-03-31 2:15 ` Wei Gao via ltp
2024-03-31 2:17 ` [LTP] [PATCH v3] " Wei Gao via ltp
2024-04-03 9:28 ` Petr Vorel [this message]
2024-04-15 10:17 ` Wei Gao via ltp
2024-04-15 12:17 ` Petr Vorel
2024-04-15 11:46 ` [LTP] [PATCH v4] " Wei Gao via ltp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240403092827.GA419563@pevik \
--to=pvorel@suse.cz \
--cc=ltp@lists.linux.it \
--cc=wegao@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.