[LTP] [PATCH] poll: fix LTP check warnings and improve poll01 validation

[LTP] [PATCH] poll: fix LTP check warnings and improve poll01 validation

[Jinseok Kim]

Remove extra blank lines, mark helper functions as static, and fix
documentation comments starting with '/*\'.

Add explicit checks for poll() return value and validate that no
unexpected revents bits are set for POLLIN and POLLOUT cases.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/poll/poll01.c | 24 ++++++++++++++++++------
 testcases/kernel/syscalls/poll/poll02.c |  4 ++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/syscalls/poll/poll01.c b/testcases/kernel/syscalls/poll/poll01.c
index b05e809ab..ca68ec9c4 100644
--- a/testcases/kernel/syscalls/poll/poll01.c
+++ b/testcases/kernel/syscalls/poll/poll01.c
@@ -5,7 +5,7 @@
  * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
  */

-/*
+/*\
  * Check that poll() works for POLLOUT and POLLIN and that revents is set
  * correctly.
  */
@@ -34,11 +34,14 @@ static void verify_pollout(void)
 		return;
 	}

-	if (outfds[0].revents != POLLOUT) {
-		tst_res(TFAIL | TTERRNO, "poll() failed to set POLLOUT");
+	if (TST_RET != 1) {
+		tst_res(TFAIL, "Unexpected poll() return value %ld in POLLOUT", TST_RET);
 		return;
 	}

+	TST_EXP_EXPR(outfds[0].revents & POLLOUT);
+	TST_EXP_EXPR((outfds[0].revents & ~POLLOUT) == 0);
+
 	tst_res(TPASS, "poll() POLLOUT");
 }

@@ -60,11 +63,20 @@ static void verify_pollin(void)
 		goto end;
 	}

-	if (infds[0].revents != POLLIN) {
-		tst_res(TFAIL, "poll() failed to set POLLIN");
+	if (TST_RET != 1) {
+		tst_res(TFAIL, "Unexpected poll() return value %ld in POLLIN", TST_RET);
 		goto end;
 	}

+	if (!(infds[0].revents & POLLIN)) {
+		tst_res(TFAIL, "infds[0].revents & POLLIN");
+		goto end;
+	}
+
+	if (!((infds[0].revents & ~POLLIN) == 0)) {
+		tst_res(TFAIL, "infds[0].revents & ~POLLIN) == 0");
+		goto end;
+	}

 	tst_res(TPASS, "poll() POLLIN");

@@ -72,7 +84,7 @@ end:
 	SAFE_READ(1, fildes[0], read_buf, sizeof(write_buf));
 }

