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

[Petr Vorel <pvorel@suse.cz>]

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