Re: prob: fg not sending CONT to 'make' children

Re: prob: fg not sending CONT to 'make' children

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Aug 30, 10:21am, Sven Wischnowsky wrote:
> } Subject: Re: prob: fg not sending CONT to 'make' children
> }
> } Bart Schaefer wrote:
> } 
> } > I think this must be a case where process group management was updated
> } > elsewhere but the change was not reflected in killjb().  The following
> } > patch resumes the make properly in this particular example, but I'm not
> } > sure it isn't an over-use of killpg() -- Sven, can you comment?
> } 
> } I think we could add some extra safety by first trying killpg() and
> } then using kill() if that fails.
> 
> As long as we're on the subject ...
> 
> Why does killjb() return failure only if the last attempted kill failed?
> Is the second-to-last job in the pn->next chain special for some reason?
> Or should it be returning failure when any of the kills failed?

Yes, I have been wondering about this, too for some time now (I don't
remember writing it that way in the first place, but I may be wrong,
and if I did, I don't remember why I wrote it like that).

The patch below makes the return value be -1 if at least one of the
kill()s failed.


NOTE: while playing with this I found a bug when I did `ls|sleep 800'
where the `ls' finished before the `sleep' process was set up -- the
`sleep' put itself in its own little process group and one couldn't fg 
the whole thing. I tried several ways to fix this now and the hunks in 
`exec.c' and `utils.c' are the only ones which made this work and
didn't break things like `ls|foo' where `foo' is a function.
We'll have to see if anything else fails (although at least the hunk
in `utils.c' seems to make sense anyway and the one in `exec.c' is
hopefully the cleanest way to handle dead group leaders).


Bye
 Sven

diff -u os/signals.c Src/signals.c
--- os/signals.c	Tue Aug 31 09:11:02 1999
+++ Src/signals.c	Tue Aug 31 11:55:04 1999
@@ -592,22 +592,44 @@
             if (sig == SIGCONT) {
                 for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
                     if (killpg(pn->pid, sig) == -1)
-			kill(pn->pid, sig);
+			if (kill(pn->pid, sig) == -1 && errno != ESRCH)
+#ifdef BROKEN_KILL_ESRCH
+			    if(errno != EINVAL || sig != 0)
+#endif /* BROKEN_KILL_ESRCH */
+				err = -1;
  
                 for (pn = jn->procs; pn->next; pn = pn->next)
-                    err = kill(pn->pid, sig);
+                    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
+#ifdef BROKEN_KILL_ESRCH
+			if(errno != EINVAL || sig != 0)
+#endif /* BROKEN_KILL_ESRCH */
+			    err = -1;
 
 		if (!jobtab[jn->other].procs && pn)
-		    err = kill(pn->pid, sig);
+		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
+#ifdef BROKEN_KILL_ESRCH
+			if(errno != EINVAL || sig != 0)
+#endif /* BROKEN_KILL_ESRCH */
+			    err = -1;
 
                 return err;
             }
- 
-            killpg(jobtab[jn->other].gleader, sig);
-            return killpg(jn->gleader, sig);
+            if (killpg(jobtab[jn->other].gleader, sig) == -1 && errno != ESRCH)
+#ifdef BROKEN_KILL_ESRCH
+		if(errno != EINVAL || sig != 0)
+#endif /* BROKEN_KILL_ESRCH */
+		    err = -1;
+		
+	    if (killpg(jn->gleader, sig) == -1 && errno != ESRCH)
+#ifdef BROKEN_KILL_ESRCH
+		if(errno != EINVAL || sig != 0)
+#endif /* BROKEN_KILL_ESRCH */
+		    err = -1;
+
+	    return err;
         }
         else
-            return (killpg(jn->gleader, sig));
+	    return killpg(jn->gleader, sig);
     }
     for (pn = jn->procs; pn; pn = pn->next)
         if ((err = kill(pn->pid, sig)) == -1 && errno != ESRCH)
diff -u os/exec.c Src/exec.c
--- os/exec.c	Tue Aug 31 09:10:58 1999
+++ Src/exec.c	Tue Aug 31 13:22:15 1999
@@ -1760,6 +1760,13 @@
 #ifdef PATH_DEV_FD
 	    closem(2);
 #endif
