[LTP] [PATCH] waitpid04: Convert to new API

[LTP] [PATCH] waitpid04: Convert to new API

[Martin Doucha]

Convert waitpid() error state checks to new API, fix bugs in the original
test and add ESRCH subtest.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/syscalls/waitpid/waitpid04.c | 171 +++++-------------
 1 file changed, 45 insertions(+), 126 deletions(-)

diff --git a/testcases/kernel/syscalls/waitpid/waitpid04.c b/testcases/kernel/syscalls/waitpid/waitpid04.c
index abf94eed1..ab3c1a4f4 100644
--- a/testcases/kernel/syscalls/waitpid/waitpid04.c
+++ b/testcases/kernel/syscalls/waitpid/waitpid04.c
@@ -1,144 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ * Copyright (c) International Business Machines  Corp., 2001
+ * Copyright (c) 2024 SUSE LLC <mdoucha@suse.cz>
  */
 
-/*
- * NAME
- *	waitpid04.c
- *
- * DESCRIPTION
- *	test to check the error conditions in waitpid sys call
+/*\
+ * [Description]
  *
- * USAGE:  <for command-line>
- *      waitpid04 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
- *      where,  -c n : Run n copies concurrently.
- *              -e   : Turn on errno logging.
- *              -i n : Execute test n times.
- *              -I x : Execute test for x seconds.
- *              -P x : Pause for x seconds between iterations.
- *              -t   : Turn on syscall timing.
- *
- * History
- *	07/2001 John George
- *		-Ported
- *
- * Restrictions
- *	NONE
+ * Test to check the error conditions in waitpid() syscall.
  */
 
 #include <sys/signal.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-#include <errno.h>
-#include "test.h"
-
-static void setup(void);
-static void cleanup(void);
-
-char *TCID = "waitpid04";
-int TST_TOTAL = 1;
-
-#define INVAL_FLAG	-1
-
-static int flag, condition_number;
-
-int main(int ac, char **av)
+#include "tst_test.h"
+
+#define TCFMT "waipid(%d, NULL, 0x%x)"
+#define TCFMTARGS(tc) (tc)->pid, (tc)->flags
+
+static struct testcase {
+	pid_t pid;
+	int flags;
+	int err;
+} testcase_list[] = {
+	{-1, 0, ECHILD},	/* Wait for any child when none exist */
+	{1, 0, ECHILD},		/* Wait for non-child process */
+	{-1, -1, EINVAL},	/* Invalid flags */
+	{INT_MIN, 0, ESRCH},	/* Wait for invalid process group */
+};
+
+void run(unsigned int n)
 {
-	int pid, status, ret;
-
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	/* check for looping state if -i option is given */
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		/* reset tst_count in case we are looping */
-		tst_count = 0;
-
-		ret = waitpid(pid, &status, WNOHANG);
-		flag = 0;
-		condition_number = 1;
-		if (ret != -1) {
-			tst_resm(TFAIL, "condition %d test failed",
-				 condition_number);
-		} else {
-			if (errno != ECHILD) {
-				tst_resm(TFAIL, "waitpid() set invalid "
-					 "errno, expected ECHILD, got: %d",
-					 errno);
-			} else {
-				tst_resm(TPASS, "condition %d test passed",
-					 condition_number);
-			}
-		}
-		condition_number++;
+	const struct testcase *tc = testcase_list + n;
 
-		if (FORK_OR_VFORK() == 0)
-			exit(0);
+	TEST(waitpid(tc->pid, NULL, tc->flags));
 
-		pid = 1;
-		ret = waitpid(pid, &status, WUNTRACED);
-		flag = 0;
-		if (ret != -1) {
-			tst_resm(TFAIL, "condition %d test failed",
-				 condition_number);
-		} else {
-			if (errno != ECHILD) {
-				tst_resm(TFAIL, "waitpid() set invalid "
-					 "errno, expected ECHILD, got: %d",
-					 errno);
-			} else {
-				tst_resm(TPASS, "condition %d test passed",
-					 condition_number);
-			}
-		}
-		condition_number++;
+	if (TST_RET == -1 && TST_ERR == tc->err) {
+		tst_res(TPASS | TTERRNO, TCFMT " failed as expected",
+			TCFMTARGS(tc));
+		return;
+	}
 
-		/* Option is Inval = INVAL_FLAG */
-		ret = waitpid(pid, &status, INVAL_FLAG);
-		flag = 0;
-		if (ret != -1) {
-			tst_resm(TFAIL, "condition %d test failed",
-				 condition_number);
-		} else {
-			if (errno != EINVAL) {
-				tst_resm(TFAIL, "waitpid() set invalid "
-					 "errno, expected EINVAL, got: %d",
-					 errno);
-			} else {
-				tst_resm(TPASS, "condition %d test passed",
-					 condition_number);
-			}
-		}
-		condition_number++;
+	if (TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got",
+			TCFMTARGS(tc), tst_strerrno(tc->err));
+		return;
 	}
 
