[LTP] [PATCH] read: verify read size and data correctness

[LTP] [PATCH] read: verify read size and data correctness

[Jinseok Kim]

The test only checked that read(2) did not fail, which allows
partial reads or unexpected data to pass unnoticed.

Verify that read(2) returns the expected number of bytes and
that the read data matches the written buffer.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/read/read01.c | 27 ++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/testcases/kernel/syscalls/read/read01.c b/testcases/kernel/syscalls/read/read01.c
index 68d6346c5..f205979b9 100644
--- a/testcases/kernel/syscalls/read/read01.c
+++ b/testcases/kernel/syscalls/read/read01.c
@@ -10,25 +10,38 @@
 #define SIZE 512

 static int fd;
-static char buf[SIZE];
+static char write_buf[SIZE];
+static char read_buf[SIZE];

 static void verify_read(void)
 {
 	SAFE_LSEEK(fd, 0, SEEK_SET);

-	TEST(read(fd, buf, SIZE));
+	TEST(read(fd, read_buf, SIZE));

-	if (TST_RET == -1)
+	if (TST_RET == -1) {
 		tst_res(TFAIL | TTERRNO, "read(2) failed");
-	else
-		tst_res(TPASS, "read(2) returned %ld", TST_RET);
+		return;
+	}
+
+	if (TST_RET != SIZE) {
+		tst_res(TFAIL, "read(2) returned %ld, expected %d", TST_RET, SIZE);
+		return;
+	}
+
+	if (memcmp(write_buf, read_buf, SIZE)) {
+		tst_res(TFAIL, "read(2) returned unexpected data");
+		return;
+	}
+
+	tst_res(TPASS, "read(2) returned expected data %ld", TST_RET);
 }

 static void setup(void)
 {
-	memset(buf, '*', SIZE);
+	memset(write_buf, '*', SIZE);
 	fd = SAFE_OPEN("testfile", O_RDWR | O_CREAT, 0700);
-	SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, SIZE);
+	SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, SIZE);
 }

 static void cleanup(void)
--
2.43.0

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

Re: [LTP] [PATCH] read: verify read size and data correctness

[Andrea Cervesato via ltp]

Hi Jinseok,

the name of the commit is related to read01, so it should be:

	read01: verify read size and data correctness

> The test only checked that read(2) did not fail, which allows
> partial reads or unexpected data to pass unnoticed.
> 
> Verify that read(2) returns the expected number of bytes and
> that the read data matches the written buffer.
> 
> Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
> ---
>  testcases/kernel/syscalls/read/read01.c | 27 ++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/read/read01.c b/testcases/kernel/syscalls/read/read01.c
> index 68d6346c5..f205979b9 100644
> --- a/testcases/kernel/syscalls/read/read01.c
> +++ b/testcases/kernel/syscalls/read/read01.c
> @@ -10,25 +10,38 @@
>  #define SIZE 512
> 
>  static int fd;
> -static char buf[SIZE];
> +static char write_buf[SIZE];
> +static char read_buf[SIZE];
> 
>  static void verify_read(void)
>  {
>  	SAFE_LSEEK(fd, 0, SEEK_SET);
> 
> -	TEST(read(fd, buf, SIZE));
> +	TEST(read(fd, read_buf, SIZE));

We have TST_EXP_PASS().

> 
> -	if (TST_RET == -1)
> +	if (TST_RET == -1) {
>  		tst_res(TFAIL | TTERRNO, "read(2) failed");
> -	else
> -		tst_res(TPASS, "read(2) returned %ld", TST_RET);
> +		return;
> +	}

So this won't be needed.

> +
> +	if (TST_RET != SIZE) {
> +		tst_res(TFAIL, "read(2) returned %ld, expected %d", TST_RET, SIZE);
> +		return;
> +	}

TST_EXP_EQ_LI(TST_RET, SIZE);

> +
> +	if (memcmp(write_buf, read_buf, SIZE)) {
> +		tst_res(TFAIL, "read(2) returned unexpected data");
> +		return;
> +	}

TST_EXP_EQ_STRN(write_buf, read_buf, SIZE);

> +
> +	tst_res(TPASS, "read(2) returned expected data %ld", TST_RET);

So this is not needed anymore.

>  }
> 
>  static void setup(void)
>  {
> -	memset(buf, '*', SIZE);
> +	memset(write_buf, '*', SIZE);
>  	fd = SAFE_OPEN("testfile", O_RDWR | O_CREAT, 0700);
> -	SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, SIZE);
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, SIZE);
>  }
> 
>  static void cleanup(void)
> --
> 2.43.0
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

[LTP] [PATCH] read: verify read size and data correctness

[Jinseok Kim]

Hi,

Thanks for the review.

While looking into this further, I realized that strengthening read01.c
in this way would make it overlap with read04.c.
Would you prefer me to:

- keep read01.c as a minimal success test, or
- update read01.c and drop read04.c as redundant?

The main distinction seems to be full read vs short read at EOF,
so I wanted to confirm your preference before sending the
next revision.

Thanks,
Jinseok.

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

Re: [LTP] [PATCH] read: verify read size and data correctness

[Andrea Cervesato via ltp]

Hi Jinseok,

> Hi,
> 
> Thanks for the review.
> 
> While looking into this further, I realized that strengthening read01.c
> in this way would make it overlap with read04.c.
> Would you prefer me to:
> 
> - keep read01.c as a minimal success test, or
> - update read01.c and drop read04.c as redundant?
> 
> The main distinction seems to be full read vs short read at EOF,
> so I wanted to confirm your preference before sending the
> next revision.
> 
> Thanks,
> Jinseok.

I'm sorry, indeed I didn't notice that this test was overlapping
I would keep the test minimal then, because we don't want to have
two tests haveing the same purpose.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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