Re: fg jobs info

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Tue, 04 Sep 2007 08:31:04 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> Looks to me as if there's a bug here; compare
> 
>     print $(jobs)
>     (print $(jobs))

This is another catalogue of disasters.  Thanks for the extra work.
What's happening here is entersubsh() is being called with the MONITOR
option (job control) already unset so it doesn't know there are jobs to
save.  I've made it an extra flag to entersubsh() and taken the
opportunity to neaten the argument handling.

> Also, although "jobs" or "jobs -l" prints out the entire jobs listing
> even in subshells/pipelines/etc., "jobs %2" (for example) produces a
> "no such job" error, which surprised me.  That should at least be
> documented.  Try:
> 
>     sleep 40 & sleep 30 &
>     jobs | cat
>     jobs %1 | cat

Numbers in the saved job table aren't looked up properly.  There's
another bug that you can't refer to "thisjob" by number but that
restriction doesn't make sense once you're in a subshell that doesn't
have jobs of its own.  I'm not sure if it makes sense to have thisjob !=
-1 in subshells at all, but I've just taken the easy way out this time.

I've made sure you can only look up jobs from within a subshell for
"jobs", not fg, bg, wait, disown where they don't make sense since you
no longer have control of the job.  (Well, maybe you do, but *I* don't.)

> Incidentally, yet another bug:
> 
>     % sleep 30 & sleep 50 &
>     % fg %- %-
>     [1]  - running    sleep 30
>     fg: %3: no such job
> 
> Where did I ever ask for job 3 ?

I've never been quite sure of the handling of the prevjob variable that
remembers the job to which %- applies, but the obvious fix here is to
output the command line argument that caused the failure, not the
possibly nonsensical job number that resulted.

As we previously discussed, there is in any case a race that the job may
be gone (even long gone) by the time the subshell looks at the table,
but that's not really fixable and it's not clear it's a big issue for
the sort of use made of the saved job table.

I'll leave someone else to invent race-free tests.  In fact, tests
generally are a nice job for some philanthropist who doesn't need to
know anything about the source code.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.123
diff -u -r1.123 exec.c
--- Src/exec.c	31 Aug 2007 09:07:46 -0000	1.123
+++ Src/exec.c	4 Sep 2007 20:28:18 -0000
@@ -804,6 +804,105 @@
     return cn;
 }
 
