[LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Petr Vorel]

This fixes error on unsupported __NR__newselect:
select_var.h:89: TCONF: syscall(-1) __NR__newselect not supported on your arch
select03.c:90: TFAIL: Child exited with 32

Fixes: ffdd3b36cd ("syscalls: Add missing exit status check")
Reported-by: Dominique Leuenberger <dleuenberger@suse.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Alternatively, we could revert to previous state (remove
"!WEXITSTATUS(status)" check), if we really don't care about any other
exit code.

Kind regards,
Petr

 testcases/kernel/syscalls/select/select03.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c
index 216b22104f..34aea12603 100644
--- a/testcases/kernel/syscalls/select/select03.c
+++ b/testcases/kernel/syscalls/select/select03.c
@@ -77,8 +77,10 @@ static void run(unsigned int n)
 
 	SAFE_WAITPID(pid, &status, 0);
 
-	if (WIFEXITED(status) && !WEXITSTATUS(status))
+	if (WIFEXITED(status) &&
+		(WEXITSTATUS(status) == 0 || WEXITSTATUS(status) == TCONF)) {
 		return;
+	}
 
 	if (tst_variant == GLIBC_SELECT_VARIANT &&
 	    tests[n].timeout == &invalid_to &&
-- 
2.45.2


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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Wei Gao via ltp]

On Wed, Nov 13, 2024 at 11:59:25AM +0100, Petr Vorel wrote:
> This fixes error on unsupported __NR__newselect:
> select_var.h:89: TCONF: syscall(-1) __NR__newselect not supported on your arch
> select03.c:90: TFAIL: Child exited with 32
> 
> Fixes: ffdd3b36cd ("syscalls: Add missing exit status check")
> Reported-by: Dominique Leuenberger <dleuenberger@suse.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Alternatively, we could revert to previous state (remove
> "!WEXITSTATUS(status)" check), if we really don't care about any other
> exit code.
I suggest remove "!WEXITSTATUS(status)" in current case unless we clearly know all
*correct* value return from do_select_faulty_to.
> 
> Kind regards,
> Petr
> 
>  testcases/kernel/syscalls/select/select03.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c
> index 216b22104f..34aea12603 100644
> --- a/testcases/kernel/syscalls/select/select03.c
> +++ b/testcases/kernel/syscalls/select/select03.c
> @@ -77,8 +77,10 @@ static void run(unsigned int n)
>  
>  	SAFE_WAITPID(pid, &status, 0);
>  
> -	if (WIFEXITED(status) && !WEXITSTATUS(status))
> +	if (WIFEXITED(status) &&
> +		(WEXITSTATUS(status) == 0 || WEXITSTATUS(status) == TCONF)) {
>  		return;
> +	}
>  
>  	if (tst_variant == GLIBC_SELECT_VARIANT &&
>  	    tests[n].timeout == &invalid_to &&
> -- 
> 2.45.2
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Avinesh Kumar]

Hi Petr,

On Wednesday, November 13, 2024 11:59:25 AM CET Petr Vorel wrote:
> This fixes error on unsupported __NR__newselect:
> select_var.h:89: TCONF: syscall(-1) __NR__newselect not supported on your arch
> select03.c:90: TFAIL: Child exited with 32
> 
> Fixes: ffdd3b36cd ("syscalls: Add missing exit status check")
> Reported-by: Dominique Leuenberger <dleuenberger@suse.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Reviewed-by: Avinesh Kumar <akumar@suse.de>


