Re: Cant fg a suspended su (4.1.0-dev-7)

[Philippe Troin <phil@xxxxxxxx>]

Peter Whaite <peta@xxxxxxxxxxxxxxxxx> writes:

> Philippe Troin said:
> > Philippe Troin <phil@xxxxxxxx> writes:
> > 
> > > I'll check out how the other shells (bash and tcsh) handle this case
> > > and will post a patch for bin_suspend() later.
> > 
> > I was looking at bash, and they do not do anything special (like
> > reverting to the original pgrp) before suspending.
> > 
> > Can you try su'ing with bash? I was able to get it stuck with my local
> > test program here... but I do not have GNU su installed (yet).
> 
> Looks like bash can deal with GNU su.  So can tcsh. 
> 
> % su --shell=/bin/bash peta
> Password: 
> [peta@aragorn zsh]$ suspend
> zsh: suspended (signal)  /bin/su --shell=/bin/bash peta
> 1 % fg
> [1]  + continued  /bin/su --shell=/bin/bash peta
> 
> 11779 11790 11790 11790 pts/2    12959 S      501   0:00      \_ -zsh
> 11790 12894 12894 11790 pts/2    12959 S        0   0:00          \_ /bin/su --shell=/bin/bash peta
> 12894 12903 12903 11790 pts/2    12959 S      501   0:00              \_ bash
> 12903 12959 12959 11790 pts/2    12959 R      501   0:00                  \_ ps xajfw

Mrm, that's not GNU su. Or it's a very heavily patched version of GNU
su. I've check shellutils 2.0 and it does a straight exec(), and does
not leave a parent process like seen above...

So I cannot reproduce your exact problem, but I think I've fixed it
nevertheless... Can you try the enclosed patch? It's against the
latest CVS, but it applies with a little bit of fuzz to 4.1.0-dev-7.

Here is a transcript of a session which shows that it works. "spawn"
is a very simple program that does what I believe your version of su
does. Replace it with "su --shell=/path/to/zsh".

    phil@ceramic:~/y% ./spawn zsh/Src/zsh
    phil@ceramic:~/y[2]% suspend

    zsh: 13283 suspended  ./spawn zsh/Src/zsh
    phil@ceramic:~/y% bg
    [1]  + continued  ./spawn zsh/Src/zsh
    phil@ceramic:~/y% 
    [1]  + 13283 suspended (tty input)  ./spawn zsh/Src/zsh
    phil@ceramic:~/y% fg
    [1]  + continued  ./spawn zsh/Src/zsh
    phil@ceramic:~/y[2]% exec zsh/Src/zsh                         
    phil@ceramic:~/y[2]% suspend

    zsh: 13283 suspended  ./spawn zsh/Src/zsh
    zsh: exit 20
    phil@ceramic:~/y% bg
    [1]  + continued  ./spawn zsh/Src/zsh
    phil@ceramic:~/y% 
    [1]  + 13283 suspended (tty input)  ./spawn zsh/Src/zsh
    phil@ceramic:~/y% fg
    [1]  + continued  ./spawn zsh/Src/zsh
    phil@ceramic:~/y[2]% exit
    child exited with status 0
    phil@ceramic:~/y% 

You'll notice that:

 - suspend now works.

 - suspend STILL works across exec (nice touch)

 - bg'ing or resuming reports "suspended (tty input)" correctly,
   without the 1 second delay anymore.

Technical explanation:

 - zsh-workers/17859 makes the case why we always want to have
   interactive zsh instances to be the leader of their process group,
   so I won't come back to this.

 - zsh-workers/17859 created a new process group if zsh was started
   interactively without being a process group leader. This happens
   frequently we have have this typical process hierarchy:

      zsh
       \_ program running system("zsh") (eg. mail, dpkg, your su)
          \_ zsh

The fix consists in the following:

 - when the user invokes "suspend", and we created our own process
   group, we must go back to the process group we came from and send a
   SIGTSTP to the whole process group. When the process is resumed, it
   will reacquire its process group leader status.

 - also, when we run "exec somecommand", we must restore the original
   process group.