+/**/
+int
+forklevel;
+
+/* Arguments to entersubsh() */
+enum {
+    /* Subshell is to be run asynchronously (else synchronously) */
+    ESUB_ASYNC = 0x01,
+    /*
+     * Perform process group and tty handling and clear the
+     * (real) job table, since it won't be any longer valid
+     */
+    ESUB_PGRP = 0x02,
+    /* Don't unset traps */
+    ESUB_KEEPTRAP = 0x04,
+    /* This is only a fake entry to a subshell */
+    ESUB_FAKE = 0x08,
+    /* Release the process group if pid is the shell's process group */
+    ESUB_REVERTPGRP = 0x10,
+    /* Don't handle the MONITOR option even if previously set */
+    ESUB_NOMONITOR = 0x20
+};
+
+/**/
+static void
+entersubsh(int flags)
+{
+    int sig, monitor;
+
+    if (!(flags & ESUB_KEEPTRAP))
+	for (sig = 0; sig < VSIGCOUNT; sig++)
+	    if (!(sigtrapped[sig] & ZSIG_FUNC))
+		unsettrap(sig);
+    monitor = isset(MONITOR);
+    if (flags & ESUB_NOMONITOR)
+	opts[MONITOR] = 0;
+    if (!isset(MONITOR)) {
+	if (flags & ESUB_ASYNC) {
+	    settrap(SIGINT, NULL, 0);
+	    settrap(SIGQUIT, NULL, 0);
+	    if (isatty(0)) {
+		close(0);
+		if (open("/dev/null", O_RDWR | O_NOCTTY)) {
+		    zerr("can't open /dev/null: %e", errno);
+		    _exit(1);
+		}
+	    }
+	}
+    } else if (thisjob != -1 && (flags & ESUB_PGRP)) {
+	if (jobtab[list_pipe_job].gleader && (list_pipe || list_pipe_child)) {
+	    if (setpgrp(0L, jobtab[list_pipe_job].gleader) == -1 ||
+		killpg(jobtab[list_pipe_job].gleader, 0) == -1) {
+		jobtab[list_pipe_job].gleader =
+		    jobtab[thisjob].gleader = (list_pipe_child ? mypgrp : getpid());
+		setpgrp(0L, jobtab[list_pipe_job].gleader);
+		if (!(flags & ESUB_ASYNC))
+		    attachtty(jobtab[thisjob].gleader);
+	    }
+	}
+	else if (!jobtab[thisjob].gleader ||
+		 setpgrp(0L, jobtab[thisjob].gleader) == -1) {
+	    jobtab[thisjob].gleader = getpid();
+	    if (list_pipe_job != thisjob &&
+		!jobtab[list_pipe_job].gleader)
+		jobtab[list_pipe_job].gleader = jobtab[thisjob].gleader;
+	    setpgrp(0L, jobtab[thisjob].gleader);
+	    if (!(flags & ESUB_ASYNC))
+		attachtty(jobtab[thisjob].gleader);
+	}
+    }
+    if (!(flags & ESUB_FAKE))
+	subsh = 1;
+    if ((flags & ESUB_REVERTPGRP) && getpid() == mypgrp)
+	release_pgrp();
+    if (SHTTY != -1) {
+	shout = NULL;
+	zclose(SHTTY);
+	SHTTY = -1;
+    }
+    if (isset(MONITOR)) {
+	signal_default(SIGTTOU);
+	signal_default(SIGTTIN);
+	signal_default(SIGTSTP);
+    }
+    if (interact) {
+	signal_default(SIGTERM);
+	if (!(sigtrapped[SIGINT] & ZSIG_IGNORED))
+	    signal_default(SIGINT);
+    }
+    if (!(sigtrapped[SIGQUIT] & ZSIG_IGNORED))
+	signal_default(SIGQUIT);
+    opts[MONITOR] = opts[USEZLE] = 0;
+    zleactive = 0;
+    if (flags & ESUB_PGRP)
+	clearjobtab(monitor);
+    get_usage();
+    forklevel = locallevel;
+}
+
 /* execute a string */
 
 /**/
@@ -1301,7 +1400,7 @@
 		    }
 		    else {
 			close(synch[0]);
-			entersubsh(Z_ASYNC, 0, 0, 0);
+			entersubsh(ESUB_ASYNC);
 			if (jobtab[list_pipe_job].procs) {
 			    if (setpgrp(0L, mypgrp = jobtab[list_pipe_job].gleader)
 				== -1) {
@@ -1413,7 +1512,8 @@
 	    } else {
 		zclose(pipes[0]);
 		close(synch[0]);
-		entersubsh(how, 2, 0, 0);
+		entersubsh(((how & Z_ASYNC) ? ESUB_ASYNC : 0)
+			   | ESUB_PGRP | ESUB_KEEPTRAP);
 		close(synch[1]);
 		execcmd(state, input, pipes[1], how, 0);
 		_exit(lastval);
@@ -2419,7 +2519,7 @@
 	  (!is_cursh && (last1 != 1 || nsigtrapped || havefiles()))))) {
 
 	pid_t pid;
-	int synch[2];
+	int synch[2], flags;
 	char dummy;
 	struct timeval bgtime;
 
@@ -2432,7 +2532,9 @@
 	    if (oautocont >= 0)
 		opts[AUTOCONTINUE] = oautocont;
 	    return;
-	} if (pid) {
+	}
+	if (pid) {
+
 	    close(synch[1]);
 	    read(synch[0], &dummy, 1);
 	    close(synch[0]);
@@ -2462,7 +2564,10 @@
 	}
 	/* pid == 0 */
 	close(synch[0]);
