[1/3] Add ioctl_ficlone01 test

From: Andrea Cervesato <andrea.cervesato@suse.com>

This test verifies that ioctl() FICLONE feature clones file content
from one file to an another.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/lapi/fs.h                                 |   4 +
 runtest/syscalls                                  |   2 +
 testcases/kernel/syscalls/ioctl/.gitignore        |   1 +
 testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c | 116 ++++++++++++++++++++++
 4 files changed, 123 insertions(+)

Hi!
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Andrea Cervesato andrea.cervesato@suse.com
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test verifies that ioctl() FICLONE feature clones file content from
> + * one file to an another.
> + *
> + * [Algorithm]
> + *
> + * * populate source file
> + * * clone source content inside destination file
> + * * verify that source content has been cloned inside destination file
> + * * write a single byte inside destination file
> + * * verify that source content didn't change while destination did

This is very minor but I find dashes to be a better choice for lists
inside of C comments.

> + */
> +
> +#include "tst_test.h"
> +#include "lapi/fs.h"
> +
> +#define MNTPOINT "mnt"
> +#define SRCPATH MNTPOINT "/file0"
> +#define DSTPATH MNTPOINT "/file1"
> +
> +#define FILEDATA "qwerty"
> +#define FILESIZE sizeof(FILEDATA)
> +
> +static int src_fd = -1;
> +static int dst_fd = -1;
> +
> +static void run(void)
> +{
> +	char buff[FILESIZE];
> +	struct stat src_stat;
> +	struct stat dst_stat;
> +
> +	src_fd = SAFE_OPEN(SRCPATH, O_CREAT | O_RDWR, 0640);
> +	dst_fd = SAFE_OPEN(DSTPATH, O_CREAT | O_RDWR, 0640);
> +
> +	tst_res(TINFO, "Writing data inside src file");
> +
> +	SAFE_WRITE(1, src_fd, FILEDATA, FILESIZE);
> +	SAFE_FSYNC(src_fd);
> +
> +	TST_EXP_PASS(ioctl(dst_fd, FICLONE, src_fd));
> +	if (TST_RET == -1)
> +		return;
> +
> +	SAFE_FSYNC(dst_fd);
> +
> +	tst_res(TINFO, "Verifing that data is cloned between files");
> +
> +	SAFE_FSTAT(src_fd, &src_stat);
> +	SAFE_FSTAT(dst_fd, &dst_stat);
> +
> +	TST_EXP_EXPR(src_stat.st_ino != dst_stat.st_ino,
> +		"inode is different. %lu != %lu",
> +		src_stat.st_ino,
> +		dst_stat.st_ino);
> +
> +	TST_EXP_EQ_LI(src_stat.st_size, dst_stat.st_size);
> +
> +	SAFE_READ(0, dst_fd, buff, FILESIZE);
> +
> +	TST_EXP_EXPR(!strncmp(buff, FILEDATA, FILESIZE),
> +		"dst file has the src file content (\"%s\" - %ld bytes)",
> +		buff,
> +		FILESIZE);
> +
> +	tst_res(TINFO, "Writing a byte inside dst file");
> +
> +	SAFE_WRITE(SAFE_WRITE_ALL, dst_fd, "a", 1);
> +	SAFE_FSYNC(dst_fd);
> +
> +	tst_res(TINFO, "Verifing that src file content didn't change");
> +
> +	SAFE_FSTAT(src_fd, &src_stat);
> +	SAFE_FSTAT(dst_fd, &dst_stat);
> +
> +	TST_EXP_EQ_LI(dst_stat.st_size, src_stat.st_size + 1);
> +
> +	SAFE_READ(0, src_fd, buff, FILESIZE);
> +
> +	TST_EXP_EXPR(!strncmp(buff, FILEDATA, FILESIZE),
> +		"src file content didn't change");

So you append to the file but then only read the initial part? That does
not sound right. I guess that easiest solution is to seek to the start
of the file or od pwrite() so that we overwrite the first byte rather
than appending.

Or at least check the return value from the read.

> +	SAFE_CLOSE(src_fd);
> +	SAFE_CLOSE(dst_fd);
> +	src_fd = -1;
> +	dst_fd = -1;

This is not needed, the SAFE_CLOSE() macro sets the fd to -1.

> +	remove(SRCPATH);
> +	remove(DSTPATH);

Please use SAFE_UNLINK() instead.

> +}
> +
> +static void cleanup(void)
> +{
> +	if (src_fd != -1)
> +		SAFE_CLOSE(src_fd);
> +
> +	if (dst_fd != -1)
> +		SAFE_CLOSE(dst_fd);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.cleanup = cleanup,
> +	.min_kver = "4.5",
> +	.needs_root = 1,
> +	.mount_device = 1,
> +	.mntpoint = MNTPOINT,
> +	.dev_fs_type = "btrfs",


I suppose that we need .use_filesystems or similar and convert the
dev_fs_type to an array so that we can run this test on xfs as well...

> +};
> 
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Hi!

