Re: [LTP] [PATCH] syscalls/read04:add multiple read size

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Liao Huangjie <liaohj.jy@fujitsu.com> writes:

> From: Huangjie Liao <liaohj.jy@fujitsu.com>
>
> Signed-off-by: Huangjie Liao <liaohj.jy@fujitsu.com>
> ---
>  testcases/kernel/syscalls/read/read04.c | 59 ++++++++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/read/read04.c b/testcases/kernel/syscalls/read/read04.c
> index 47875c0..37c670f 100644
> --- a/testcases/kernel/syscalls/read/read04.c
> +++ b/testcases/kernel/syscalls/read/read04.c
> @@ -6,36 +6,49 @@
>  /*\
>   * [Description]
>   *
> - * Testcase to check if read() returns the number of bytes read correctly.
> + * Testcase to check if read() returns the correct number of bytes

Trailing whitespace at the end of this line

> + * when using multip sizes [0, 1/2*page_size, 3/2*page_size, 2*page_size].
>   */
>  
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <stdio.h>
>  #include <fcntl.h>
> +#include <unistd.h>
>  #include "tst_test.h"
>  
> +#define MNT_POINT "mntpoint"
> +static int page_size;
>  static const char *fname = "test_file";
> -static const char palfa[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
> -#define PALFA_LEN (sizeof(palfa)-1)
> +static char *write_buf[2], *read_buf;
>  
> -static void verify_read(void)
> +static void verify_read(unsigned int n)
>  {
>  	int fd;
> -	char prbuf[BUFSIZ];
> -
> +	int size = n * page_size / 2;

Usually interesting things happen around page boundaries. It would be
better to read (page_size - 1), page_size and (page_size + 1). And also
use an offset starting at (page_size - 1), pages_size etc.

>  	fd = SAFE_OPEN(fname, O_RDONLY);
> -	TEST(read(fd, prbuf, BUFSIZ));
> +	TEST(read(fd, read_buf, size));
>  
> -	if (TST_RET != PALFA_LEN) {
> -		tst_res(TFAIL, "Bad read count - got %ld - expected %zu",
> -				TST_RET, PALFA_LEN);
> +	if (TST_RET != size) {
> +		tst_res(TFAIL, "Bad read count - got %ld - expected %d",
> +				TST_RET, size);

There is no requirement for read to return size bytes in a single
call. A loop is required.

The test presently passes because the buffer is small and the process is
unlikely to be interrupted. However if you start to read one page at a
time and use all filesystems it's much more likely to fail.

>  		goto out;
>  	}
>  
> -	if (memcmp(palfa, prbuf, PALFA_LEN)) {
> -		tst_res(TFAIL, "read buffer not equal to write buffer");
> -		goto out;
> +	if (n <= 2) {
> +		if (memcmp(read_buf, write_buf[0], size)) {
> +			tst_res(TFAIL, "read buffer not equal to write buffer1");
> +			goto out;
> +		}
> +	} else {
> +		if (memcmp(read_buf, write_buf[0], page_size)) {
> +			tst_res(TFAIL, "read buffer not equal to write buffer2");
> +			goto out;
> +		}
> +		if (memcmp(read_buf + page_size, write_buf[1], size - page_size)) {
> +			tst_res(TFAIL, "read buffer not equal to write buffer3");
> +			goto out;
> +		}
>  	}
>  
>  	tst_res(TPASS, "read() data correctly");
> @@ -48,13 +61,27 @@ static void setup(void)
>  {
>  	int fd;
>  
> +	page_size = getpagesize();
> +	write_buf[0] = tst_alloc(page_size);
> +	write_buf[1] = tst_alloc(page_size);
> +	read_buf = tst_alloc(2 * page_size);
> +
> +	memset(write_buf[0], 'A', page_size);
> +	memset(write_buf[1], 'B', page_size);
> +	memset(read_buf, 0, 2 * page_size);
> +
>  	fd = SAFE_CREAT(fname, 0777);
> -	SAFE_WRITE(1, fd, palfa, PALFA_LEN);
> +	SAFE_WRITE(1, fd, write_buf[0], page_size);
> +	SAFE_WRITE(1, fd, write_buf[1], page_size);

This patch fails to apply now because this was changed to use
SAFE_WRITE_ALL.

>  	SAFE_CLOSE(fd);
> +

More trailing whitespace.

>  }
>  
>  static struct tst_test test = {
> -	.needs_tmpdir = 1,
>  	.setup = setup,
> -	.test_all = verify_read,
> +	.test = verify_read,
> +	.tcnt = 5,
> +	.mount_device = 1,
> +	.mntpoint = MNT_POINT,
> +	.all_filesystems = 1,
>  };
> -- 
> 1.8.3.1


-- 
Thank you,
Richard.

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