-	cleanup();
-	tst_exit();
-}
+	if (TST_RET < 0) {
+		tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld",
+			TCFMTARGS(tc), TST_RET);
+		return;
+	}
 
-static void setup(void)
-{
-	TEST_PAUSE;
+	tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc),
+		TST_RET);
 }
 
-static void cleanup(void)
-{
-}
+static struct tst_test test = {
+	.test = run,
+	.tcnt = ARRAY_SIZE(testcase_list)
+};
-- 
2.42.1


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

Re: [LTP] [PATCH] waitpid04: Convert to new API

[Petr Vorel]

> Convert waitpid() error state checks to new API, fix bugs in the original
> test and add ESRCH subtest.

Very nice cleanup.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW what were these bugs? It looks like old test uses pid O for the first ECHILD
test (not sure how it got defined to 0).

>  #include <sys/signal.h>
nit: this should be <signal.h>
warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp]

Can be fixed before merge.

>  #include <sys/types.h>
>  #include <sys/wait.h>
...
> +#include "tst_test.h"
> +
> +#define TCFMT "waipid(%d, NULL, 0x%x)"
> +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags

checkpatch.pl complains:
waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses

I guess we can ignore this unless you see a quick fix (I guess macro which runs
tst_res() inside would be needed.

> +static struct testcase {
> +	pid_t pid;
> +	int flags;
> +	int err;
> +} testcase_list[] = {
> +	{-1, 0, ECHILD},	/* Wait for any child when none exist */
> +	{1, 0, ECHILD},		/* Wait for non-child process */
> +	{-1, -1, EINVAL},	/* Invalid flags */
> +	{INT_MIN, 0, ESRCH},	/* Wait for invalid process group */
> +};
> +
> +void run(unsigned int n)
>  {
...
> +	const struct testcase *tc = testcase_list + n;

> -		if (FORK_OR_VFORK() == 0)
> -			exit(0);
> +	TEST(waitpid(tc->pid, NULL, tc->flags));
How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid
macros, right?  Or you want to explicitly state what failed?

Kind regards,
Petr

> +	if (TST_RET == -1 && TST_ERR == tc->err) {
> +		tst_res(TPASS | TTERRNO, TCFMT " failed as expected",
> +			TCFMTARGS(tc));
> +		return;
> +	}

> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got",
> +			TCFMTARGS(tc), tst_strerrno(tc->err));
> +		return;
>  	}

> +	if (TST_RET < 0) {
> +		tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld",
> +			TCFMTARGS(tc), TST_RET);
> +		return;
> +	}

> +	tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc),
> +		TST_RET);
>  }

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

Re: [LTP] [PATCH] waitpid04: Convert to new API

[Martin Doucha]

On 31. 01. 24 18:21, Petr Vorel wrote:
>> Convert waitpid() error state checks to new API, fix bugs in the original
>> test and add ESRCH subtest.
> 
> Very nice cleanup.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> BTW what were these bugs? It looks like old test uses pid O for the first ECHILD
> test (not sure how it got defined to 0).

The first subtest used uninitialized pid value in the first iteration 
and then it'd test only pid=1 in all other iterations.

> 
>>   #include <sys/signal.h>
> nit: this should be <signal.h>
> warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp]
> 
> Can be fixed before merge.

Yes, please fix. Strangely, I didn't get this warning during compilation.

>>   #include <sys/types.h>
>>   #include <sys/wait.h>
> ...
>> +#include "tst_test.h"
>> +
>> +#define TCFMT "waipid(%d, NULL, 0x%x)"
>> +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags
> 
> checkpatch.pl complains:
> waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses
> 
> I guess we can ignore this unless you see a quick fix (I guess macro which runs
> tst_res() inside would be needed.

The warning is expected, the macro should expand to multiple arguments. 
Parentheses would prevent that. I could fix it by tst_res(TINFO, 
"waitpid($args)"); before each test.

> 
>> +static struct testcase {
>> +	pid_t pid;
>> +	int flags;
>> +	int err;
>> +} testcase_list[] = {
>> +	{-1, 0, ECHILD},	/* Wait for any child when none exist */
>> +	{1, 0, ECHILD},		/* Wait for non-child process */
>> +	{-1, -1, EINVAL},	/* Invalid flags */
>> +	{INT_MIN, 0, ESRCH},	/* Wait for invalid process group */
>> +};
>> +
>> +void run(unsigned int n)
>>   {
> ...
>> +	const struct testcase *tc = testcase_list + n;
> 
>> -		if (FORK_OR_VFORK() == 0)
>> -			exit(0);
>> +	TEST(waitpid(tc->pid, NULL, tc->flags));
> How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid
> macros, right?  Or you want to explicitly state what failed?

The waitpid() call would be converted to a string that gives no useful 
information about which testcase is actually running and there isn't any 
good error message if the call succeeds.

> 
> Kind regards,
> Petr
> 
>> +	if (TST_RET == -1 && TST_ERR == tc->err) {
>> +		tst_res(TPASS | TTERRNO, TCFMT " failed as expected",
>> +			TCFMTARGS(tc));
>> +		return;
>> +	}
> 
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got",
>> +			TCFMTARGS(tc), tst_strerrno(tc->err));
>> +		return;
>>   	}
> 
>> +	if (TST_RET < 0) {
>> +		tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld",
>> +			TCFMTARGS(tc), TST_RET);
>> +		return;
>> +	}
> 
>> +	tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc),
>> +		TST_RET);
>>   }

