Re: [LTP] [PATCH 1/3] Add ioctl_ficlone01 test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

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


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