-void verify_poll(unsigned int n)
+static void verify_poll(unsigned int n)
 {
 	switch (n) {
 	case 0:
diff --git a/testcases/kernel/syscalls/poll/poll02.c b/testcases/kernel/syscalls/poll/poll02.c
index c0665927b..47e323451 100644
--- a/testcases/kernel/syscalls/poll/poll02.c
+++ b/testcases/kernel/syscalls/poll/poll02.c
@@ -3,7 +3,7 @@
  * Copyright (C) 2015-2017 Cyril Hrubis <chrubis@suse.cz>
  */

-/*
+/*\
  * Check that poll() timeouts correctly.
  */
 #include <errno.h>
@@ -15,7 +15,7 @@

 static int fds[2];

-int sample_fn(int clk_id, long long usec)
+static int sample_fn(int clk_id, long long usec)
 {
 	unsigned int sleep_ms = usec / 1000;

--
2.43.0

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

Re: [LTP] [PATCH] poll: fix LTP check warnings and improve poll01 validation

[Petr Vorel]

Hi Jinseok,

> Remove extra blank lines, mark helper functions as static, and fix
> documentation comments starting with '/*\'.

> Add explicit checks for poll() return value and validate that no
> unexpected revents bits are set for POLLIN and POLLOUT cases.

FYI we already do check for unexpected revents bits, see below.

> Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
> ---
>  testcases/kernel/syscalls/poll/poll01.c | 24 ++++++++++++++++++------
>  testcases/kernel/syscalls/poll/poll02.c |  4 ++--
>  2 files changed, 20 insertions(+), 8 deletions(-)

> diff --git a/testcases/kernel/syscalls/poll/poll01.c b/testcases/kernel/syscalls/poll/poll01.c
> index b05e809ab..ca68ec9c4 100644
> --- a/testcases/kernel/syscalls/poll/poll01.c
> +++ b/testcases/kernel/syscalls/poll/poll01.c
> @@ -5,7 +5,7 @@
>   * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
>   */

> -/*
> +/*\
>   * Check that poll() works for POLLOUT and POLLIN and that revents is set
While you at it, could you please use form
:manpage:`poll(2)`
because that links in our docs [1] into manpage [2].

[1] https://linux-test-project.readthedocs.io/en/latest/users/test_catalog.html
[2] https://man7.org/linux/man-pages/man2/poll.2.html

>   * correctly.
>   */
> @@ -34,11 +34,14 @@ static void verify_pollout(void)
>  		return;
>  	}

> -	if (outfds[0].revents != POLLOUT) {
> -		tst_res(TFAIL | TTERRNO, "poll() failed to set POLLOUT");
> +	if (TST_RET != 1) {
> +		tst_res(TFAIL, "Unexpected poll() return value %ld in POLLOUT", TST_RET);
>  		return;
>  	}

Checking for TST_RET != 1 is probably a wanted improvement.

But how about test both with TST_EXP_VAL()?
	TST_EXP_VAL(poll(outfds, 1, -1), 1);
	if (!TST_PASS)
		return;

> +	TST_EXP_EXPR(outfds[0].revents & POLLOUT);
> +	TST_EXP_EXPR((outfds[0].revents & ~POLLOUT) == 0);

This is equivalent of existing code:

	if (outfds[0].revents != POLLOUT) {
		tst_res(TFAIL | TTERRNO, "poll() failed to set POLLOUT");
	}

Maybe the message could be more precise (it also fails if revents contains
POLLOUT and something else, but that might be obvious). Also it could print the
actual revents value (but TST_EXP_EXPR() don't print it either).

I don't have strong opinion which variant is better.

But if you use TST_EXP_EXPR() ...
> +
>  	tst_res(TPASS, "poll() POLLOUT");
... then this TPASS is redundant and misleading - it would be printed also on
the failure because you don't skip it on failure.

>  }

> @@ -60,11 +63,20 @@ static void verify_pollin(void)
>  		goto end;
>  	}

> -	if (infds[0].revents != POLLIN) {
> -		tst_res(TFAIL, "poll() failed to set POLLIN");
> +	if (TST_RET != 1) {
> +		tst_res(TFAIL, "Unexpected poll() return value %ld in POLLIN", TST_RET);
>  		goto end;
>  	}

> +	if (!(infds[0].revents & POLLIN)) {
> +		tst_res(TFAIL, "infds[0].revents & POLLIN");
> +		goto end;
NOTE: you could use TST_EXP_EXPR():

	SAFE_WRITE(SAFE_WRITE_ALL, fildes[1], write_buf, sizeof(write_buf));

	TST_EXP_VAL(poll(infds, 1, -1), 1);
	if (!TST_PASS)
		goto end;

	TST_EXP_EXPR(infds[0].revents & POLLIN);
	TST_EXP_EXPR((infds[0].revents & ~POLLIN) == 0);

end:
	SAFE_READ(1, fildes[0], read_buf, sizeof(write_buf));

Kind regards,
Petr

> +	}
> +
> +	if (!((infds[0].revents & ~POLLIN) == 0)) {
> +		tst_res(TFAIL, "infds[0].revents & ~POLLIN) == 0");
> +		goto end;
> +	}

>  	tst_res(TPASS, "poll() POLLIN");

> @@ -72,7 +84,7 @@ end:
>  	SAFE_READ(1, fildes[0], read_buf, sizeof(write_buf));
>  }

> -void verify_poll(unsigned int n)
> +static void verify_poll(unsigned int n)
>  {
>  	switch (n) {
>  	case 0:
> diff --git a/testcases/kernel/syscalls/poll/poll02.c b/testcases/kernel/syscalls/poll/poll02.c
> index c0665927b..47e323451 100644
> --- a/testcases/kernel/syscalls/poll/poll02.c
> +++ b/testcases/kernel/syscalls/poll/poll02.c
> @@ -3,7 +3,7 @@
>   * Copyright (C) 2015-2017 Cyril Hrubis <chrubis@suse.cz>
>   */

> -/*
> +/*\
>   * Check that poll() timeouts correctly.
And here please as well:
:manpage:`poll(2)`

>   */
>  #include <errno.h>
> @@ -15,7 +15,7 @@

>  static int fds[2];

> -int sample_fn(int clk_id, long long usec)
> +static int sample_fn(int clk_id, long long usec)
>  {
>  	unsigned int sleep_ms = usec / 1000;

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

[LTP] [PATCH v2] poll: fix make check findings and improve poll01 validation

[Jinseok Kim]

Remove extra blank lines, mark helper functions as static, and fix
documentation comments starting with '/*\'.

Use :manpage:`poll(2)` in the documentation comments to link to the
man page.

Add explicit checks for poll() return value and tighten revents
validation for POLLIN and POLLOUT cases.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/poll/poll01.c | 37 ++++++++-----------------
 testcases/kernel/syscalls/poll/poll02.c |  6 ++--
 2 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/testcases/kernel/syscalls/poll/poll01.c b/testcases/kernel/syscalls/poll/poll01.c
index b05e809ab..fc11c11bc 100644
--- a/testcases/kernel/syscalls/poll/poll01.c
+++ b/testcases/kernel/syscalls/poll/poll01.c
@@ -5,8 +5,8 @@
  * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
  */

-/*
- * Check that poll() works for POLLOUT and POLLIN and that revents is set
+/*\
+ * Check that :manpage:`poll(2)` works for POLLOUT and POLLIN and that revents is set
  * correctly.
  */
 #include <unistd.h>
@@ -27,19 +27,12 @@ static void verify_pollout(void)
 		{.fd = fildes[1], .events = POLLOUT},
 	};

-	TEST(poll(outfds, 1, -1));
-
-	if (TST_RET == -1) {
-		tst_res(TFAIL | TTERRNO, "poll() POLLOUT failed");
-		return;
-	}
-
-	if (outfds[0].revents != POLLOUT) {
-		tst_res(TFAIL | TTERRNO, "poll() failed to set POLLOUT");
+	TST_EXP_VAL(poll(outfds, 1, -1), 1);
+	if (!TST_PASS)
 		return;
-	}

-	tst_res(TPASS, "poll() POLLOUT");
+	TST_EXP_EXPR(outfds[0].revents & POLLOUT);
+	TST_EXP_EXPR((outfds[0].revents & ~POLLOUT) == 0);
 }

 static void verify_pollin(void)
@@ -53,26 +46,18 @@ static void verify_pollin(void)

 	SAFE_WRITE(SAFE_WRITE_ALL, fildes[1], write_buf, sizeof(write_buf));

-	TEST(poll(infds, 1, -1));
-
-	if (TST_RET == -1) {
-		tst_res(TFAIL | TTERRNO, "poll() POLLIN failed");
-		goto end;
-	}
-
-	if (infds[0].revents != POLLIN) {
-		tst_res(TFAIL, "poll() failed to set POLLIN");
+	TST_EXP_VAL(poll(infds, 1, -1), 1);
+	if (!TST_PASS)
 		goto end;
-	}
-

-	tst_res(TPASS, "poll() POLLIN");
+	TST_EXP_EXPR(infds[0].revents & POLLIN);
+	TST_EXP_EXPR((infds[0].revents & ~POLLIN) == 0);

 end:
 	SAFE_READ(1, fildes[0], read_buf, sizeof(write_buf));
 }

-void verify_poll(unsigned int n)
+static void verify_poll(unsigned int n)
 {
 	switch (n) {
 	case 0:
diff --git a/testcases/kernel/syscalls/poll/poll02.c b/testcases/kernel/syscalls/poll/poll02.c
index c0665927b..76cebc70d 100644
--- a/testcases/kernel/syscalls/poll/poll02.c
+++ b/testcases/kernel/syscalls/poll/poll02.c
@@ -3,8 +3,8 @@
  * Copyright (C) 2015-2017 Cyril Hrubis <chrubis@suse.cz>
  */

-/*
- * Check that poll() timeouts correctly.
+/*\
+ * Check that :manpage:`poll(2)` timeouts correctly.
  */
 #include <errno.h>
 #include <fcntl.h>
@@ -15,7 +15,7 @@

 static int fds[2];

-int sample_fn(int clk_id, long long usec)
+static int sample_fn(int clk_id, long long usec)
 {
 	unsigned int sleep_ms = usec / 1000;

--
2.43.0

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

Re: [LTP] [PATCH v2] poll: fix make check findings and improve poll01 validation

[Petr Vorel]

Hi Jinseok,

thanks, merged!

Kind regards,
Petr

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