Re: PATCH: job control debug

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

Typo fixed, now with setpgrp debugging.

Starting to think the key difference may be set_gleader(11) --- transfer
of job ownership on exit, which happens too early for that to be
possible in the bad case.

pws


Good:

% echo | { sleep 0.01 | more }
attachtty(7233728): pgrp = 15378, mypgrp = 15378
set_gleader(10): 1 = 0
set_gleader(3): 1 = 15379
setpgrp(4): pid 0, pgid 15379, current pid 15379, ret 0
attachtty(3): pgrp = 15379, mypgrp = 15378
set_gleader(10): 1 = 0
attachtty(7233728): pgrp = 15378, mypgrp = 15378
set_gleader(10): 1 = 0
set_gleader(9): 1 = 15379
set_gleader(10): 2 = 0
setpgrp(1): pid 0, pgid 15379, current pid 15380, ret 0
set_gleader(10): 1 = 0
set_gleader(10): 2 = 0
set_gleader(9): 2 = 15380
setpgrp(1): pid 0, pgid 15379, current pid 15381, ret 0
set_gleader(10): 1 = 0
set_gleader(10): 2 = 0
set_gleader(9): 2 = 15380
set_gleader(11): 2 = 0
attachtty(14): pgrp = 15378, mypgrp = 15378
set_gleader(8): 2 = 0
attachtty(8): pgrp = 15378, mypgrp = 15378
set_gleader(8): 1 = 0
attachtty(4): pgrp = 15378, mypgrp = 15378


Bad ("sleep 0" is the same, should perhaps use that for consistency):

camnpupstephen% echo | { : | more }
attachtty(7233728): pgrp = 15378, mypgrp = 15378
set_gleader(10): 1 = 0
set_gleader(3): 1 = 15384
setpgrp(4): pid 0, pgid 15384, current pid 15384, ret 0
attachtty(3): pgrp = 15384, mypgrp = 15378
set_gleader(10): 1 = 0
attachtty(7233728): pgrp = 15378, mypgrp = 15378
set_gleader(10): 1 = 0
set_gleader(9): 1 = 15384
set_gleader(10): 2 = 0
setpgrp(1): pid 0, pgid 15384, current pid 15385, ret 0
set_gleader(10): 1 = 0
set_gleader(10): 2 = 0
set_gleader(9): 2 = 15385
setpgrp(1): pid 0, pgid 15384, current pid 15386, ret 0
set_gleader(10): 1 = 0
zsh: done                    echo |
zsh: suspended (tty output)  { : | more; }
set_gleader(10): 2 = 0
set_gleader(9): 2 = 15385
set_gleader(11): 2 = 0
attachtty(14): pgrp = 15378, mypgrp = 15378
set_gleader(6): 2 = 15384
attachtty(4): pgrp = 15378, mypgrp = 15378


diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 2f83f7c..f8a5a95 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -356,7 +356,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 	if (get_pty(0, &slave))
 	    exit(1);
 	SHTTY = slave;
-	attachtty(mypid);
+	ATTACHTTY(mypid, 17);
 #ifdef TIOCGWINSZ
 	/* Set the window size before associating with the terminal *
 	 * so that we don't get hit with a SIGWINCH.  I'm paranoid. */
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 3487b5d..770c0c4 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -233,7 +233,7 @@ zsetterm(void)
     shttyinfo.lmodes &= ~LFLUSHO;
 #endif
 
-    attachtty(mypgrp);
+    ATTACHTTY(mypgrp, 18);
     ti = shttyinfo;
 #ifdef HAS_TIO
     if (unset(FLOWCONTROL))
@@ -917,7 +917,7 @@ getbyte(long do_keytmout, int *timeout, int full)
 	    } else if (errno == EIO && !die) {
 		ret = opts[MONITOR];
 		opts[MONITOR] = 1;
-		attachtty(mypgrp);
+		ATTACHTTY(mypgrp, 19);
 		zrefresh();	/* kludge! */
 		opts[MONITOR] = ret;
 		die = 1;
diff --git a/Src/builtin.c b/Src/builtin.c
index 93fa911..02bcd85 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6129,7 +6129,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 	    if (unset(INTERACTIVE))
 		gettyinfo(&shttyinfo);
 	    /* attach to the tty */
-	    attachtty(mypgrp);
+	    ATTACHTTY(mypgrp, 1);
 	    if (!isem)
 		setcbreak();
 	    readfd = SHTTY;
diff --git a/Src/exec.c b/Src/exec.c
index 09ee132..e00fae0 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1022,17 +1022,18 @@ entersubsh(int flags)
 	}
     } 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 ||