-	entersubsh(how, (type != WC_SUBSH) && !(how & Z_ASYNC) ? 2 : 1, 0, 0);
+	flags = ((how & Z_ASYNC) ? ESUB_ASYNC : 0) | ESUB_PGRP;
+	if ((type != WC_SUBSH) && !(how & Z_ASYNC))
+	    flags |= ESUB_KEEPTRAP;
+	entersubsh(flags);
 	close(synch[1]);
 	forked = 1;
 	if (sigtrapped[SIGINT] & ZSIG_IGNORED)
@@ -2737,10 +2842,16 @@
 	 * the current shell (including a fake exec to run a builtin then
 	 * exit) in case there is an error return.
 	 */
-	if (is_exec)
-	    entersubsh(how, (type != WC_SUBSH) ? 2 : 1, 1,
-		       (do_exec || (type >= WC_CURSH && last1 == 1)) 
-		       && !forked);
+	if (is_exec) {
+	    int flags = ((how & Z_ASYNC) ? ESUB_ASYNC : 0) |
+		ESUB_PGRP | ESUB_FAKE;
+	    if (type != WC_SUBSH)
+		flags |= ESUB_KEEPTRAP;
+	    if ((do_exec || (type >= WC_CURSH && last1 == 1)) 
+		&& !forked)
+		flags |= ESUB_REVERTPGRP;
+	    entersubsh(flags);
+	}
 	if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
@@ -3037,83 +3148,6 @@
     errno = old_errno;
 }
 
-/**/
-int
-forklevel;
-
-/**/
-static void
-entersubsh(int how, int cl, int fake, int revertpgrp)
-{
-    int sig, monitor;
-
-    if (cl != 2)
-	for (sig = 0; sig < VSIGCOUNT; sig++)
-	    if (!(sigtrapped[sig] & ZSIG_FUNC))
-		unsettrap(sig);
-    if (!(monitor = isset(MONITOR))) {
-	if (how & Z_ASYNC) {
-	    settrap(SIGINT, NULL, 0);
-	    settrap(SIGQUIT, NULL, 0);
-	    if (isatty(0)) {
-		close(0);
-		if (open("/dev/null", O_RDWR | O_NOCTTY)) {
-		    zerr("can't open /dev/null: %e", errno);
-		    _exit(1);
-		}
-	    }
-	}
-    } else if (thisjob != -1 && cl) {
-	if (jobtab[list_pipe_job].gleader && (list_pipe || list_pipe_child)) {
-	    if (setpgrp(0L, jobtab[list_pipe_job].gleader) == -1 ||
-		killpg(jobtab[list_pipe_job].gleader, 0) == -1) {
-		jobtab[list_pipe_job].gleader =
-		    jobtab[thisjob].gleader = (list_pipe_child ? mypgrp : getpid());
-		setpgrp(0L, jobtab[list_pipe_job].gleader);
-		if (how & Z_SYNC)
-		    attachtty(jobtab[thisjob].gleader);
-	    }
-	}
-	else if (!jobtab[thisjob].gleader ||
-		 setpgrp(0L, jobtab[thisjob].gleader) == -1) {
-	    jobtab[thisjob].gleader = getpid();
-	    if (list_pipe_job != thisjob &&
-		!jobtab[list_pipe_job].gleader)
-		jobtab[list_pipe_job].gleader = jobtab[thisjob].gleader;
-	    setpgrp(0L, jobtab[thisjob].gleader);
-	    if (how & Z_SYNC)
-		attachtty(jobtab[thisjob].gleader);
-	}
-    }
-    if (!fake)
-	subsh = 1;
-    if (revertpgrp && getpid() == mypgrp)
-	release_pgrp();
-    if (SHTTY != -1) {
-	shout = NULL;
-	zclose(SHTTY);
-	SHTTY = -1;
-    }
-    if (isset(MONITOR)) {
-	signal_default(SIGTTOU);
-	signal_default(SIGTTIN);
-	signal_default(SIGTSTP);
-    }
-    if (interact) {
-	signal_default(SIGTERM);
-	if (!(sigtrapped[SIGINT] & ZSIG_IGNORED))
-	    signal_default(SIGINT);
-    }
-    if (!(sigtrapped[SIGQUIT] & ZSIG_IGNORED))
-	signal_default(SIGQUIT);
-    opts[MONITOR] = opts[USEZLE] = 0;
-    zleactive = 0;
-    if (cl)
-	clearjobtab(monitor);
-    get_usage();
-    forklevel = locallevel;
-}
-
 /*
  * Close internal shell fds.
  *
@@ -3307,8 +3341,7 @@
     child_unblock();
     zclose(pipes[0]);
     redup(pipes[1], 1);
-    opts[MONITOR] = 0;
-    entersubsh(Z_SYNC, 1, 0, 0);
+    entersubsh(ESUB_PGRP|ESUB_NOMONITOR);
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1);
     cmdpop();
@@ -3460,8 +3493,7 @@
 
     /* pid == 0 */
     redup(fd, 1);