-- 
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] waitpid04: Convert to new API

[Petr Vorel]

Hi Martin,

> On 31. 01. 24 18:21, Petr Vorel wrote:
> > > Convert waitpid() error state checks to new API, fix bugs in the original
> > > test and add ESRCH subtest.

> > Very nice cleanup.

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > BTW what were these bugs? It looks like old test uses pid O for the first ECHILD
> > test (not sure how it got defined to 0).

> The first subtest used uninitialized pid value in the first iteration and
> then it'd test only pid=1 in all other iterations.

Thanks for info.

> > >   #include <sys/signal.h>
> > nit: this should be <signal.h>
> > warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp]

> > Can be fixed before merge.

> Yes, please fix. Strangely, I didn't get this warning during compilation.

The problem is visible only on pedantic MUSL. Not a big issue, it would work:

cat /usr/include/sys/signal.h
#include <signal.h>

but it's just better to use the standard header, not the fallback one
(one day MUSL might removes the fallback).

> > >   #include <sys/types.h>
> > >   #include <sys/wait.h>
> > ...
> > > +#include "tst_test.h"
> > > +
> > > +#define TCFMT "waipid(%d, NULL, 0x%x)"
> > > +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags

> > checkpatch.pl complains:
> > waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses

> > I guess we can ignore this unless you see a quick fix (I guess macro which runs
> > tst_res() inside would be needed.

> The warning is expected, the macro should expand to multiple arguments.
> Parentheses would prevent that. I could fix it by tst_res(TINFO,
> "waitpid($args)"); before each test.

Yep, that's what I would do - just to print params before test:

tst_res(TINFO, "waipid(%d, NULL, 0x%x)", tc->pid, tc->flags);
TEST(waitpid(tc->pid, NULL, tc->flags));

> > > +static struct testcase {
> > > +	pid_t pid;
> > > +	int flags;
> > > +	int err;
> > > +} testcase_list[] = {
> > > +	{-1, 0, ECHILD},	/* Wait for any child when none exist */
> > > +	{1, 0, ECHILD},		/* Wait for non-child process */
> > > +	{-1, -1, EINVAL},	/* Invalid flags */
> > > +	{INT_MIN, 0, ESRCH},	/* Wait for invalid process group */
> > > +};
> > > +
> > > +void run(unsigned int n)
> > >   {
> > ...
> > > +	const struct testcase *tc = testcase_list + n;

> > > -		if (FORK_OR_VFORK() == 0)
> > > -			exit(0);
> > > +	TEST(waitpid(tc->pid, NULL, tc->flags));
> > How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid
> > macros, right?  Or you want to explicitly state what failed?

> The waitpid() call would be converted to a string that gives no useful
> information about which testcase is actually running and there isn't any
> good error message if the call succeeds.

Indeed plain TST_EXP_FAIL2() would do it:
	TST_EXP_FAIL2(waitpid(tc->pid, NULL, tc->flags), tc->err);

waitpid04.c:38: TFAIL: waitpid(tc->pid, NULL, tc->flags) expected ESRCH: ECHILD (10)
...
waitpid04.c:38: TFAIL: waitpid(tc->pid, NULL, tc->flags) expected EINVAL: ECHILD (10)

I definitely don't want to push for it, but the output is similar:

	TST_EXP_FAIL2(waitpid(tc->pid, NULL, tc->flags), tc->err,
				  "waipid(%d, NULL, 0x%x)", tc->pid, tc->flags);

When testing that with 2 broken testcase values:

	{-1, 0, ESRCH},		/* pvorel: changed from ECHILD */
	{1, 0, ECHILD},		/* Wait for non-child process */
	{-1, 0, EINVAL},	/* Invalid flags */
	{INT_MIN, 0, ESRCH},  /* pvorel: flag changed from -1 */

waitpid04.c:38: TFAIL: waipid(-1, NULL, 0x0) expected ESRCH: ECHILD (10) /* new */
waitpid04.c:49: TFAIL: waipid(-1, NULL, 0x0): expected error ESRCH, got: ECHILD (10) /* orig */
...

waitpid04.c:38: TFAIL: waipid(-1, NULL, 0x0) expected EINVAL: ECHILD (10) /* new */
waitpid04.c:49: TFAIL: waipid(-1, NULL, 0x0): expected error EINVAL, got: ECHILD (10) /* orig */

But no problem to merge it as is. Sooner or later we should add test specific
checkpatch.pl options to ignore certain warnings.

Kind regards,
Petr

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