Patch walk-through:

 - introduced a new global variable, origpgrp, containing the initial
   process group. It is only set in init_io().

 - moved the process group acquiring and terminal acquiring code from
   init_io() to a new function, acquire_pgrp() in jobs.c. init_io()
   now calls acquire_pgrp().

 - created a new function in jobs.c, release_pgrp() that gives the
   terminal foreground process group back to origprgp and moves back
   zsh to this process group.

 - in bin_suspend(): before suspending, call release_pgrp(). Use
   killpg() to broadcast the signal to the whole process group, rather
   than just kill()ing ourselves (note: in most cases it's the same
   since zsh is alone in its process group, however in the above case
   where zsh is spawned by another process, the spawning process must
   also receive SIGTSTP, just as if CTRL-Z had been pressed). After
   zsh is resumed call acquire_pgrp() again which will (maybe) move
   back zsh to its own process group, and handle the terminal in the
   proper way (rather than playing tricks with sleep()).

 - in exec.c: we want to be sure that if zsh has moved to its own
   process group, it moves back to its original process group before
   exec'ing a final command. I had to add an extra parameter to
   entersubsh() indicating whether or not reverting to an eventual
   origpgrp is needed. It is always zero, except when called from
   execsubst().

   Note to Bart: at the stage where entersubsh() is called from
   execsubst(), the condition seems to be:

     (do_exec || (type >= WC_CURSH && last1 == 1)) && !forked

   You said that do_exec && !forked was sufficient, but do_exec is set
   to 1 two lines down if (type >= WC_CURSH && last1 == 1).

   Also, inside entersubsh(), where release_pgrp() is called, I have
   to check that (getpid() == mypgrp) in addition to revertpgrp being
   true. Otherwise some temporary zsh subprocesses (like the ones
   created during a pipe before they exec the actual executables) will
   wrongly revert their process group.

I've also enclosed the trivial "spawn" program, in case you don't have
the RH shellutils. And you might want to try it with other shells. I
believe zsh is the only shell that implements terminal handling
properly now: bash fails to suspend correctly with spawn, and tcsh
gets lost after it execs itself.

Peter, can you confirm that this patch makes zsh perform as expected
with your su?

If yes, then Bart, would you consider it for inclusion?

Phil.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.48
diff -b -u -r1.48 exec.c
--- Src/exec.c	18 Dec 2002 16:57:02 -0000	1.48
+++ Src/exec.c	6 Mar 2003 03:52:57 -0000
@@ -1149,7 +1149,7 @@
 		    }
 		    else {
 			close(synch[0]);
-			entersubsh(Z_ASYNC, 0, 0);
+			entersubsh(Z_ASYNC, 0, 0, 0);
 			if (jobtab[list_pipe_job].procs) {
 			    if (setpgrp(0L, mypgrp = jobtab[list_pipe_job].gleader)
 				== -1) {
@@ -1258,7 +1258,7 @@
 	    } else {
 		zclose(pipes[0]);
 		close(synch[0]);
-		entersubsh(how, 2, 0);
+		entersubsh(how, 2, 0, 0);
 		close(synch[1]);
 		execcmd(state, input, pipes[1], how, 0);
 		_exit(lastval);
@@ -2060,7 +2060,7 @@
 	}
 	/* pid == 0 */
 	close(synch[0]);
-	entersubsh(how, (type != WC_SUBSH) && !(how & Z_ASYNC) ? 2 : 1, 0);
+	entersubsh(how, (type != WC_SUBSH) && !(how & Z_ASYNC) ? 2 : 1, 0, 0);
 	close(synch[1]);
 	forked = 1;
 	if (sigtrapped[SIGINT] & ZSIG_IGNORED)
@@ -2277,7 +2277,9 @@
 	 * exit) in case there is an error return.
 	 */
 	if (is_exec)
-	    entersubsh(how, (type != WC_SUBSH) ? 2 : 1, 1);
+	    entersubsh(how, (type != WC_SUBSH) ? 2 : 1, 1,
+		       (do_exec || (type >= WC_CURSH && last1 == 1)) 
+		       && !forked);
 	if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
@@ -2536,7 +2538,7 @@
 
 /**/
 static void
-entersubsh(int how, int cl, int fake)
+entersubsh(int how, int cl, int fake, int revertpgrp)
 {
     int sig, monitor;
 
@@ -2580,6 +2582,8 @@
     }
     if (!fake)
 	subsh = 1;
+    if (revertpgrp && getpid() == mypgrp)
+	release_pgrp();
     if (SHTTY != -1) {
 	shout = NULL;
 	zclose(SHTTY);
@@ -2769,7 +2773,7 @@
     zclose(pipes[0]);
     redup(pipes[1], 1);
     opts[MONITOR] = 0;
-    entersubsh(Z_SYNC, 1, 0);
+    entersubsh(Z_SYNC, 1, 0, 0);
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1);
     cmdpop();
@@ -2900,7 +2904,7 @@
     /* pid == 0 */
     redup(fd, 1);
     opts[MONITOR] = 0;
-    entersubsh(Z_SYNC, 1, 0);
+    entersubsh(Z_SYNC, 1, 0, 0);
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1);
     cmdpop();
