Re: [LTP] [PATCH] syscalls/write06: Add new test

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Signed-off-by: Dai Shili <daisl.fnst@fujitsu.com>
> ---
>  runtest/syscalls                           |  1 +
>  testcases/kernel/syscalls/write/.gitignore |  1 +
>  testcases/kernel/syscalls/write/write06.c  | 94 ++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/write/write06.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index bcf3d56..32fcda4 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1699,6 +1699,7 @@ write02 write02
>  write03 write03
>  write04 write04
>  write05 write05
> +write06 write06
>  
>  writev01 writev01
>  writev02 writev02
> diff --git a/testcases/kernel/syscalls/write/.gitignore b/testcases/kernel/syscalls/write/.gitignore
> index 7f36194..8529aae 100644
> --- a/testcases/kernel/syscalls/write/.gitignore
> +++ b/testcases/kernel/syscalls/write/.gitignore
> @@ -3,3 +3,4 @@
>  /write03
>  /write04
>  /write05
> +/write06
> diff --git a/testcases/kernel/syscalls/write/write06.c b/testcases/kernel/syscalls/write/write06.c
> new file mode 100644
> index 0000000..c62a266
> --- /dev/null
> +++ b/testcases/kernel/syscalls/write/write06.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili <daisl.fnst@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test the write() system call with O_APPEND.
> + *
> + * Writing 2k data to the file, close it and reopen it with O_APPEND.
> + * Verify that the file size is 3k and offset is moved to the end of the file.
> + */
> +
> +#include <stdlib.h>
> +#include <inttypes.h>
> +#include "tst_test.h"
> +#include "tst_safe_prw.h"
> +
> +#define K1              1024
> +#define K2              (K1 * 2)
> +#define K3              (K1 * 3)
> +#define DATA_FILE       "write06_file"
> +
> +static int fd = -1;
> +static char *write_buf[2];
> +
> +static void l_seek(int fdesc, off_t offset, int whence, off_t checkoff)
> +{
> +	off_t offloc;
> +
> +	offloc = SAFE_LSEEK(fdesc, offset, whence);
> +	if (offloc != checkoff) {
> +		tst_res(TFAIL, "%" PRId64 " = lseek(%d, %" PRId64 ", %d) != %" PRId64,
> +			(int64_t)offloc, fdesc, (int64_t)offset, whence, (int64_t)checkoff);
> +	}
> +}
> +
> +static void verify_write(void)
> +{
> +	int fail = 0;
> +	struct stat statbuf;
> +
> +	fd = SAFE_OPEN(DATA_FILE, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +	SAFE_WRITE(1, fd, write_buf[0], K2);
> +	SAFE_CLOSE(fd);
> +
> +	fd = SAFE_OPEN(DATA_FILE, O_RDWR | O_APPEND, 0666);
                                                     ^
						     No need to pass
						     mode without
						     O_CREAT
> +	SAFE_FSTAT(fd, &statbuf);
> +	if (statbuf.st_size != K2) {
> +		fail++;
> +		tst_res(TFAIL, "file size is %ld != K2", statbuf.st_size);
> +	}
> +
> +	l_seek(fd, K1, SEEK_SET, K1);

Why do we do the seek here?

Wouldn't it make much more sense just to write the write_buf[1] then
check that the st_size is K3?

That way we would check that O_APPEND opened file has correct position
associated with the file descriptor.

> +	SAFE_WRITE(1, fd, write_buf[1], K1);
> +	l_seek(fd, 0, SEEK_CUR, K3);

And here as well, why the seek? This is actually the place that
increases the size not the write() as it should have been.

> +	SAFE_FSTAT(fd, &statbuf);
> +	if (statbuf.st_size != K3) {
> +		fail++;
> +		tst_res(TFAIL, "file size is %ld != K3", statbuf.st_size);
> +	}

Really the whole test should do:

	open(FILE, O_RDWR | O_CREAT | O_TRUNC, 0666)
	write()
	close()

	open(FILE, O_RDWR | O_APPEND)
	check size
	write()
	check size
	close()

	open(FILE, O_RDONLY);
	read()
	verify data
	close()


> +	if (!fail)
> +		tst_res(TPASS, "O_APPEND test passed.");
> +	SAFE_CLOSE(fd);
> +}
> +
> +static void setup(void)
> +{
> +	write_buf[0] = SAFE_MALLOC(K2);
> +	memset(write_buf[0], 0, K2);
> +	write_buf[1] = SAFE_MALLOC(K1);
> +	memset(write_buf[0], 1, K1);
                         ^
			 1

Also can you please switch these two buffers to a guarded buffers?

See:

https://github.com/linux-test-project/ltp/wiki/C-Test-API#131-guarded-buffers

> +}
> +
> +static void cleanup(void)
> +{
> +	free(write_buf[0]);
> +	free(write_buf[1]);
> +
> +	if (fd > -1)

Probably fd != -1 would be a bit clearer.

> +		SAFE_CLOSE(fd);
> +
> +	SAFE_UNLINK(DATA_FILE);
> +}
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = verify_write,
> +};
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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