+
+	    /* If there is already a group leader but that has died, we make
+	     * this one the leader. */
+	    if (pline_level == 1 && jobtab[thisjob].procs &&
+		kill(jobtab[thisjob].gleader, 0) == -1)
+		jobtab[thisjob].gleader = pid;
+
 	    if (how & Z_ASYNC) {
 		lastpid = (zlong) pid;
 	    } else if (!jobtab[thisjob].stty_in_env &&
diff -u os/utils.c Src/utils.c
--- os/utils.c	Tue Aug 31 09:11:03 1999
+++ Src/utils.c	Tue Aug 31 13:16:56 1999
@@ -2438,7 +2438,7 @@
 # endif
 #endif
 	{
-	    if (pgrp != mypgrp && kill(pgrp, 0) == -1)
+	    if (pgrp != mypgrp && kill(-pgrp, 0) == -1)
 		attachtty(mypgrp);
 	    else {
 		if (errno != ENOTTY)

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: prob: fg not sending CONT to 'make' children

[Bart Schaefer]

On Aug 31,  1:25pm, Sven Wischnowsky wrote:
} Subject: Re: prob: fg not sending CONT to 'make' children
}
} The patch below makes the return value be -1 if at least one of the
} kill()s failed.

Really?  It doesn't look to me as though it changes that behavior at all.
And all those #ifdefs make it really hard to read, and testing sig != 0 is
redundant because the whole thing is inside "if (sig == SIGCONT) ...".

} NOTE: while playing with this I found a bug when I did `ls|sleep 800'
} where the `ls' finished before the `sleep' process was set up -- the
} `sleep' put itself in its own little process group and one couldn't fg 
} the whole thing. I tried several ways to fix this now and the hunks in 
} `exec.c' and `utils.c' are the only ones which made this work

That's very odd.  I didn't think the PID passed to killpg() should change
if you still want to signal the whole group, even if the group leader had
exited.  I wonder what happens if there are more than two processes in
the group when the leader exits?  Do all the leader's children become
their own little leaders?

Hmm, this is something I used to know, about 12 years ago ...

Index: Src/jobs.c
===================================================================
@@ -835,11 +835,7 @@
 
     /* child_block() around this loop in case #ifndef WNOHANG */
     child_block();		/* unblocked in child_suspend() */
-#ifdef BROKEN_KILL_ESRCH
-    while (!errflag && (kill(pid, 0) >= 0 || (errno != ESRCH && errno != EINVAL))) {
-#else /* not BROKEN_KILL_ESRCH */
     while (!errflag && (kill(pid, 0) >= 0 || errno != ESRCH)) {
-#endif /* BROKEN_KILL_ESRCH */
 	if (first)
 	    first = 0;
 	else
Index: Src/signals.c
===================================================================
@@ -593,38 +593,23 @@
                 for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
                     if (killpg(pn->pid, sig) == -1)
 			if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-			    if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
-				err = -1;
+			    err |= -1;
  
                 for (pn = jn->procs; pn->next; pn = pn->next)
                     if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-			if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
-			    err = -1;
+			err |= -1;
 
 		if (!jobtab[jn->other].procs && pn)
 		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-			if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
-			    err = -1;
+			err |= -1;
 
                 return err;
             }
             if (killpg(jobtab[jn->other].gleader, sig) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-		if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
-		    err = -1;
+		err = -1;
 		
 	    if (killpg(jn->gleader, sig) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-		if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
-		    err = -1;
+		err |= -1;
 
 	    return err;
         }
@@ -632,10 +617,7 @@
 	    return killpg(jn->gleader, sig);
     }
     for (pn = jn->procs; pn; pn = pn->next)
-        if ((err = kill(pn->pid, sig)) == -1 && errno != ESRCH)
-#ifdef BROKEN_KILL_ESRCH
-          if(errno != EINVAL || sig != 0)
-#endif /* BROKEN_KILL_ESRCH */
+        if ((err = kill(pn->pid, sig)) == -1 && errno != ESRCH && sig != 0)
             return -1;
     return err;
 }
Index: Src/system.h
===================================================================
@@ -620,3 +620,8 @@
 #ifdef BROKEN_TCSETPGRP
 #undef JOB_CONTROL
 #endif /* BROKEN_TCSETPGRP */
+
+#ifdef BROKEN_KILL_ESRCH
+#undef ESRCH
+#define ESRCH EINVAL
+#endif /* BROKEN_KILL_ESRCH */

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: prob: fg not sending CONT to 'make' children

[Bart Schaefer]

