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

[Petr Vorel <pvorel@suse.cz>]

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