-    opts[MONITOR] = 0;
-    entersubsh(Z_SYNC, 1, 0, 0);
+    entersubsh(ESUB_PGRP|ESUB_NOMONITOR);
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1);
     cmdpop();
@@ -3532,7 +3564,7 @@
 	zerr("can't open %s: %e", pnam, errno);
 	_exit(1);
     }
-    entersubsh(Z_ASYNC, 1, 0, 0);
+    entersubsh(ESUB_ASYNC|ESUB_PGRP);
     redup(fd, out);
 #else /* PATH_DEV_FD */
     int pipes[2];
@@ -3558,7 +3590,7 @@
 	}
 	return pnam;
     }
-    entersubsh(Z_ASYNC, 1, 0, 0);
+    entersubsh(ESUB_ASYNC|ESUB_PGRP);
     redup(pipes[out], out);
     closem(FDT_UNUSED);   /* this closes pipes[!out] as well */
 #endif /* PATH_DEV_FD */
@@ -3602,7 +3634,7 @@
 	    addproc(pid, NULL, 1, &bgtime);
 	return pipes[!out];
     }
-    entersubsh(Z_ASYNC, 1, 0, 0);
+    entersubsh(ESUB_ASYNC|ESUB_PGRP);
     redup(pipes[out], out);
     closem(FDT_UNUSED);	/* this closes pipes[!out] as well */
     cmdpush(CS_CMDSUBST);
Index: Src/jobs.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/jobs.c,v
retrieving revision 1.58
diff -u -r1.58 jobs.c
--- Src/jobs.c	6 Jul 2007 21:52:39 -0000	1.58
+++ Src/jobs.c	4 Sep 2007 20:28:19 -0000
@@ -821,10 +821,7 @@
     int doneprint = 0;
     FILE *fout = (synch == 2) ? stdout : shout;
 
-    /*
-     * Wow, what a hack.  Did I really write this? --- pws
-     */
-    if (jn < jobtab || jn >= jobtab + jobtabsize)
+    if (oldjobtab != NULL)
 	job = jn - oldjobtab;
     else
 	job = jn - jobtab;
