| Message ID | 59CB9FFA.20307@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix posix/tst-spawn test | expand |
On 09/27/2017 02:56 PM, Szabolcs Nagy wrote: > + /* Wait for the child. */ > + if (waitpid (pid, &status, 0) != pid) > + error (EXIT_FAILURE, errno, "wrong child"); You could use TEST_VERIFY (xwaitpid (pid, &status, 0) != pid)); instead. In fact, all the error calls are invalid in tests because they write to standard error. So perhaps use TEST_VERIFY (WIFEXITED (status)); TEST_VERIFY (!WIFSIGNALED (status)); TEST_VERIFY (WEXITSTATUS (status) == 0); The error messages did not contain the status bits anyway, so this is not a regression as far as diagnostics are concerned. The existing WTERMSIG check is invalid because !WIFSIGNALED (status). Thanks, Florian
On 27/09/17 14:00, Florian Weimer wrote: > On 09/27/2017 02:56 PM, Szabolcs Nagy wrote: >> + /* Wait for the child. */ >> + if (waitpid (pid, &status, 0) != pid) >> + error (EXIT_FAILURE, errno, "wrong child"); > > You could use > > TEST_VERIFY (xwaitpid (pid, &status, 0) != pid)); > > instead. > > In fact, all the error calls are invalid in tests because they write to standard error. > > So perhaps use > > TEST_VERIFY (WIFEXITED (status)); > TEST_VERIFY (!WIFSIGNALED (status)); > TEST_VERIFY (WEXITSTATUS (status) == 0); > > The error messages did not contain the status bits anyway, so this is not a regression as far as diagnostics > are concerned. i didn't want to fix the entire test to use the new conventions, so i only changed these lines. The test spawns two children but only waited for one. The fix avoids printing to stderr. 2017-09-27 Szabolcs Nagy <szabolcs.nagy@arm.com> * posix/tst-spawn.c (do_test): Wait for both children. diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c index 08d92bd7a7..851978f224 100644 --- a/posix/tst-spawn.c +++ b/posix/tst-spawn.c @@ -23,9 +23,10 @@ #include <spawn.h> #include <stdlib.h> #include <string.h> -#include <unistd.h> #include <wait.h> #include <sys/param.h> +#include <support/check.h> +#include <support/xunistd.h> /* Nonzero if the program gets called via `exec'. */ @@ -240,22 +241,26 @@ do_test (int argc, char *argv[]) if (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ) != 0) error (EXIT_FAILURE, errno, "posix_spawn"); + /* Wait for the child. */ + TEST_VERIFY (xwaitpid (pid, &status, 0) == pid); + TEST_VERIFY (WIFEXITED (status)); + TEST_VERIFY (!WIFSIGNALED (status)); + TEST_VERIFY (WEXITSTATUS (status) == 0); + /* Same test but with a NULL pid argument. */ if (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ) != 0) error (EXIT_FAILURE, errno, "posix_spawn"); + /* Wait for the child. */ + xwaitpid (-1, &status, 0); + TEST_VERIFY (WIFEXITED (status)); + TEST_VERIFY (!WIFSIGNALED (status)); + TEST_VERIFY (WEXITSTATUS (status) == 0); + /* Cleanup. */ if (posix_spawn_file_actions_destroy (&actions) != 0) error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy"); free (name3_copy); - /* Wait for the child. */ - if (waitpid (pid, &status, 0) != pid) - error (EXIT_FAILURE, errno, "wrong child"); - - if (WTERMSIG (status) != 0) - error (EXIT_FAILURE, 0, "Child terminated incorrectly"); - status = WEXITSTATUS (status); - - return status; + return 0; }
On 09/27/2017 04:40 PM, Szabolcs Nagy wrote: > The test spawns two children but only waited for one. > The fix avoids printing to stderr. > > 2017-09-27 Szabolcs Nagy<szabolcs.nagy@arm.com> > > * posix/tst-spawn.c (do_test): Wait for both children. Sorry, one more thing: I think you should not interleave the spawns and waits. I think you can do a -1 wait after the PID wait, and the test will still pass. This should be closer to the original test. Thanks, Florian
On 27/09/17 15:45, Florian Weimer wrote: > On 09/27/2017 04:40 PM, Szabolcs Nagy wrote: >> The test spawns two children but only waited for one. >> The fix avoids printing to stderr. >> >> 2017-09-27 Szabolcs Nagy<szabolcs.nagy@arm.com> >> >> * posix/tst-spawn.c (do_test): Wait for both children. > > Sorry, one more thing: I think you should not interleave the spawns and waits. I think you can do a -1 wait > after the PID wait, and the test will still pass. This should be closer to the original test. > why does that matter? (attached the updated patch) diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c index 08d92bd7a7..4e5e76351c 100644 --- a/posix/tst-spawn.c +++ b/posix/tst-spawn.c @@ -23,9 +23,10 @@ #include <spawn.h> #include <stdlib.h> #include <string.h> -#include <unistd.h> #include <wait.h> #include <sys/param.h> +#include <support/check.h> +#include <support/xunistd.h> /* Nonzero if the program gets called via `exec'. */ @@ -249,13 +250,16 @@ do_test (int argc, char *argv[]) error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy"); free (name3_copy); - /* Wait for the child. */ - if (waitpid (pid, &status, 0) != pid) - error (EXIT_FAILURE, errno, "wrong child"); + /* Wait for the children. */ + TEST_VERIFY (xwaitpid (pid, &status, 0) == pid); + TEST_VERIFY (WIFEXITED (status)); + TEST_VERIFY (!WIFSIGNALED (status)); + TEST_VERIFY (WEXITSTATUS (status) == 0); - if (WTERMSIG (status) != 0) - error (EXIT_FAILURE, 0, "Child terminated incorrectly"); - status = WEXITSTATUS (status); + xwaitpid (-1, &status, 0); + TEST_VERIFY (WIFEXITED (status)); + TEST_VERIFY (!WIFSIGNALED (status)); + TEST_VERIFY (WEXITSTATUS (status) == 0); - return status; + return 0; }
On 09/27/2017 04:53 PM, Szabolcs Nagy wrote: >> Sorry, one more thing: I think you should not interleave the spawns and waits. I think you can do a -1 wait >> after the PID wait, and the test will still pass. This should be closer to the original test. >> > why does that matter? It checks that you can actually launch multiple child processes. > diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c > index 08d92bd7a7..4e5e76351c 100644 > --- a/posix/tst-spawn.c > +++ b/posix/tst-spawn.c > @@ -23,9 +23,10 @@ > #include <spawn.h> > #include <stdlib.h> > #include <string.h> > -#include <unistd.h> > #include <wait.h> > #include <sys/param.h> > +#include <support/check.h> > +#include <support/xunistd.h> > > > /* Nonzero if the program gets called via `exec'. */ > @@ -249,13 +250,16 @@ do_test (int argc, char *argv[]) > error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy"); > free (name3_copy); > > - /* Wait for the child. */ > - if (waitpid (pid, &status, 0) != pid) > - error (EXIT_FAILURE, errno, "wrong child"); > + /* Wait for the children. */ > + TEST_VERIFY (xwaitpid (pid, &status, 0) == pid); > + TEST_VERIFY (WIFEXITED (status)); > + TEST_VERIFY (!WIFSIGNALED (status)); > + TEST_VERIFY (WEXITSTATUS (status) == 0); > > - if (WTERMSIG (status) != 0) > - error (EXIT_FAILURE, 0, "Child terminated incorrectly"); > - status = WEXITSTATUS (status); > + xwaitpid (-1, &status, 0); > + TEST_VERIFY (WIFEXITED (status)); > + TEST_VERIFY (!WIFSIGNALED (status)); > + TEST_VERIFY (WEXITSTATUS (status) == 0); > > - return status; > + return 0; > } Looks good to me, thanks. Florian
diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c index 08d92bd7a7afdaf1a1abf2f996b00bb107c1d631..c224e6a563327234b134214c98e6123ed99adbc8 100644 --- a/posix/tst-spawn.c +++ b/posix/tst-spawn.c @@ -240,22 +240,30 @@ do_test (int argc, char *argv[]) if (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ) != 0) error (EXIT_FAILURE, errno, "posix_spawn"); + /* Wait for the child. */ + if (waitpid (pid, &status, 0) != pid) + error (EXIT_FAILURE, errno, "wrong child"); + if (WTERMSIG (status) != 0) + error (EXIT_FAILURE, 0, "Child terminated incorrectly"); + if (WEXITSTATUS (status) != 0) + error (EXIT_FAILURE, 0, "Child failed"); + /* Same test but with a NULL pid argument. */ if (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ) != 0) error (EXIT_FAILURE, errno, "posix_spawn"); + /* Wait for the child. */ + if (waitpid (-1, &status, 0) == -1) + error (EXIT_FAILURE, errno, "waitpid failed"); + if (WTERMSIG (status) != 0) + error (EXIT_FAILURE, 0, "Child terminated incorrectly"); + if (WEXITSTATUS (status) != 0) + error (EXIT_FAILURE, 0, "Child failed"); + /* Cleanup. */ if (posix_spawn_file_actions_destroy (&actions) != 0) error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy"); free (name3_copy); - /* Wait for the child. */ - if (waitpid (pid, &status, 0) != pid) - error (EXIT_FAILURE, errno, "wrong child"); - - if (WTERMSIG (status) != 0) - error (EXIT_FAILURE, 0, "Child terminated incorrectly"); - status = WEXITSTATUS (status); - - return status; + return 0; }