+	    if (SETPGRP(0L, jobtab[list_pipe_job].gleader, 1) == -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);
+		SET_GLEADER(list_pipe_job,
+			    (list_pipe_child ? mypgrp : getpid()), 1);
+		SET_GLEADER(thisjob, jobtab[list_pipe_job].gleader, 2);
+		SETPGRP(0L, jobtab[list_pipe_job].gleader, 2);
 		if (!(flags & ESUB_ASYNC))
-		    attachtty(jobtab[thisjob].gleader);
+		    ATTACHTTY(jobtab[thisjob].gleader, 2);
 	    }
 	}
 	else if (!jobtab[thisjob].gleader ||
-		 setpgrp(0L, jobtab[thisjob].gleader) == -1) {
+		 SETPGRP(0L, jobtab[thisjob].gleader, 3) == -1) {
 	    /*
 	     * This is the standard point at which a newly started
 	     * process gets put into the foreground by taking over
@@ -1043,13 +1044,13 @@ entersubsh(int flags)
 	     * the operation is allowed to work (in addition to not
 	     * causing the shell to be suspended).
 	     */
-	    jobtab[thisjob].gleader = getpid();
+	    SET_GLEADER(thisjob, getpid(), 3);
 	    if (list_pipe_job != thisjob &&
 		!jobtab[list_pipe_job].gleader)
-		jobtab[list_pipe_job].gleader = jobtab[thisjob].gleader;
-	    setpgrp(0L, jobtab[thisjob].gleader);
+		SET_GLEADER(list_pipe_job, jobtab[thisjob].gleader, 4);
+	    SETPGRP(0L, jobtab[thisjob].gleader, 4);
 	    if (!(flags & ESUB_ASYNC))
-		attachtty(jobtab[thisjob].gleader);
+		ATTACHTTY(jobtab[thisjob].gleader, 3);
 	}
     }
     if (!(flags & ESUB_FAKE))
@@ -1687,7 +1688,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 		    /* If the super-job contains only the sub-shell, the
 		       sub-shell is the group leader. */
 		    if (!jn->procs->next || lpforked == 2) {
-			jn->gleader = list_pipe_pid;
+			SET_GLEADER(jn->gleader, list_pipe_pid, 5);
 			jn->stat |= STAT_SUBLEADER;
 			/*
 			 * Pick up any subjob that's still lying around
@@ -1810,7 +1811,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			    jn->stat |= STAT_SUBJOB | STAT_NOPRINT;
 			    jn->other = list_pipe_pid;	/* see zsh.h */
 			    if (hasprocs(list_pipe_job))
-				jn->gleader = jobtab[list_pipe_job].gleader;
+				SET_GLEADER(jn-jobtab, jobtab[list_pipe_job].gleader, 6);
 			}
 			if ((list_pipe || last1) && hasprocs(list_pipe_job))
 			    killpg(jobtab[list_pipe_job].gleader, SIGSTOP);
@@ -1833,7 +1834,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			 * do anything other than the following, but no
 			 * doubt we'll find out...
 			 */
-			setpgrp(0L, mypgrp = getpid());
+			SETPGRP(0L, mypgrp = getpid(), 5);
 			close(synch[1]);
 			kill(getpid(), SIGSTOP);
 			list_pipe = 0;
diff --git a/Src/hist.c b/Src/hist.c
index dbdc1e4..4e50b75 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1116,7 +1116,7 @@ hbegin(int dohist)
 	hist_ring->ftim = time(NULL);
     if ((dohist == 2 || (interact && isset(SHINSTDIN))) && !strin) {
 	histactive = HA_ACTIVE;
-	attachtty(mypgrp);
+	ATTACHTTY(mypgrp, 4);
 	linkcurline();
 	defev = addhistnum(curhist, -1, HIST_FOREIGN);
     } else
