syscalls/write06: Add new test

Like pwrite04.c, test the write() system call with O_APPEND.

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

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

Hi Cyril

在 2021/12/7 21:51, Cyril Hrubis 写道:
> 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
Got it.
>> +	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.

I want to verify that the offset will move to the end of the file.

So I put the offset in the middle of the file before write.

>> +	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.
Do seek to check the offset is moved to the end of the file.
>> +	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()
>
I'm so sorry, my commit msg is not clear.

What I want to test is the following description of open(2) man-pages:

O_APPEND
       The file is opened in append mode.  Before each write(2), the 
file offset is positioned at the end of the file, as if with lseek(2).  
The modification of the file offset and the write operation are 
performed as a single atomic step.
>> +	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
OK. I will switch to 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.
Got it.
>> +		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

Hi!
> What I want to test is the following description of open(2) man-pages:
> 
> O_APPEND
>  ?????? ?? The file is opened in append mode.?? Before each write(2), the 
> file offset is positioned at the end of the file, as if with lseek(2).?? 
> The modification of the file offset and the write operation are 
> performed as a single atomic step.

Ah, now it makes much more sense.

I think part of the problem is that the code is actually confusing. I do
not like the l_seek() function at all, it's confusing at best because it
does several different things at the same time. So it would be much
better to write the offset check as:

	off = SAFE_LSEEK(fd, 1K, SEEK_SET)
	if (off != 1K)
		tst_brk(TBROK, "Failed to seek to 1K");

	SAFE_WRITE(1, fd, write_buf[1], K1);

	off = SAFE_LSEEK(fd, 0, SEEK_CUR);
	if (off != K3)
		tst_res(TFAIL, "Wrong offset after write %zu expected %u", off, K3)
	else
		tst_res(TPASS, "Offset is correct after write %zu", off);

	SAFE_FSTAT(fd, &statbuf);
	if (statbuf.st_size != K3) {
		tst_res(TFAIL, "Wrong file size after append %zu expected %u",
			statubuf.st_size, K3);
	} else {
		tst_res(TPASS, "Correct file size after append %u", K3);
	}


This makes it much clearer what happens here and the TPASS message
actually says what has passed...