PATCH: tty pgrp problem again

PATCH: tty pgrp problem again

[Peter Stephenson]

The last patch I sent didn't handle this case (this is the most difficult
one I've come across, not coincidentally compinstall uses it):

% cat tst
cat /dev/null | while read; do; done
read
% . ./tst
%

where the second read again can't read from the tty.

The only thing for it seems to be to trust the STAT_CURSH flag on the job,
which says the last stage of the pipeline was run in the current shell, and
hence must be in the foreground whatever the job number, which can get
jiggled about if there are different structures like the while-loop above
running in the current shell.  This should work, since that's what it's
there for.  But maybe there are pathological cases [the whole @!$!!! shell,
if you ask me sometimes].  This goes on top of the other one.

(I can't actually swear to understanding why thisjob isn't the job that's
terminating, but it's to do with the pipeline code.)

--- Src/jobs.c.job	Tue May 11 13:28:59 1999
+++ Src/jobs.c	Tue May 11 13:41:21 1999
@@ -183,13 +183,11 @@
 	 * and let it handle its own traps, but always allow the test
 	 * for the pgrp.
 	 */
-	if (job == thisjob) {
-	    if (jn->stat & STAT_CURSH)
-		inforeground = 1;
-	    else {
-		lastval = val;
-		inforeground = 2;
-	    }
+	if (jn->stat & STAT_CURSH)
+	    inforeground = 1;
+	else if (job == thisjob) {
+	    lastval = val;
+	    inforeground = 2;
 	}
     }
 
-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

PATCH: tty pgrp problem again: third attempt

[Peter Stephenson]

Peter Stephenson wrote:
> The only thing for it seems to be to trust the STAT_CURSH flag on the job,
> which says the last stage of the pipeline was run in the current shell, and
> hence must be in the foreground whatever the job number, which can get
> jiggled about if there are different structures like the while-loop above
> running in the current shell.  This should work, since that's what it's
> there for.  But maybe there are pathological cases [the whole @!$!!! shell,
> if you ask me sometimes].  This goes on top of the other one.
> 
> (I can't actually swear to understanding why thisjob isn't the job that's
> terminating, but it's to do with the pipeline code.)

Nope, that's no good.  In something like

% zcat foo.gz | if true; less; fi

the zcat can exit, in which case the test for messing around with the pgrp
comes up.  The code in update_job (called asynchronously as soon as zcat
finishes) doesn't know that the shell part of the code hasn't yet finished,
so it calls attachtty(mypgrp), with the consequence that the end of the
pipeline fails disastrously.

The best I can think of is simply marking the fact when we can't reattach
the tty, and the calling it when we know its safe, which is when the job
that's executing is finally deleted.  At least this seems to fix up the
problem without destroying the previous fixes (which still need applying).

Sven, you're the only person that ever understood the list_pipe stuff. Is
there a better way of handling this?  Should it really go somewhere in
execpline() or execpline2()?

--- Src/jobs.c.try3	Tue May 11 13:41:21 1999
+++ Src/jobs.c	Tue May 11 15:01:35 1999
@@ -201,8 +201,20 @@
 	/* is this job in the foreground of an interactive shell? */
 	if (mypgrp != pgrp && inforeground &&
 	    (jn->gleader == pgrp || (pgrp > 1 && kill(-pgrp, 0) == -1))) {
-	    attachtty(mypgrp);
-	    adjustwinsize(0);   /* check window size and adjust if necessary */
+	    if (list_pipe) {
+		/*
+		 * Oh, dear, we're right in the middle of some confusion
+		 * of shell jobs on the righthand side of a pipeline, so
+		 * it's death to call attachtty() just yet.  Mark the
+		 * fact in the job, so that the attachtty() will be called
+		 * when the job is finally deleted.
+		 */
+		jn->stat |= STAT_ATTACH;
+	    } else {
+		attachtty(mypgrp);
+		/* check window size and adjust if necessary */
+		adjustwinsize(0);
+	    }
 	}
     }
 
@@ -611,6 +623,11 @@
 deletejob(Job jn)
 {
     struct process *pn, *nx;
+
+    if (jn->stat & STAT_ATTACH) {
+	attachtty(mypgrp);
+	adjustwinsize(0);
+    }
 
     pn = jn->procs;
     jn->procs = NULL;
--- Src/zsh.h.try3	Sat May  8 14:31:51 1999
+++ Src/zsh.h	Tue May 11 14:54:58 1999
@@ -611,6 +611,7 @@
 #define STAT_CURSH	(1<<9)	/* last command is in current shell     */
 #define STAT_NOSTTY	(1<<10)	/* the tty settings are not inherited   */
 				/* from this job when it exits.         */
+#define STAT_ATTACH	(1<<11)	/* delay reattaching shell to tty       */
 
 #define SP_RUNNING -1		/* fake status for jobs currently running */
 
-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: PATCH: tty pgrp problem again: third attempt

[Sven Wischnowsky]

Peter Stephenson wrote:

> Sven, you're the only person that ever understood the list_pipe stuff. Is
> there a better way of handling this?  Should it really go somewhere in
> execpline() or execpline2()?

Somehow I think, it would be safer in execpline[2](), but I don't
really remember all the list_pipe stuff and we had some patches from
others for it (and the jobbing and signal code) after I worked on that
part of the code, so I may be wrong.
But while playing a bit, I found at least one more bug. If you do:
`ls | if true; then sleep 10; cat; fi' and ^Z the sleep, then fg the
whole thing again, the shell doesn't get hold of the terminal again
after the cat finishes. If you bg before the fg, the cat doesn't get
control (and neither does the shell).

Maybe I'll look some more into this (but I'm sure you are currently
deeper into all this then I).

Bye
 Sven


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