diff --git a/Src/jobs.c b/Src/jobs.c
index 38b3d89..1edb065 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -260,7 +260,7 @@ handle_sub(int job, int fg)
 		       killpg(jn->gleader, 0) == -1))) {
 		Process p;
 		for (p = jn->procs; p->next; p = p->next);
-		jn->gleader = p->pid;
+		SET_GLEADER(jn-jobtab, p->pid, 7);
 	    }
 	    /* This deleted the job too early if the parent
 	       shell waited for a command in a list that will
@@ -275,7 +275,7 @@ handle_sub(int job, int fg)
 	       now. */
 	    if ((fg || thisjob == job) &&
 		(!jn->procs->next || cp || jn->procs->pid != jn->gleader))
-		attachtty(jn->gleader);
+		ATTACHTTY(jn->gleader, 5);
 	    kill(sj->other, SIGCONT);
 	    if (jn->stat & STAT_DISOWN)
 	    {
@@ -495,7 +495,7 @@ update_job(Job jn)
 	    (jn->gleader == pgrp || (pgrp > 1 && kill(-pgrp, 0) == -1))) {
 	    if (list_pipe) {
 		if (somestopped || (pgrp > 1 && kill(-pgrp, 0) == -1)) {
-		    attachtty(mypgrp);
+		    ATTACHTTY(mypgrp, 6);
 		    /* check window size and adjust if necessary */
 		    adjustwinsize(0);
 		} else {
@@ -522,7 +522,7 @@ update_job(Job jn)
 		    inerrflush();
 		}
 	    } else {
-		attachtty(mypgrp);
+		ATTACHTTY(mypgrp, 7);
 		/* check window size and adjust if necessary */
 		adjustwinsize(0);
 	    }
@@ -1326,7 +1326,8 @@ freejob(Job jn, int deleting)
 	    freejob(jobtab + jn->other, 0);
 	jn = jobtab + job;
     }
-    jn->gleader = jn->other = 0;
+    SET_GLEADER(jn-jobtab, 0, 8);
+    jn->other = 0;
     jn->stat = jn->stty_in_env = 0;
     jn->filelist = NULL;
     jn->ty = NULL;
@@ -1353,7 +1354,7 @@ deletejob(Job jn, int disowning)
 {
     deletefilelist(jn->filelist, disowning);
     if (jn->stat & STAT_ATTACH) {
-	attachtty(mypgrp);
+	ATTACHTTY(mypgrp, 8);
 	adjustwinsize(0);
     }
     if (jn->stat & STAT_SUPERJOB) {
@@ -1395,7 +1396,7 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime)
 	/* if this is the first process we are adding to *
 	 * the job, then it's the group leader.          */
 	if (!jobtab[thisjob].gleader)
-	    jobtab[thisjob].gleader = pid;
+	    SET_GLEADER(thisjob, pid, 9);
 	/* attach this process to end of process list of current job */
 	pnlist = &jobtab[thisjob].procs;
     }
@@ -1643,7 +1644,7 @@ static int initnewjob(int i)
 	zsfree(jobtab[i].pwd);
 	jobtab[i].pwd = NULL;
     }
-    jobtab[i].gleader = 0;
+    SET_GLEADER(i, 0, 10);
 
     if (i > maxjob)
 	maxjob = i;
@@ -2384,9 +2385,9 @@ bin_fg(char *name, char **argv, Options ops, int func)
 			  (jobtab[job].stat & STAT_SUBLEADER) ||
 			  killpg(jobtab[job].gleader, 0) == -1)) &&
 			jobtab[jobtab[job].other].gleader)
-			attachtty(jobtab[jobtab[job].other].gleader);
+			ATTACHTTY(jobtab[jobtab[job].other].gleader, 9);
 		    else