On Sep 1, 10:08am, Sven Wischnowsky wrote:
} Subject: Re: prob: fg not sending CONT to 'make' children
}
} Bart Schaefer wrote:
} 
} > Really?  It doesn't look to me as though it changes that behavior at all.
} 
} So, err is only set to -1 if (and always if) a kill fails with an
} intersting error and if another kill() later succeeds it will not be
} reset to zero -- that's certainly different from before, isn't it?

Yes, it is; sorry about that.  I'd already done a patch to my local copy
where I used `err |= kill(...);' rather than `if (kill(...)) err = -1;'
and I didn't think hard enough when resolving the conflicts.


Index: Src/signals.c
===================================================================
@@ -593,15 +593,15 @@
                 for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
                     if (killpg(pn->pid, sig) == -1)
 			if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-			    err |= -1;
+			    err = -1;
  
                 for (pn = jn->procs; pn->next; pn = pn->next)
                     if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-			err |= -1;
+			err = -1;
 
 		if (!jobtab[jn->other].procs && pn)
 		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
-			err |= -1;
+			err = -1;
 
                 return err;
             }
@@ -609,7 +609,7 @@
 		err = -1;
 		
 	    if (killpg(jn->gleader, sig) == -1 && errno != ESRCH)
-		err |= -1;
+		err = -1;
 
 	    return err;
         }

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: prob: fg not sending CONT to 'make' children

[Sven Wischnowsky]

I wrote:

> (Damn. After thinking again: there is a problem with `a | b | c' if `b' 
> is started when `a' still lives and `c' is started when `a' is dead.
> I'll have to look at it again. Sigh.)
> 
> The problem is in `entersubsh()' where we use `kill(..)' to test if a
> forked process has to put itself in its own process group. I tried
> to change this to `killpg(..)' and it fixed the problem with `a | b'
> where `b' is a command. But it also made that fail if `b' is a shell
> construct (or function). There is some code elsewhere which makes this 
> work with the `kill(..)' for pipes ending in shell constructs -- I
> have to find that place again to make it work with normal pipes, too.

The problem was only in bin_fg(), it seems where it checked if the
leader was dead and then attached the tty to the sub-job's leader.

So this patch removes the nasty the-leader-is-dead-long-live-the-leader
and instead uses the killpg() in entersubsh(). Then it changes the
test in bin_fg().

Bye
 Sven

diff -u os/exec.c Src/exec.c
--- os/exec.c	Wed Sep  1 11:59:38 1999
+++ Src/exec.c	Wed Sep  1 14:05:52 1999
@@ -1760,13 +1760,6 @@
 #ifdef PATH_DEV_FD
 	    closem(2);
 #endif
-
-	    /* If there is already a group leader but that has died, we make
-	     * this one the leader. */
-	    if (pline_level == 1 && jobtab[thisjob].procs &&
-		kill(jobtab[thisjob].gleader, 0) == -1)
-		jobtab[thisjob].gleader = pid;
-
 	    if (how & Z_ASYNC) {
 		lastpid = (zlong) pid;
 	    } else if (!jobtab[thisjob].stty_in_env &&
@@ -2242,7 +2235,7 @@
 	}
     } else if (thisjob != -1 && cl) {
 	if (jobtab[list_pipe_job].gleader && (list_pipe || list_pipe_child)) {
-	    if (kill(jobtab[list_pipe_job].gleader, 0) == -1 ||
+	    if (killpg(jobtab[list_pipe_job].gleader, 0) == -1 ||
 		setpgrp(0L, jobtab[list_pipe_job].gleader) == -1) {
 		jobtab[list_pipe_job].gleader =
 		    jobtab[thisjob].gleader = (list_pipe_child ? mypgrp : getpid());
diff -u os/jobs.c Src/jobs.c
--- os/jobs.c	Wed Sep  1 11:59:38 1999
+++ Src/jobs.c	Wed Sep  1 14:05:41 1999
@@ -1292,8 +1292,7 @@
 		    thisjob = job;
 		    if ((jobtab[job].stat & STAT_SUPERJOB) &&
 			((!jobtab[job].procs->next ||
-			  WIFEXITED(jobtab[job].procs->status) ||
-			  WIFSIGNALED(jobtab[job].procs->status))) &&
+			  killpg(jobtab[job].gleader, 0) == -1)) &&
 			jobtab[jobtab[job].other].gleader)
 			attachtty(jobtab[jobtab[job].other].gleader);
 		    else

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: prob: fg not sending CONT to 'make' children

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Aug 31,  1:25pm, Sven Wischnowsky wrote:
> } Subject: Re: prob: fg not sending CONT to 'make' children
> }
> } The patch below makes the return value be -1 if at least one of the
> } kill()s failed.
> 
> Really?  It doesn't look to me as though it changes that behavior at all.
> And all those #ifdefs make it really hard to read, and testing sig != 0 is
> redundant because the whole thing is inside "if (sig == SIGCONT) ...".

(Sorry about the #ifdefs, I just copied them froom the end of the
function. I certainly should have noticed the `sig!=0' and probably
that they were not needed anyway, given the definiton in system.h.)