> ---
> Alternatively, we could revert to previous state (remove
> "!WEXITSTATUS(status)" check), if we really don't care about any other
> exit code.
> 
> Kind regards,
> Petr
> 
>  testcases/kernel/syscalls/select/select03.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c
> index 216b22104f..34aea12603 100644
> --- a/testcases/kernel/syscalls/select/select03.c
> +++ b/testcases/kernel/syscalls/select/select03.c
> @@ -77,8 +77,10 @@ static void run(unsigned int n)
>  
>  	SAFE_WAITPID(pid, &status, 0);
>  
> -	if (WIFEXITED(status) && !WEXITSTATUS(status))
> +	if (WIFEXITED(status) &&
> +		(WEXITSTATUS(status) == 0 || WEXITSTATUS(status) == TCONF)) {
>  		return;
> +	}
>  
>  	if (tst_variant == GLIBC_SELECT_VARIANT &&
>  	    tests[n].timeout == &invalid_to &&
> 

Thanks,
Avinesh



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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Cyril Hrubis]

Hi!
> ---
> Alternatively, we could revert to previous state (remove
> "!WEXITSTATUS(status)" check), if we really don't care about any other
> exit code.
> 
> Kind regards,
> Petr
> 
>  testcases/kernel/syscalls/select/select03.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c
> index 216b22104f..34aea12603 100644
> --- a/testcases/kernel/syscalls/select/select03.c
> +++ b/testcases/kernel/syscalls/select/select03.c
> @@ -77,8 +77,10 @@ static void run(unsigned int n)
>  
>  	SAFE_WAITPID(pid, &status, 0);
>  
> -	if (WIFEXITED(status) && !WEXITSTATUS(status))
> +	if (WIFEXITED(status) &&
> +		(WEXITSTATUS(status) == 0 || WEXITSTATUS(status) == TCONF)) {
>  		return;

I think that the main mistake here is that I kept the code in
tst_vbrk_() that exits the test with a return value in the case of a
child processes. So ideal fix would be to change the test library not to
do that, but that would require a bit more work, especially we would
have to look at all WAITPID() usages in newlib tests and make sure that
there are not tests that depends on such behavior. If there are none it
would stil require a few changes in the test library.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Petr Vorel]

Hi Cyril,

> Hi!
> > ---
> > Alternatively, we could revert to previous state (remove
> > "!WEXITSTATUS(status)" check), if we really don't care about any other
> > exit code.

> > Kind regards,
> > Petr

> >  testcases/kernel/syscalls/select/select03.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)

> > diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c
> > index 216b22104f..34aea12603 100644
> > --- a/testcases/kernel/syscalls/select/select03.c
> > +++ b/testcases/kernel/syscalls/select/select03.c
> > @@ -77,8 +77,10 @@ static void run(unsigned int n)

> >  	SAFE_WAITPID(pid, &status, 0);

> > -	if (WIFEXITED(status) && !WEXITSTATUS(status))
> > +	if (WIFEXITED(status) &&
> > +		(WEXITSTATUS(status) == 0 || WEXITSTATUS(status) == TCONF)) {
> >  		return;

> I think that the main mistake here is that I kept the code in
> tst_vbrk_() that exits the test with a return value in the case of a
> child processes.

I suppose you talk about in tst_vbrk_ (lib/tst_test.c):

	if (getpid() == lib_pid)
		do_exit(TTYPE_RESULT(ttype));

> So ideal fix would be to change the test library not to
> do that,

So would you just do_exit(0) or even just exit(0)?
Or exit 0 for TPASS or TCONF and 1 otherwise (TBROK, TFAIL, TWARN)? Or what
exactly did you meant?

Other option could be to keep library as is and have a custom functions which
"translate" LTP exit codes. I was thinking something like this:

bool tst_exit_status(int status)
{
	if (!WEXITSTATUS(status))
		return false;

	if (WEXITSTATUS(status) == TPASS || WEXITSTATUS(status) == TCONF)
		return true;

	return false;
}

But we actually have check_child_status(pid_t pid, int status) for this:

static void check_child_status(pid_t pid, int status)
{
	int ret;

	if (WIFSIGNALED(status)) {
		tst_brk(TBROK, "Child (%i) killed by signal %s", pid,
			tst_strsig(WTERMSIG(status)));
	}

	if (!(WIFEXITED(status)))
		tst_brk(TBROK, "Child (%i) exited abnormally", pid);

	ret = WEXITSTATUS(status);
	switch (ret) {
	case TPASS:
	case TBROK:
	case TCONF:
	break;
	default:
		tst_brk(TBROK, "Invalid child (%i) exit value %i", pid, ret);
	}
}

BTW I'm surprised that there is TBROK whitelisted.

> but that would require a bit more work, especially we would
> have to look at all WAITPID() usages in newlib tests and make sure that
> there are not tests that depends on such behavior. If there are none it
> would stil require a few changes in the test library.

Well, what I fear more is to broke even more tests :). But if you consider is
useful enough I can do the cleanup.

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Cyril Hrubis]