-			attachtty(jobtab[job].gleader);
+			ATTACHTTY(jobtab[job].gleader, 10);
 		}
 	    }
 	    if (stopped) {
@@ -2848,11 +2849,15 @@ acquire_pgrp(void)
 	oldset = signal_block(blockset);
 	while ((ttpgrp = gettygrp()) != -1 && ttpgrp != mypgrp) {
 	    mypgrp = GETPGRP();
+#ifdef DEBUG_JOB_CONTROL
+	    fprintf(stderr, "mypgrp(1) %ld -> %d, pid %d, mypid %ld\n",
+		    lastpgrp, mypgrp, getpid(), mypid);
+#endif
 	    if (mypgrp == mypid) {
 		if (!interact)
 		    break; /* attachtty() will be a no-op, give up */
 		signal_setmask(oldset);
-		attachtty(mypgrp); /* Might generate SIGT* */
+		ATTACHTTY(mypgrp, 11); /* Might generate SIGT* */
 		signal_block(blockset);
 	    }
 	    if (mypgrp == gettygrp())
@@ -2861,14 +2866,22 @@ acquire_pgrp(void)
 	    if (read(0, NULL, 0) != 0) {} /* Might generate SIGT* */
 	    signal_block(blockset);
 	    mypgrp = GETPGRP();
+#ifdef DEBUG_JOB_CONTROL
+	    fprintf(stderr, "mypgrp(2) %ld -> %d, pid %d, mypid %ld\n",
+		    lastpgrp, mypgrp, getpid(), mypid);
+#endif
 	    if (mypgrp == lastpgrp && !interact)
 		break; /* Unlikely that pgrp will ever change */
 	    lastpgrp = mypgrp;
 	}
 	if (mypgrp != mypid) {
-	    if (setpgrp(0, 0) == 0) {
+	    if (SETPGRP(0, 0, 6) == 0) {
 		mypgrp = mypid;
-		attachtty(mypgrp);
+#ifdef DEBUG_JOB_CONTROL
+		fprintf(stderr, "mypgrp(3) %ld -> %d, pid %d, mypid %ld\n",
+			lastpgrp, mypgrp, getpid(), mypid);
+#endif
+		ATTACHTTY(mypgrp, 12);
 	    } else
 		opts[MONITOR] = 0;
 	}
@@ -2886,9 +2899,13 @@ release_pgrp(void)
     if (origpgrp != mypgrp) {
 	/* in linux pid namespaces, origpgrp may never have been set */
 	if (origpgrp) {
-	    attachtty(origpgrp);
-	    setpgrp(0, origpgrp);
+	    ATTACHTTY(origpgrp, 13);
+	    SETPGRP(0, origpgrp, 7);
 	}
+#ifdef DEBUG_JOB_CONTROL
+	fprintf(stderr, "mypgrp(4) %d -> %d, pid %d, mypid %ld\n",
+		mypgrp, origpgrp, getpid(), mypid);
+#endif
 	mypgrp = origpgrp;
     }
 }
diff --git a/Src/params.c b/Src/params.c
index a1c299f..66687fb 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2586,7 +2586,7 @@ assignstrvalue(Value v, char *val, int flags)
 		Param pm = v->pm;
                 /* Size doesn't change, can limit actions to only
                  * overwriting bytes in already allocated string */
-                strncpy(z + v->start, val, vlen);
+                memcpy(z + v->start, val, vlen);
 		/* Implement remainder of strsetfn */
 		if (!(pm->node.flags & PM_HASHELEM) &&
 		    ((pm->node.flags & PM_NAMEDDIR) ||
diff --git a/Src/signals.c b/Src/signals.c
index 20c6fdf..868764d 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -540,7 +540,7 @@ wait_for_processes(void)
 		if (WIFEXITED(status) &&
 		    pn->pid == jn->gleader &&
 		    killpg(pn->pid, 0) == -1) {
-		    jn->gleader = 0;
+		    SET_GLEADER(jn-jobtab, 0, 11);
 		    if (!(jn->stat & STAT_NOSTTY)) {
 			/*
 			 * This PID was in control of the terminal;
@@ -549,7 +549,7 @@ wait_for_processes(void)
 			 * process of this job will become group
 			 * leader, however.
 			 */
-			attachtty(mypgrp);
+			ATTACHTTY(mypgrp, 14);
 		    }
 		}
 	    }
diff --git a/Src/utils.c b/Src/utils.c
index 075d272..42f6ab7 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2952,7 +2952,7 @@ getquery(char *valid_chars, int purge)
     int isem = !strcmp(term, "emacs");
     struct ttyinfo ti;
 
-    attachtty(mypgrp);
+    ATTACHTTY(mypgrp, 15);
 
     gettyinfo(&ti);
 #ifdef HAS_TIO
@@ -4639,13 +4639,21 @@ setcbreak(void)
 
 /* give the tty to some process */
 
-/**/
+/* note: deliberately not automatically prototyped */
 mod_export void
-attachtty(pid_t pgrp)
+attachtty(pid_t pgrp
+#ifdef DEBUG_JOB_CONTROL
+	  , int index
+#endif
+    )
 {
     static int ep = 0;
 
     if (jobbing && interact) {
+#ifdef DEBUG_JOB_CONTROL
+	fprintf(stderr, "attachtty(%d): pgrp = %d, mypgrp = %d\n",
+		index, pgrp, mypgrp);
+#endif
 #ifdef HAVE_TCSETPGRP
 	if (SHTTY != -1 && tcsetpgrp(SHTTY, pgrp) == -1 && !ep)
 #else
@@ -4658,8 +4666,11 @@ attachtty(pid_t pgrp)
 # endif
 #endif
 	{
+#ifdef DEBUG_JOB_CONTROL
+	    fprintf(stderr, "attachtty failed\n");
+#endif
 	    if (pgrp != mypgrp && kill(-pgrp, 0) == -1)
-		attachtty(mypgrp);
+		ATTACHTTY(mypgrp, 16);
 	    else {
 		if (errno != ENOTTY)
 		{
@@ -4673,6 +4684,26 @@ attachtty(pid_t pgrp)
     }
 }
 
+/**/
+#ifdef DEBUG_JOB_CONTROL
+
+void set_gleader(int job, int pid, int index)
+{
+    jobtab[job].gleader = pid;
+    fprintf(stderr, "set_gleader(%d): %d = %d\n", index, job, pid);
+}
+
+int setpgrp_debug(int pid, int pgid, int index)
+{
+    int ret = setpgrp(pid, pgid);
+    fprintf(stderr, "setpgrp(%d): pid %d, pgid %d, current pid %d, ret %d\n",
+	    index, pid, pgid, getpid(), ret);
+    return ret;
+}
+
+/**/
+#endif /* DEBUG_JOB_CONTROL */
+
 /* get the process group associated with the tty */
 
 /**/
diff --git a/Src/zsh.h b/Src/zsh.h
index 8e7f20b..bcad47b 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -3292,7 +3292,7 @@ typedef int convchar_t;
 #define MB_METASTRLEN2(str, widthp)	ztrlen(str)
 #define MB_METASTRLEN2END(str, widthp, eptr)	ztrlenend(str, eptr)
 
-#define MB_CHARINIT()
+#define MBT_CHARINIT()
 #define MB_CHARLENCONV(str, len, cp) charlenconv((str), (len), (cp))
 #define MB_CHARLEN(str, len) ((len) ? 1 : 0)
 
@@ -3303,3 +3303,26 @@ typedef int convchar_t;
 #define ZWS(s)	s
 
 #endif /* MULTIBYTE_SUPPORT */
+
+
+/* Uncomment to debug problems with job control */
+/*#define DEBUG_JOB_CONTROL*/
+
+#ifdef DEBUG_JOB_CONTROL
+#define ATTACHTTY(pgrp, index) attachtty(pgrp, index)
+void attachtty(pid_t pgrp, int index);
+
+#define SET_GLEADER(job, pid, index) set_gleader(job, pid, index)
+void set_gleader(int job, int pid, int index);
+
+#define SETPGRP(pid, pgid, index) setpgrp_debug(pid, pgid, index)
+int setpgrp_debug(int pid, int pgid, int index);
+#else
+
+#define ATTACHTTY(pgrp, index) attachtty(pgrp)
+
+#define SET_GLEADER(job, pid, index) (jobtab[(job)].gleader = (pid))
+void attachtty(pid_t pgrp);
+
+#define SETPGRP(pid, pgid, index) setpgrp(pid, pgid)
+#endif