diff mbox

[v2] test-skeleton: Kill any child process's offspring

Message ID alpine.DEB.1.10.1406232023480.25395@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki June 30, 2014, 9:15 a.m. UTC 
On Mon, 23 Jun 2014, Roland McGrath wrote:

> I don't know what you did to test that change.  I can't see how it could
> possibly work as intended.  You've changed the value of PID in the child,
> but it's only actually used in the parent (where it won't have changed).
> Am I missing something?

 That my brain long needed the time off that I took last week perhaps, 
sigh...

> Without more futzing than is worthwhile, there is no way that the child can
> communicate back to the parent whether setpgid succeeded or failed.  The
> error message is nice to have, but I don't think the parent actually needs
> to care whether setpgid worked or not.  If it didn't, there won't be any
> pgrp with pgid -pid.

 That's sort-of why I argued there's no need to check for an error from 
setpgid (0, 0) in the first place (of course with other sets of arguments 
setpgid can indeed fail).

> So I think all you really need is to make the handler do:
> 
>   assert (pid > 1);
>   /* Kill the whole process group.  */
>   kill (-pid, SIGKILL);
>   /* In case, setpgid failed in the child, kill it individually too.  */
>   kill (pid, SIGKILL);

 Thanks, here's the resulting implementation.  I put it through a test run 
so as to avoid any surprises.  It produced one `Timed out: killed the 
child process' message on the way and I saw no weird stuff.

 OK to apply?

2014-06-30  Maciej W. Rozycki  <macro@codesourcery.com>
            Roland McGrath <roland@hack.frob.com>

	* test-skeleton.c (signal_handler): Kill the whole process group
	before killing the child individually.
	(main): Report any failure on `setpgid'.

  Maciej

glibc-test-skeleton-kill-pgid.diff

Comments

Roland McGrath June 30, 2014, 6:22 p.m. UTC | #1 
> +  /* In case, setpgid failed in the child, kill it individually too.  */
>    kill (pid, SIGKILL);

Drop the first comma.  Otherwise all looks right to me.
Maciej W. Rozycki June 30, 2014, 7:18 p.m. UTC | #2 
On Mon, 30 Jun 2014, Roland McGrath wrote:

> > +  /* In case, setpgid failed in the child, kill it individually too.  */
> >    kill (pid, SIGKILL);
> 
> Drop the first comma.  Otherwise all looks right to me.

 Applied.  Thanks for your effort with the review.

  Maciej
diff mbox

Patch

Index: glibc-fsf-trunk-quilt/test-skeleton.c
===================================================================
--- glibc-fsf-trunk-quilt.orig/test-skeleton.c	2014-06-23 02:07:41.000000000 +0100
+++ glibc-fsf-trunk-quilt/test-skeleton.c	2014-06-23 21:59:15.801805581 +0100
@@ -17,6 +17,7 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
@@ -138,7 +139,10 @@  signal_handler (int sig __attribute__ ((
   int killed;
   int status;
 
-  /* Send signal.  */
+  assert (pid > 1);
+  /* Kill the whole process group.  */
+  kill (-pid, SIGKILL);
+  /* In case, setpgid failed in the child, kill it individually too.  */
   kill (pid, SIGKILL);
 
   /* Wait for it to terminate.  */
@@ -342,7 +346,8 @@  main (int argc, char *argv[])
 
       /* We put the test process in its own pgrp so that if it bogusly
 	 generates any job control signals, they won't hit the whole build.  */
-      setpgid (0, 0);
+      if (setpgid (0, 0) != 0)
+	printf ("Failed to set the process group ID: %m\n");
 
       /* Execute the test function and exit with the return value.   */
       exit (TEST_FUNCTION);