Hi!
> > I think that the main mistake here is that I kept the code in
> > tst_vbrk_() that exits the test with a return value in the case of a
> > child processes.
> 
> I suppose you talk about in tst_vbrk_ (lib/tst_test.c):
> 
> 	if (getpid() == lib_pid)
> 		do_exit(TTYPE_RESULT(ttype));

That one should stay, that is the overall exit value from the test.

There is one below that that should be replaced with exit(0).

> > So ideal fix would be to change the test library not to
> > do that,
> 
> So would you just do_exit(0) or even just exit(0)?
> Or exit 0 for TPASS or TCONF and 1 otherwise (TBROK, TFAIL, TWARN)? Or what
> exactly did you meant?

Anything but the main library process would do exit(0).

> Other option could be to keep library as is and have a custm functions which
> "translate" LTP exit codes. I was thinking something like this:
> 
> bool tst_exit_status(int status)
> {
> 	if (!WEXITSTATUS(status))
> 		return false;
> 
> 	if (WEXITSTATUS(status) == TPASS || WEXITSTATUS(status) == TCONF)
> 		return true;
> 
> 	return false;
> }
> 
> But we actually have check_child_status(pid_t pid, int status) for this:
> 
> static void check_child_status(pid_t pid, int status)
> {
> 	int ret;
> 
> 	if (WIFSIGNALED(status)) {
> 		tst_brk(TBROK, "Child (%i) killed by signal %s", pid,
> 			tst_strsig(WTERMSIG(status)));
> 	}
> 
> 	if (!(WIFEXITED(status)))
> 		tst_brk(TBROK, "Child (%i) exited abnormally", pid);
> 
> 	ret = WEXITSTATUS(status);
> 	switch (ret) {
> 	case TPASS:
> 	case TBROK:
> 	case TCONF:
> 	break;
> 	default:
> 		tst_brk(TBROK, "Invalid child (%i) exit value %i", pid, ret);
> 	}
> }
> 
> BTW I'm surprised that there is TBROK whitelisted.

That just checks for valid exit status.

> > but that would require a bit more work, especially we would
> > have to look at all WAITPID() usages in newlib tests and make sure that
> > there are not tests that depends on such behavior. If there are none it
> > would stil require a few changes in the test library.
> 
> Well, what I fear more is to broke even more tests :). But if you consider is
> useful enough I can do the cleanup.

The change would allow us to implement things like tst_brk(TFAIL, ...)
and tst_brk(TPASS, ...) correctly. The problem is that the result from
children after tst_brk() is propagated as a return value. What we need
instead is to add an abort flag to the results structure.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Petr Vorel]

> Hi!
> > > I think that the main mistake here is that I kept the code in
> > > tst_vbrk_() that exits the test with a return value in the case of a
> > > child processes.

> > I suppose you talk about in tst_vbrk_ (lib/tst_test.c):

> > 	if (getpid() == lib_pid)
> > 		do_exit(TTYPE_RESULT(ttype));

> That one should stay, that is the overall exit value from the test.

> There is one below that that should be replaced with exit(0).

> > > So ideal fix would be to change the test library not to
> > > do that,

> > So would you just do_exit(0) or even just exit(0)?
> > Or exit 0 for TPASS or TCONF and 1 otherwise (TBROK, TFAIL, TWARN)? Or what
> > exactly did you meant?

> Anything but the main library process would do exit(0).