But: behavior not changed? Before it was:

                for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
                    if (killpg(pn->pid, sig) == -1)
			kill(pn->pid, sig);
 
                for (pn = jn->procs; pn->next; pn = pn->next)
                    err = kill(pn->pid, sig);

		if (!jobtab[jn->other].procs && pn)
		    err = kill(pn->pid, sig);

                return err;

I changed it to (without the #ifdefs):

                for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
                    if (killpg(pn->pid, sig) == -1)
			if (kill(pn->pid, sig) == -1 && errno != ESRCH)
			    err = -1;
 
                for (pn = jn->procs; pn->next; pn = pn->next)
                    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
			err = -1;

		if (!jobtab[jn->other].procs && pn)
		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
			err = -1;

                return err;

So, err is only set to -1 if (and always if) a kill fails with an
intersting error and if another kill() later succeeds it will not be
reset to zero -- that's certainly different from before, isn't it?
(And how does your change to `|= -1' change the behavior?)

> } NOTE: while playing with this I found a bug when I did `ls|sleep 800'
> } where the `ls' finished before the `sleep' process was set up -- the
> } `sleep' put itself in its own little process group and one couldn't fg 
> } the whole thing. I tried several ways to fix this now and the hunks in 
> } `exec.c' and `utils.c' are the only ones which made this work
> 
> That's very odd.  I didn't think the PID passed to killpg() should change
> if you still want to signal the whole group, even if the group leader had
> exited.  I wonder what happens if there are more than two processes in
> the group when the leader exits?  Do all the leader's children become
> their own little leaders?

