Re: long-standing tty related issue: wrapped Emacs not suspended

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

Just for clarity, what I'm actually testing is the following.  I'm using
vi as a simpler alternative to emacs --- as Bart noted, emacs may have
additional ishoos of its own.

% foo() { vi; }
% foo
^Z
% bg
# Not sure why bg loses the current job number but assuming this is
# standard behaviour...
% fg %1

This should cleanly restore a full-window vi, which it does after the
last patch, but not before.

Now read on.

Or not.

On Mon, 24 Sep 2018 20:51:06 +0100
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx> wrote:
> - Send SIGCONT when attaching a subjob to the TTY.  This is because
> its stoppedness is invisible to the user, who is (unknowlingly) only
> directly manipulating the superjob, so the shell has to give a helping
> hand.  Comment added.

After a bit more research: we have code to SIGTSTP the superjob if the
subjob does get stopped (as it does here because of the SIGTTOU), but it
doesn't trigger because it expects the job to be STAT_STOPPED already,
whereas that's only going to happen further down the file.  As it *is*
going to happen I think the use of the somestopped flag is good enough,
but as we return immediately we now need to change the job state here.
(The superjob has some code to put itself back to sleep but I hope the
interaction is benign.)

Also, we need to mark the superjob (not just the subjob) as stopped at
this point because if it's just the forked shell it's already suspended
so there's no other place to hang that on.

With that in place, the "stopped = 1" I added to fg/bg to send a
possibly unnecessary SIGCONT isn't needed as the records are correct.

(Hope you're enjoying this.)

One remaining glitch is there's no message that the backgrounded job as
been stopped again because we suppress messages for the job.  This isn't
particularly consistent given we do display the effect of the bg and the
fg.  Not sure what a consistent way forward would be here.

> - Wait for subjob of superjob if waiting for the superjob, else the "fg"
> makes the top-level shell continue immediately, so both jobs are running
> at once.  (The wait for the superjob returns when that exits, even
> though the only process, the forked shell, is stopped at this point.
> It's not clear to me this is correct --- see note on makerunning()
> below.  I think with this behaviour waiting for the subjob may be
> superfluous, but I think it's correct anyway as the whole lot is
> considered to be in the forground.)

Logically, we should wait for the subjob first --- its exit should
provoke the superjob into further action.  So I've swapped it round.

As before, the only way of teasing out problems will be to try this out,
so I'll commit it.

pws

diff --git a/Src/jobs.c b/Src/jobs.c
index c399f1d..f64b46b 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -459,19 +459,29 @@ update_job(Job jn)
 	    jn->ty = (struct ttyinfo *) zalloc(sizeof(struct ttyinfo));
 	    gettyinfo(jn->ty);
 	}
-	if (jn->stat & STAT_STOPPED) {
-	    if (jn->stat & STAT_SUBJOB) {
-		/* If we have `cat foo|while read a; grep $a bar;done'
-		 * and have hit ^Z, the sub-job is stopped, but the
-		 * super-job may still be running, waiting to be stopped
-		 * or to exit. So we have to send it a SIGTSTP. */
-		int i;
-
-		if ((i = super_job(job)))
-		    killpg(jobtab[i].gleader, SIGTSTP);
+	if (jn->stat & STAT_SUBJOB) {
+	    /* If we have `cat foo|while read a; grep $a bar;done'
+	     * and have hit ^Z, the sub-job is stopped, but the
+	     * super-job may still be running, waiting to be stopped
+	     * or to exit. So we have to send it a SIGTSTP. */
+	    int i;
+
+	    if ((i = super_job(job))) {
+		killpg(jobtab[i].gleader, SIGTSTP);
+		/*
+		 * Job may already be stopped if it consists of only the
+		 * forked shell waiting for the subjob -- so mark as
+		 * stopped immediately.  This ensures we send it (and,
+		 * crucially, the subjob, as the visible job used with
+		 * fg/bg is the superjob) a SIGCONT if we need it.
+		 */
+		jobtab[i].stat |= STAT_CHANGED | STAT_STOPPED;
 	    }
+	    jn->stat |= STAT_CHANGED | STAT_STOPPED;
 	    return;
 	}
+	if (jn->stat & STAT_STOPPED)
+	    return;
     }
     {                   /* job is done or stopped, remember return value */
 	lastval2 = val;
@@ -1608,10 +1618,11 @@ waitjobs(void)
     Job jn = jobtab + thisjob;
     DPUTS(thisjob == -1, "No valid job in waitjobs.");
 
-    waitonejob(jn);
+    /* If there's a subjob, it should finish first. */
     if (jn->stat & STAT_SUPERJOB)
 	waitonejob(jobtab + jn->other);
-	
+    waitonejob(jn);
+
     thisjob = -1;
 }
 
@@ -2407,17 +2418,9 @@ bin_fg(char *name, char **argv, Options ops, int func)
 			((!jobtab[job].procs->next ||
 			  (jobtab[job].stat & STAT_SUBLEADER) ||
 			  killpg(jobtab[job].gleader, 0) == -1)) &&
-			jobtab[jobtab[job].other].gleader) {
+			jobtab[jobtab[job].other].gleader)
 			attachtty(jobtab[jobtab[job].other].gleader);
-			/*
-			 * In case stopped by putting in background.
-			 * Usually that's visible to the user, who
-			 * can restart, but with a superjob this is done
-			 * behind the scenes, so do it here.  Should
-			 * be harmless if not needed.
-			 */
-			stopped = 1;
-		    } else
+		    else
 			attachtty(jobtab[job].gleader);
 		}
 	    }