On 5/30/24 12:48, Cyril Hrubis wrote:
> Hi!
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024 Andrea Cervesato andrea.cervesato@suse.com
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * This test verifies that ioctl() FICLONE feature clones file content from
>> + * one file to an another.
>> + *
>> + * [Algorithm]
>> + *
>> + * * populate source file
>> + * * clone source content inside destination file
>> + * * verify that source content has been cloned inside destination file
>> + * * write a single byte inside destination file
>> + * * verify that source content didn't change while destination did
> This is very minor but I find dashes to be a better choice for lists
> inside of C comments.
>
>> + */
>> +
>> +#include "tst_test.h"
>> +#include "lapi/fs.h"
>> +
>> +#define MNTPOINT "mnt"
>> +#define SRCPATH MNTPOINT "/file0"
>> +#define DSTPATH MNTPOINT "/file1"
>> +
>> +#define FILEDATA "qwerty"
>> +#define FILESIZE sizeof(FILEDATA)
>> +
>> +static int src_fd = -1;
>> +static int dst_fd = -1;
>> +
>> +static void run(void)
>> +{
>> +	char buff[FILESIZE];
>> +	struct stat src_stat;
>> +	struct stat dst_stat;
>> +
>> +	src_fd = SAFE_OPEN(SRCPATH, O_CREAT | O_RDWR, 0640);
>> +	dst_fd = SAFE_OPEN(DSTPATH, O_CREAT | O_RDWR, 0640);
>> +
>> +	tst_res(TINFO, "Writing data inside src file");
>> +
>> +	SAFE_WRITE(1, src_fd, FILEDATA, FILESIZE);
>> +	SAFE_FSYNC(src_fd);
>> +
>> +	TST_EXP_PASS(ioctl(dst_fd, FICLONE, src_fd));
>> +	if (TST_RET == -1)
>> +		return;
>> +
>> +	SAFE_FSYNC(dst_fd);
>> +
>> +	tst_res(TINFO, "Verifing that data is cloned between files");
>> +
>> +	SAFE_FSTAT(src_fd, &src_stat);
>> +	SAFE_FSTAT(dst_fd, &dst_stat);
>> +
>> +	TST_EXP_EXPR(src_stat.st_ino != dst_stat.st_ino,
>> +		"inode is different. %lu != %lu",
>> +		src_stat.st_ino,
>> +		dst_stat.st_ino);
>> +
>> +	TST_EXP_EQ_LI(src_stat.st_size, dst_stat.st_size);
>> +
>> +	SAFE_READ(0, dst_fd, buff, FILESIZE);
>> +
>> +	TST_EXP_EXPR(!strncmp(buff, FILEDATA, FILESIZE),
>> +		"dst file has the src file content (\"%s\" - %ld bytes)",
>> +		buff,
>> +		FILESIZE);
>> +
>> +	tst_res(TINFO, "Writing a byte inside dst file");
>> +
>> +	SAFE_WRITE(SAFE_WRITE_ALL, dst_fd, "a", 1);
>> +	SAFE_FSYNC(dst_fd);
>> +
>> +	tst_res(TINFO, "Verifing that src file content didn't change");
>> +
>> +	SAFE_FSTAT(src_fd, &src_stat);
>> +	SAFE_FSTAT(dst_fd, &dst_stat);
>> +
>> +	TST_EXP_EQ_LI(dst_stat.st_size, src_stat.st_size + 1);
>> +
>> +	SAFE_READ(0, src_fd, buff, FILESIZE);
>> +
>> +	TST_EXP_EXPR(!strncmp(buff, FILEDATA, FILESIZE),
>> +		"src file content didn't change");
> So you append to the file but then only read the initial part? That does
> not sound right. I guess that easiest solution is to seek to the start
> of the file or od pwrite() so that we overwrite the first byte rather
> than appending.
>
> Or at least check the return value from the read.
>
>> +	SAFE_CLOSE(src_fd);
>> +	SAFE_CLOSE(dst_fd);
>> +	src_fd = -1;
>> +	dst_fd = -1;
> This is not needed, the SAFE_CLOSE() macro sets the fd to -1.
>
>> +	remove(SRCPATH);
>> +	remove(DSTPATH);
> Please use SAFE_UNLINK() instead.
>
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (src_fd != -1)
>> +		SAFE_CLOSE(src_fd);
>> +
>> +	if (dst_fd != -1)
>> +		SAFE_CLOSE(dst_fd);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.cleanup = cleanup,
>> +	.min_kver = "4.5",
>> +	.needs_root = 1,
>> +	.mount_device = 1,
>> +	.mntpoint = MNTPOINT,
>> +	.dev_fs_type = "btrfs",
>
> I suppose that we need .use_filesystems or similar and convert the
> dev_fs_type to an array so that we can run this test on xfs as well...
This might be tricky to implement, since we need to adapt .dev_fs_ops as 
well..
>> +};
>>
>> -- 
>> 2.35.3
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

Andrea

Hi!
> > I suppose that we need .use_filesystems or similar and convert the
> > dev_fs_type to an array so that we can run this test on xfs as well...
> This might be tricky to implement, since we need to adapt .dev_fs_ops as 
> well..

I guess that we need to do a bigger surgery and put all the device
related flags into a structure, e.g.

struct tst_fs {
	const char *dev_fs_type;

	const char *const *mkfs_opts;
        const char *const *mkfs_extra_opts;

	unsigned int mnt_flags;
	const char *mnt_data;
};

struct tst_test {
	...
	struct tst_fs filesystems[];
	...
};

That way we can specify all the options per a filesystems. I can try to
do prepare the patchset next week.

Ok thank you

On 5/31/24 10:01, Cyril Hrubis wrote:
> Hi!
>>> I suppose that we need .use_filesystems or similar and convert the
>>> dev_fs_type to an array so that we can run this test on xfs as well...
>> This might be tricky to implement, since we need to adapt .dev_fs_ops as
>> well..
> I guess that we need to do a bigger surgery and put all the device
> related flags into a structure, e.g.
>
> struct tst_fs {
> 	const char *dev_fs_type;
>
> 	const char *const *mkfs_opts;
>          const char *const *mkfs_extra_opts;
>
> 	unsigned int mnt_flags;
> 	const char *mnt_data;
> };
>
> struct tst_test {
> 	...
> 	struct tst_fs filesystems[];
> 	...
> };
>
> That way we can specify all the options per a filesystems. I can try to
> do prepare the patchset next week.
>