PATCH (?): Re: suspend while loop.

["Bart Schaefer" <schaefer@xxxxxxxxxxxxxxxxxxxxxxx>]

On May 18, 10:37am, Sven Wischnowsky wrote:
} Subject: Re: suspend while loop.
}
} Everyone can easily test this by adding child_unblock() before
} the read() in zread() and child_unblock()s after it. This makes the
                                s/un// ?
} loop suspendible, but when it is continued, the loop exits immediately
} because the read that was suspended returned non-zero.

That would be OK, but the overhead of blocking and unblocking signals
around every one-character read() makes me cringe.  

Furthermore, SIGCHLD will cause the read() to return -1 with EINTR,
which we can't have happening in normal operation.

} Depending on personal taste it is either ugly that the cat gets
} suspended or that we can't ^C out of the loop after the ^Z.

It's particularly ugly that you can't ^C out of the loop after the ^Z.
The shell should not hang uninterrupibly unless the user has really
wrapped a figurative rope around its figurative neck.

Does anyone see any problems with the following compromise patch?  This
makes `read' (the builtin, not read(2)) interruptible with ^C even when
it is at the right-hand-end of a pipeline.  For comparison, before and
after the patch try:

zsh% cat | read line ; echo $?
^C

In the before case, echo runs and prints 1.  In the after case, echo does
not run.  This is consistent with

zsh% read line ; echo $?
^C

which, in both before and after cases, does not run echo.

The thing that most worries me is that zread() can invoke zle, which can
invoke shell functions, which ....

Another question is, is there a more generic way to do this that covers all
builtin commands and not just `read'?  Or is `read' really the only one
that needs it?

Index: Src/builtin.c
===================================================================
@@ -3536,6 +3536,7 @@
     first = 1;
     bslash = 0;
     while (*args || (ops['A'] && !gotnl)) {
+	sigset_t s = child_unblock();
 	buf = bptr = (char *)zalloc(bsiz = 64);
 	/* get input, a character at a time */
 	while (!gotnl) {
@@ -3573,6 +3574,7 @@
 		bptr = buf + blen;
 	    }
 	}
+	signal_setmask(s);
 	if (c == '\n' || c == EOF)
 	    gotnl = 1;
 	*bptr = '\0';
@@ -3627,7 +3629,8 @@
     buf = bptr = (char *)zalloc(bsiz = 64);
     /* any remaining part of the line goes into one parameter */
     bslash = 0;
-    if (!gotnl)
+    if (!gotnl) {
+	sigset_t s = child_unblock();
 	for (;;) {
 	    c = zread(izle);
 	    /* \ at the end of a line introduces a continuation line, except in
@@ -3662,6 +3665,8 @@
 		bptr = buf + blen;
 	    }
 	}
+	signal_setmask(s);
+    }
     while (bptr > buf && iwsep(bptr[-1]))
 	bptr--;
     *bptr = '\0';
@@ -3718,8 +3723,8 @@
 	case 1:
 	    /* return the character read */
 	    return STOUC(cc);
-#if defined(EAGAIN) || defined(EWOULDBLOCK)
 	case -1:
+#if defined(EAGAIN) || defined(EWOULDBLOCK)
 	    if (!retry && readfd == 0 && (
 # ifdef EAGAIN
 		    errno == EAGAIN
@@ -3733,9 +3738,11 @@
 		) && setblock_stdin()) {
 		retry = 1;
 		continue;
-	    }
-	    break;
+	    } else
 #endif /* EAGAIN || EWOULDBLOCK */
+	    if (errno == EINTR && !(errflag || retflag || breaks || contflag))
+		continue;
+	    break;
 	}
 	return EOF;
     }

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com