(Damn. After thinking again: there is a problem with `a | b | c' if `b' 
is started when `a' still lives and `c' is started when `a' is dead.
I'll have to look at it again. Sigh.)

The problem is in `entersubsh()' where we use `kill(..)' to test if a
forked process has to put itself in its own process group. I tried
to change this to `killpg(..)' and it fixed the problem with `a | b'
where `b' is a command. But it also made that fail if `b' is a shell
construct (or function). There is some code elsewhere which makes this 
work with the `kill(..)' for pipes ending in shell constructs -- I
have to find that place again to make it work with normal pipes, too.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: prob: fg not sending CONT to 'make' children

[Bart Schaefer]

On Aug 30, 10:21am, Sven Wischnowsky wrote:
} Subject: Re: prob: fg not sending CONT to 'make' children
}
} Bart Schaefer wrote:
} 
} > I think this must be a case where process group management was updated
} > elsewhere but the change was not reflected in killjb().  The following
} > patch resumes the make properly in this particular example, but I'm not
} > sure it isn't an over-use of killpg() -- Sven, can you comment?
} 
} I think we could add some extra safety by first trying killpg() and
} then using kill() if that fails.

As long as we're on the subject ...

Why does killjb() return failure only if the last attempted kill failed?
Is the second-to-last job in the pn->next chain special for some reason?
Or should it be returning failure when any of the kills failed?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: prob: fg not sending CONT to 'make' children

[Sven Wischnowsky]

Bart Schaefer wrote:

> Note the negated PID, which turns the kill() into a killpg().  So gmake is
> a process group leader, though whether it did that itself or zsh did it,
> I'm not entirely sure; but I tend to "blame" zsh because 3.0.5 and 3.1.5
> do not have the same problem.
> 
> I think this must be a case where process group management was updated
> elsewhere but the change was not reflected in killjb().  The following
> patch resumes the make properly in this particular example, but I'm not
> sure it isn't an over-use of killpg() -- Sven, can you comment?

I hope: this is connected to 6824 which allowed such processes
(started from a function, no pipe) to use their own process group --
more precisely, it tried to handle such processes in their own group
correctly. I just didn't remember to change killjb() for that. Never
tried it with a command that actually needed that, it seems.

I think we could add some extra safety by first trying killpg() and
then using kill() if that fails.

Bye
 Sven

--- os/signals.c	Mon Aug 30 10:01:46 1999
+++ Src/signals.c	Mon Aug 30 10:21:42 1999
@@ -591,7 +591,8 @@
         if (jn->stat & STAT_SUPERJOB) {
             if (sig == SIGCONT) {
                 for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
-                    killpg(pn->pid, sig);
+                    if (killpg(pn->pid, sig) == -1)
+			kill(pn->pid, sig);
  
                 for (pn = jn->procs; pn->next; pn = pn->next)
                     err = kill(pn->pid, sig);

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: prob: fg not sending CONT to 'make' children

[Bart Schaefer]

On Aug 29,  3:35am, Will Day wrote:
} Subject: prob: fg not sending CONT to 'make' children
}
} I ran into the following problem under 3.1.6 while doing a recompile; after
} stopping (ctrl-Z) the make process, doing an 'fg' would not continue the
} make process.  Looking at the process tree, the make itself is continued,
} but its child process is still stopped.

There seems to be something wrong in signals.c:killjb().

} Another datapoint is that I generally use a shell function to run make; if
} I _don't_ use the function, it works properly.

I'm able to reproduce this in both 3.0.6 and 3.1.6-pws-1 on my linux box,
using exactly this function:

} 	% gm() {gmake $*}

I was able to continue the make by running `ps' to get the gmake process
ID (in my case it was 2888) and then running

    kill -CONT -2888 && fg

Note the negated PID, which turns the kill() into a killpg().  So gmake is
a process group leader, though whether it did that itself or zsh did it,
I'm not entirely sure; but I tend to "blame" zsh because 3.0.5 and 3.1.5
do not have the same problem.

I think this must be a case where process group management was updated
elsewhere but the change was not reflected in killjb().  The following
patch resumes the make properly in this particular example, but I'm not
sure it isn't an over-use of killpg() -- Sven, can you comment?

Index: Src/signals.c
===================================================================
@@ -591,7 +591,7 @@
         if (jn->stat & STAT_SUPERJOB) {
             if (sig == SIGCONT) {
                 for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
-                    kill(pn->pid, sig);
+                    killpg(pn->pid, sig);
  
                 for (pn = jn->procs; pn->next; pn = pn->next)
                     err = kill(pn->pid, sig);

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

prob: fg not sending CONT to 'make' children

[Will Day]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

I ran into the following problem under 3.1.6 while doing a recompile; after
stopping (ctrl-Z) the make process, doing an 'fg' would not continue the
make process.  Looking at the process tree, the make itself is continued,
but its child process is still stopped.

Another datapoint is that I generally use a shell function to run make; if
I _don't_ use the function, it works properly.

Here's a distilled example:
	% cat Makefile
	  default:
	  	read i
	% zsh -f
	% echo $$
	1106
	% gm() {gmake $*}
	% gm
	read i
	^Zzsh: suspended  gm
	% fg
	[1]  + continued  gm

At this point, hitting return after return doesn't finish the make.

On another terminal:
	% /usr/proc/bin/ptree 1106
	220   /usr/local/sbin/sshd
	  21855 /usr/local/sbin/sshd
	    21858 -zsh
	      1106  zsh -f
	        1110  gmake
	          1111  /bin/sh -c read i
	        1113  zsh -f
	% truss -p 1106
	sigsuspend(0xEFFFEB48)          (sleeping...)
	% truss -p 1110
	wait()                          (sleeping...)
	% truss -p 1111
	    Stopped by signal #24, SIGTSTP, in read()
	% truss -p 1113
	    Stopped by signal #23, SIGSTOP

I found it odd that there's a second zsh process there (1113), and didn't
know what to make of it.  

The problem doesn't occur under 3.1.5 (nor does the extra zsh process).
Looking through the code, it was clear things had changed quite a bit, but
I couldn't understand the code well enough to hazard a guess as to what's
causing the problem.

For reference, this is under Solaris 2.6, with zsh-3.1.6 (both with and
without dynamic-loading) as well as with zsh-3.1.6-pws-1.  Gmake is 3.71,
but it occurs with solaris /usr/ccs/bin/make as well.

-- 
Will Day     <PGP mail preferred>     OIT / O&E / Technical Support
willday@rom.oit.gatech.edu            Georgia Tech, Atlanta 30332-0715
  -> Opinions expressed are mine alone and do not reflect OIT policy <-
Those who would give up essential Liberty, to purchase a little temporary
Safety, deserve neither Liberty nor Safety.
    Benjamin Franklin, Pennsylvania Assembly, Nov. 11, 1755

[-- Attachment #2: Type: application/pgp-signature, Size: 344 bytes --]