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

[Cyril Hrubis <chrubis@suse.cz>]

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

-- 
Cyril Hrubis
chrubis@suse.cz

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