@@ -1286,6 +1283,7 @@
 
     if (monitor && oldmaxjob) {
 	int sz = oldmaxjob * sizeof(struct job);
+	DPUTS(oldjobtab != NULL, "BUG: saving job table twice\n");
 	oldjobtab = (struct job *)zalloc(sz);
 	memcpy(oldjobtab, jobtab, sz);
 
@@ -1473,7 +1471,16 @@
 static int
 getjob(char *s, char *prog)
 {
-    int jobnum, returnval;
+    int jobnum, returnval, mymaxjob;
+    Job myjobtab;
+
+    if (oldjobtab) {
+	myjobtab = oldjobtab;
+	mymaxjob = oldmaxjob;
+    } else {
+	myjobtab= jobtab;
+	mymaxjob = maxjob;
+    }
 
     /* if there is no %, treat as a name */
     if (*s != '%')
@@ -1502,8 +1509,15 @@
     /* a digit here means we have a job number */
     if (idigit(*s)) {
 	jobnum = atoi(s);
-	if (jobnum && jobnum <= maxjob && jobtab[jobnum].stat &&
-	    !(jobtab[jobnum].stat & STAT_SUBJOB) && jobnum != thisjob) {
+	if (jobnum && jobnum <= mymaxjob && myjobtab[jobnum].stat &&
+	    !(myjobtab[jobnum].stat & STAT_SUBJOB) &&
+	    /*
+	     * If running jobs in a subshell, we are allowed to
+	     * refer to the "current" job (it's not really the
+	     * current job in the subshell).  It's possible we
+	     * should reset thisjob to -1 on entering the subshell.
+	     */
+	    (myjobtab == oldjobtab || jobnum != thisjob)) {
 	    returnval = jobnum;
 	    goto done;
 	}
@@ -1515,10 +1529,11 @@
     if (*s == '?') {
 	struct process *pn;
 
-	for (jobnum = maxjob; jobnum >= 0; jobnum--)
-	    if (jobtab[jobnum].stat && !(jobtab[jobnum].stat & STAT_SUBJOB) &&
+	for (jobnum = mymaxjob; jobnum >= 0; jobnum--)
+	    if (myjobtab[jobnum].stat &&
+		!(myjobtab[jobnum].stat & STAT_SUBJOB) &&
 		jobnum != thisjob)
-		for (pn = jobtab[jobnum].procs; pn; pn = pn->next)
+		for (pn = myjobtab[jobnum].procs; pn; pn = pn->next)
 		    if (strstr(pn->text, s + 1)) {
 			returnval = jobnum;
 			goto done;
@@ -1772,7 +1787,7 @@
     In the default case for bg, fg and disown, the argument will be provided by
     the above routine.  We now loop over the arguments. */
     for (; (firstjob != -1) || *argv; (void)(*argv && argv++)) {
-	int stopped, ocj = thisjob;
+	int stopped, ocj = thisjob, jstat;
 
         func = ofunc;
 
@@ -1798,6 +1813,11 @@
 	    thisjob = ocj;
 	    continue;
 	}
+	if (func != BIN_JOBS && oldjobtab != NULL) {
+	    zwarnnam(name, "can't manipulate jobs in subshell");
+	    unqueue_signals();
+	    return 1;
+	}
 	/* The only type of argument allowed now is a job spec.  Check it. */
 	job = (*argv) ? getjob(*argv, name) : firstjob;
 	firstjob = -1;
@@ -1805,9 +1825,10 @@
 	    retval = 1;
 	    break;
 	}
-	if (!(jobtab[job].stat & STAT_INUSE) ||
-	    (jobtab[job].stat & STAT_NOPRINT)) {
-	    zwarnnam(name, "%%%d: no such job", job);
+	jstat = oldjobtab ? oldjobtab[job].stat : jobtab[job].stat;
+	if (!(jstat & STAT_INUSE) ||
+	    (jstat & STAT_NOPRINT)) {
+	    zwarnnam(name, "%s: no such job", *argv);
 	    unqueue_signals();
 	    return 1;
 	}
@@ -1815,7 +1836,7 @@
          * on disown), we actually do a bg and then delete the job table entry. */
 
         if (isset(AUTOCONTINUE) && func == BIN_DISOWN &&
-            jobtab[job].stat & STAT_STOPPED)
+            jstat & STAT_STOPPED)
             func = BIN_BG;
 
 	/* We have a job number.  Now decide what to do with it. */
@@ -1905,7 +1926,7 @@
 	        deletejob(jobtab + job);
 	    break;
 	case BIN_JOBS:
-	    printjob(job + jobtab, lng, 2);
+	    printjob(job + (oldjobtab ? oldjobtab : jobtab), lng, 2);
 	    break;
 	case BIN_DISOWN:
 	    if (jobtab[job].stat & STAT_STOPPED) {


-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/