Ah, thx for info. Makes sense to me.

> > Other option could be to keep library as is and have a custm functions which
> > "translate" LTP exit codes. I was thinking something like this:

> > bool tst_exit_status(int status)
> > {
> > 	if (!WEXITSTATUS(status))
> > 		return false;

> > 	if (WEXITSTATUS(status) == TPASS || WEXITSTATUS(status) == TCONF)
> > 		return true;

> > 	return false;
> > }

> > But we actually have check_child_status(pid_t pid, int status) for this:

> > static void check_child_status(pid_t pid, int status)
> > {
> > 	int ret;

> > 	if (WIFSIGNALED(status)) {
> > 		tst_brk(TBROK, "Child (%i) killed by signal %s", pid,
> > 			tst_strsig(WTERMSIG(status)));
> > 	}

> > 	if (!(WIFEXITED(status)))
> > 		tst_brk(TBROK, "Child (%i) exited abnormally", pid);

> > 	ret = WEXITSTATUS(status);
> > 	switch (ret) {
> > 	case TPASS:
> > 	case TBROK:
> > 	case TCONF:
> > 	break;
> > 	default:
> > 		tst_brk(TBROK, "Invalid child (%i) exit value %i", pid, ret);
> > 	}
> > }

> > BTW I'm surprised that there is TBROK whitelisted.

> That just checks for valid exit status.

OK. Shouldn't be here TFAIL as well (or even TWARN)?

> > > but that would require a bit more work, especially we would
> > > have to look at all WAITPID() usages in newlib tests and make sure that
> > > there are not tests that depends on such behavior. If there are none it
> > > would stil require a few changes in the test library.

> > Well, what I fear more is to broke even more tests :). But if you consider is
> > useful enough I can do the cleanup.

> The change would allow us to implement things like tst_brk(TFAIL, ...)
> and tst_brk(TPASS, ...) correctly. The problem is that the result from
> children after tst_brk() is propagated as a return value. What we need
> instead is to add an abort flag to the results structure.

Ah, now I see why you want to do it properly. Makes sense.

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Petr Vorel]

Hi all,

in the end we decided with Cyril to simply revert [1] the commit which caused
the regression [2].

It will be applied after Cyril fixes the library.

Thanks all for your input.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/commit/b21fd2072ba4ef415e8e590abbf431ed97608473
[2] https://github.com/linux-test-project/ltp/commit/ffdd3b36cd90ad3f4e199344d20c00fb06762eb2

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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Martin Doucha]

Hi!

On 13. 11. 24 11:59, Petr Vorel wrote:
> This fixes error on unsupported __NR__newselect:
> select_var.h:89: TCONF: syscall(-1) __NR__newselect not supported on your arch
> select03.c:90: TFAIL: Child exited with 32
> 
> Fixes: ffdd3b36cd ("syscalls: Add missing exit status check")
> Reported-by: Dominique Leuenberger <dleuenberger@suse.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Alternatively, we could revert to previous state (remove
> "!WEXITSTATUS(status)" check), if we really don't care about any other
> exit code.

As long as all errors are reported by the child through 
tst_res()/tst_brk(), we don't need to care about the exit code since 
it'll be recorded in the shared result buffer. I vote for removing the 
WEXITSTATUS() check and adding a comment above the condition that it's 
not needed.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH 1/1] select03: Fix false positive on TCONF

[Cyril Hrubis]

Hi!
> As long as all errors are reported by the child through 
> tst_res()/tst_brk(), we don't need to care about the exit code since 
> it'll be recorded in the shared result buffer. I vote for removing the 
> WEXITSTATUS() check and adding a comment above the condition that it's 
> not needed.

I think that doing exit(TTYPE_RESULT(type)) in the tst_vbrk_() for the
child processes was a mistake that needs to be corrected either way. I
will send a patch for the test library to finally fix that and then
there will be no cases where the child exits with anything but 0.

-- 
Cyril Hrubis
chrubis@suse.cz

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