@@ -2980,10 +2984,10 @@
 	zerr("can't open %s: %e", pnam, errno);
 	_exit(1);
     }
-    entersubsh(Z_ASYNC, 1, 0);
+    entersubsh(Z_ASYNC, 1, 0, 0);
     redup(fd, out);
 #else
-    entersubsh(Z_ASYNC, 1, 0);
+    entersubsh(Z_ASYNC, 1, 0, 0);
     redup(pipes[out], out);
     closem(0);   /* this closes pipes[!out] as well */
 #endif
@@ -3012,7 +3016,7 @@
 	zclose(pipes[out]);
 	return pipes[!out];
     }
-    entersubsh(Z_ASYNC, 1, 0);
+    entersubsh(Z_ASYNC, 1, 0, 0);
     redup(pipes[out], out);
     closem(0);	/* this closes pipes[!out] as well */
     cmdpush(CS_CMDSUBST);
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.30
diff -b -u -r1.30 init.c
--- Src/init.c	27 Jan 2003 16:38:09 -0000	1.30
+++ Src/init.c	6 Mar 2003 03:52:57 -0000
@@ -354,7 +354,6 @@
 mod_export void
 init_io(void)
 {
-    long ttpgrp;
     static char outbuf[BUFSIZ], errbuf[BUFSIZ];
 
 #ifdef RSH_BUG_WORKAROUND
@@ -462,37 +461,8 @@
      */
     mypid = (zlong)getpid();
     if (opts[MONITOR] && interact && (SHTTY != -1)) {
-	if ((mypgrp = GETPGRP()) > 0) {
-	    sigset_t blockset, oldset;
-	    sigemptyset(&blockset);
-	    sigaddset(&blockset, SIGTTIN);
-	    sigaddset(&blockset, SIGTTOU);
-	    sigaddset(&blockset, SIGTSTP);
-	    oldset = signal_block(blockset);
-	    while ((ttpgrp = gettygrp()) != -1 && ttpgrp != mypgrp) {
-		mypgrp = GETPGRP();
-		if (mypgrp == mypid) {
-		    signal_setmask(oldset);
-		    attachtty(mypgrp); /* Might generate SIGT* */
-		    signal_block(blockset);
-		}
-		if (mypgrp == gettygrp())
-		    break;
-		signal_setmask(oldset);
-		read(0, NULL, 0); /* Might generate SIGT* */
-		signal_block(blockset);
-		mypgrp = GETPGRP();
-	    }
-	    if (mypgrp != mypid) {
-	        if (setpgrp(0, 0) == 0) {
-		    mypgrp = mypid;
-		    attachtty(mypgrp);
-                } else
-		    opts[MONITOR] = 0;
-	    }
-	    signal_setmask(oldset);
-	} else
-	    opts[MONITOR] = 0;
+	origpgrp = GETPGRP();
+        acquire_pgrp(); /* might also clear opts[MONITOR] */
     } else
 	opts[MONITOR] = 0;
 #else
Index: Src/jobs.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/jobs.c,v
retrieving revision 1.19
diff -b -u -r1.19 jobs.c
--- Src/jobs.c	21 Feb 2003 14:37:03 -0000	1.19
+++ Src/jobs.c	6 Mar 2003 03:52:58 -0000
@@ -30,6 +30,12 @@
 #include "zsh.mdh"
 #include "jobs.pro"
 
+/* the process group of the shell at startup (equal to mypgprp, except
+   when we started without being process group leader */
+
+/**/
+mod_export pid_t origpgrp;
+
 /* the process group of the shell */
 
 /**/
@@ -1663,16 +1669,16 @@
 	signal_default(SIGTTIN);
 	signal_default(SIGTSTP);
 	signal_default(SIGTTOU);
+
+	/* Move ourselves back to the process group we came from */
+	release_pgrp();
     }
+
     /* suspend ourselves with a SIGTSTP */
-    kill(0, SIGTSTP);
+    killpg(origpgrp, SIGTSTP);
+
     if (jobbing) {
-	/* stay suspended */
-	while (gettygrp() != mypgrp) {
-	    sleep(1);
-	    if (gettygrp() != mypgrp)
-		kill(0, SIGTTIN);
-	}
+	acquire_pgrp();
 	/* restore signal handling */
 	signal_ignore(SIGTTOU);
 	signal_ignore(SIGTSTP);
@@ -1695,4 +1701,60 @@
 	    jobtab[jobnum].procs->text && strpfx(s, jobtab[jobnum].procs->text))
 	    return jobnum;
     return -1;
+}
+
+
+/* make sure we are a process group leader by creating a new process
+   group if necessary */
+
+/**/
+void
+acquire_pgrp(void)
+{
+    long ttpgrp;
+    sigset_t blockset, oldset;
+
+    if ((mypgrp = GETPGRP()) > 0) {
+	sigemptyset(&blockset);
+	sigaddset(&blockset, SIGTTIN);
+	sigaddset(&blockset, SIGTTOU);
+	sigaddset(&blockset, SIGTSTP);
+	oldset = signal_block(blockset);
+	while ((ttpgrp = gettygrp()) != -1 && ttpgrp != mypgrp) {
+	    mypgrp = GETPGRP();
+	    if (mypgrp == mypid) {
+		signal_setmask(oldset);
+		attachtty(mypgrp); /* Might generate SIGT* */
+		signal_block(blockset);
+	    }
+	    if (mypgrp == gettygrp())
+		break;
+	    signal_setmask(oldset);
+	    read(0, NULL, 0); /* Might generate SIGT* */
+	    signal_block(blockset);
+	    mypgrp = GETPGRP();
+	}
+	if (mypgrp != mypid) {
+	    if (setpgrp(0, 0) == 0) {
+		mypgrp = mypid;
+		attachtty(mypgrp);
+	    } else
+		opts[MONITOR] = 0;
+	}
+	signal_setmask(oldset);
+    } else
+	opts[MONITOR] = 0;
+}
+
+/* revert back to the process group we came from (before acquire_pgrp) */
+
+/**/
+void
+release_pgrp(void)
+{
+    if (origpgrp != mypgrp) {
+	attachtty(origpgrp);
+	setpgrp(0, origpgrp);
+	mypgrp = origpgrp;
+    }
 }

   #include <unistd.h>
   #include <stdio.h>
   #include <sys/wait.h>
   #include <sys/types.h>

   int main(int argc, char *argv[])
   {
     int status;
     pid_t child = fork();
     if (child<0)
       perror("fork");
     if (child == 0)
       {
         if (execvp(argv[1], &argv[1])<0)
           perror("execvp");
         exit(1);
       }
     waitpid(child, &status, 0);
     printf("child exited with status %x\n", status);
     exit(0);
   }