Re: PATCH: Misc. zpty tweaks, plus commentary

Re: PATCH: Misc. zpty tweaks, plus commentary

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> The problem is that whenever zsh forks a builtin, it closes all descriptors
> numbered higher than 10, which includes the master pty descriptor.  So if
> you try to use `zpty -r' or `zpty -w' in a pipeline, or try to put them in
> the background, they'll get "invalid file descriptor" when attempting to
> access the pty.  I've played with "hiding" the master fd by zeroing its slot
> in fdtable[], and that does make zpty work correctly, but it also means that
> the master descriptor is still open when running other builtins, which is
> not quite so desirable.

I don't see any simple solution either. Maybe this is yet another
reason for a more sophisticated file-descriptior/blocking/waiting
handling in the core. Not only that modules should be able to register 
file-descriptors that should be monitored whenever the shell is
waiting, they should also be able to register handler functions (for
reading, writing, exceptions, closing, etc.).


[ in another message: ]

> On Nov 4, 11:29pm, I wrote:
> }
> } `zpty -w' already was able to stream into the pty (even though that's
> } not documented); e.g. `zpty -w foo < file' writes the entire contents
> } of the file to the pty [...] Now `zpty -r foo' can stream the output
> } as well.  Unfortunately, it's still not possible to do both at once.
> }
> } The problem is that whenever zsh forks a builtin, it closes all
> } descriptors numbered higher than 10, which includes the master pty
> } descriptor.
> 
> Although a C-code-level fix would be preferable, a workaround has just
> occurred to me:  Using a subshell will prevent the descriptors from
> being closed.  So with 13116 applied, you should be able to do:
> 
>     zpty -b foo cat
>     yes blah | (zpty -w foo) &
>     (zpty -r foo) | less
>     zpty -d foo
> 
> This reveals that still another remaining problem is that `zpty -w' on a
> blocking pty doesn't stop when the process on the pty is killed.  There
> doesn't seem to be any simple fix for this; write() itself is blocked,
> and does not get interrupted with SIGPIPE as would normally occur.

Hm, yes. There might be a solution using the fact that we have a
subshell that we could make ignore SIGHUP (and write something to the
pty or whatever). But I currently can't see how this really helps.


[ and in yet another message: ]

> ...
> 
> I haven't done anything about it, but this code clearly expects that
> no '\0' bytes will ever be sent to or received from the pty.  That's
> obviously a fallacy; we shouldn't be treating this data as C strings.

Yes and no. For reading, this already worked (the call to metafy()). But
somehow I forgot to do the same for `zpty -w'. The patch fixes this.

> It would appear from the documentation that, with a blocking pty,
> `zpty -r' must always return either 0 or 2.  Is this really the case?
> (On my RH5.2 linux system, select() always returns 1 after the pty's
> command has exited, so read_poll() also returns 1 and cmd->fin never
> gets set.  read(), however, gets an I/O error (errno == 5), so `zpty -r'
> returns 1 and it's not possible to detect that the command has finished.

Rats. The same here. Why didn't I notice that?

Another reason for not using read_poll(), does anyone see a way to
allow us to find out if a command has exited and still be able to read 
the rest of its output?


Bye
 Sven

Index: Src/Modules/zpty.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/zpty.c,v
retrieving revision 1.18
diff -u -r1.18 zpty.c
--- Src/Modules/zpty.c	2000/11/11 19:50:29	1.18
+++ Src/Modules/zpty.c	2000/11/13 10:05:24
@@ -562,13 +562,15 @@
 ptywrite(Ptycmd cmd, char **args, int nonl)
 {
     if (*args) {
-	char sp = ' ';
+	char sp = ' ', *tmp;
+	int len;
 
-	while (*args)
-	    if (ptywritestr(cmd, *args, strlen(*args)) ||
+	while (*args) {
+	    unmetafy((tmp = dupstring(*args)), &len);
+	    if (ptywritestr(cmd, tmp, len) ||
 		(*++args && ptywritestr(cmd, &sp, 1)))
 		return 1;
-
+	}
 	if (!nonl) {
 	    sp = '\n';
 	    if (ptywritestr(cmd, &sp, 1))

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Misc. zpty tweaks, plus commentary

[Bart Schaefer]

On Nov 13, 11:21am, Sven Wischnowsky wrote:
} Subject: Re: PATCH: Misc. zpty tweaks, plus commentary
}
} [ in another message: ]
} 
} > This reveals that still another remaining problem is that `zpty -w' on a
} > blocking pty doesn't stop when the process on the pty is killed.  There
} > doesn't seem to be any simple fix for this; write() itself is blocked,
} > and does not get interrupted with SIGPIPE as would normally occur.
} 
} Hm, yes. There might be a solution using the fact that we have a
} subshell that we could make ignore SIGHUP (and write something to the
} pty or whatever). But I currently can't see how this really helps.

If we kept track of the fact that the process on the pty is one of our
children, we could handle all of this when the SIGCHLD arrives.  Modules
need to be able to register handlers for that as much (if not more) than
they need to be able to register I/O handlers.

A simple table mapping PIDs to callbacks would suffice.

} [ and in yet another message: ]
} 
} > I haven't done anything about it, but this code clearly expects that
} > no '\0' bytes will ever be sent to or received from the pty.  That's
} > obviously a fallacy; we shouldn't be treating this data as C strings.
} 
} Yes and no. For reading, this already worked (the call to metafy()).

Lines 467 and 519 sling cmd->old around with strcpy().  The metafy() on
525 only works if there's never been read-ahead.

} > (On my RH5.2 linux system, select() always returns 1 after the pty's
} > command has exited, so read_poll() also returns 1 and cmd->fin never
} > gets set.  read(), however, gets an I/O error (errno == 5), so `zpty -r'
} > returns 1 and it's not possible to detect that the command has finished.
} 
} Another reason for not using read_poll(), does anyone see a way to
} allow us to find out if a command has exited and still be able to read 
} the rest of its output?

The SIGCHLD handler could deal with this, too.  (The finding-out part, not
the reading-the-rest part.)

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: Misc. zpty tweaks, plus commentary

[Bart Schaefer]

The first hunk below simply adds strerror output to a couple of warning
messages, so one has a better idea of why things went wrong.

The second hunk passes (!cmd->block) as the final argument of read_poll().
Without this, `zpty -b foo bar; zpty -t foo' would hang until `bar' produced
output (which it might never do).  With it, I suspect that on systems that
don't HAVE_SELECT you can get a nonzero return from zpty -t even when some
output is still available; but I can't test that and I don't know another
fix for the blocking problem.

The third through sixth hunks change `zpty -r' to stream to stdout when
there is no parameter name into which the output is to be captured, rather
than buffering all the output in memory.

The fifth and seventh hunks remove some code that's been #if 0 for a while.

The motivation for all this is to be able to use a program running under
zpty as a filter, much like a coproc.  `zpty -w' already was able to stream
into the pty (even though that's not documented); e.g. `zpty -w foo < file'
writes the entire contents of the file to the pty (or tries to -- it might
fail if `zpty -b' was not used to create the pty).  Now `zpty -r foo' can
stream the output as well.  Unfortunately, it's still not possible to do
both at once.

The problem is that whenever zsh forks a builtin, it closes all descriptors
numbered higher than 10, which includes the master pty descriptor.  So if
you try to use `zpty -r' or `zpty -w' in a pipeline, or try to put them in
the background, they'll get "invalid file descriptor" when attempting to
access the pty.  I've played with "hiding" the master fd by zeroing its slot
in fdtable[], and that does make zpty work correctly, but it also means that
the master descriptor is still open when running other builtins, which is
not quite so desirable.

Any suggestions for the right approach to fixing this remaining problem?

Index: Src/Modules/zpty.c
===================================================================
@@ -275,12 +275,12 @@
 	return 1;
     }
     if (get_pty(1, &master)) {
-	zwarnnam(nam, "can't open pseudo terminal", NULL, 0);
+	zwarnnam(nam, "can't open pseudo terminal: %e", NULL, errno);
 	return 1;
     }
     if ((pid = fork()) == -1) {
+	zwarnnam(nam, "can't create pty command %s: %e", pname, errno);
 	close(master);
-	zwarnnam(nam, "couldn't create pty command: %s", pname, 0);
 	return 1;
     } else if (!pid) {
 
@@ -438,7 +438,8 @@
 {
     if (cmd->read != -1)
 	return;
-    if (!read_poll(cmd->fd, &cmd->read, 1) && kill(cmd->pid, 0) < 0) {
+    if (!read_poll(cmd->fd, &cmd->read, !cmd->block) &&
+	kill(cmd->pid, 0) < 0) {
 	cmd->fin = 1;
 	zclose(cmd->fd);
     }
@@ -447,7 +448,7 @@
 static int
 ptyread(char *nam, Ptycmd cmd, char **args)
 {
-    int blen = 256, used = 0, ret = 1;
+    int blen = 256, used = 0, seen = 0, ret = 1;
     char *buf = (char *) zhalloc((blen = 256) + 1);
     Patprog prog = NULL;
 
@@ -465,7 +466,9 @@
 	    zwarnnam(nam, "bad pattern: %s", args[1], 0);
 	    return 1;
 	}
-    }
+    } else
+	fflush(stdout);
+
     if (cmd->read != -1) {
 	buf[0] = (char) cmd->read;
 	buf[1] = '\0';
@@ -479,30 +482,19 @@
 		break;
 	}
 	if ((ret = read(cmd->fd, buf + used, 1)) == 1) {
+	    seen = 1;
 	    if (++used == blen) {
-		buf = hrealloc(buf, blen, blen << 1);
-		blen <<= 1;
+		if (!*args) {
+		    write(1, buf, used);
+		    used = 0;
+		} else {
+		    buf = hrealloc(buf, blen, blen << 1);
+		    blen <<= 1;
+		}
 	    }
 	}
 	buf[used] = '\0';
 
-#if 0
-	/* This once used the following test, to make sure to return
-	 * non-zero if there are no characters to read.  That looks
-	 * like a thinko now, because it disables non-blocking ptys. */
-
-	if (ret < 0 && (cmd->block
-#ifdef EWOULDBLOCK
-			|| errno != EWOULDBLOCK
-#else
-#ifdef EAGAIN
-			|| errno != EAGAIN
-#endif
-#endif
-			))
-	    break;
-#endif
-
 	if (!prog && ret <= 0)
 	    break;
     } while (!errflag && !breaks && !retflag && !contflag &&
@@ -511,11 +503,10 @@
 
     if (*args)
 	setsparam(*args, ztrdup(metafy(buf, used, META_HREALLOC)));
-    else {
-	fflush(stdout);
+    else
 	write(1, buf, used);
-    }
-    return !used;
+
+    return !seen;
 }
 
 static int
@@ -524,21 +515,7 @@
     int written;
 
     for (; len; len -= written, s += written) {
-	if ((written = write(cmd->fd, s, len)) < 0
-#if 0
-	    /* Same as above. */
-	    &&
-	    (cmd->block
-#ifdef EWOULDBLOCK
-			|| errno != EWOULDBLOCK
-#else
-#ifdef EAGAIN
-			|| errno != EAGAIN
-#endif
-#endif
-	     )
-#endif
-	    )
+	if ((written = write(cmd->fd, s, len)) < 0)
 	    return 1;
 	if (written < 0) {
 	    checkptycmd(cmd);

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: Misc. zpty tweaks, plus commentary

[Bart Schaefer]

On Nov 4, 11:29pm, I wrote:
}
} `zpty -w' already was able to stream into the pty (even though that's
} not documented); e.g. `zpty -w foo < file' writes the entire contents
} of the file to the pty [...] Now `zpty -r foo' can stream the output
} as well.  Unfortunately, it's still not possible to do both at once.
}
} The problem is that whenever zsh forks a builtin, it closes all
} descriptors numbered higher than 10, which includes the master pty
} descriptor.

Although a C-code-level fix would be preferable, a workaround has just
occurred to me:  Using a subshell will prevent the descriptors from
being closed.  So with 13116 applied, you should be able to do:

    zpty -b foo cat
    yes blah | (zpty -w foo) &
    (zpty -r foo) | less
    zpty -d foo

This reveals that still another remaining problem is that `zpty -w' on a
blocking pty doesn't stop when the process on the pty is killed.  There
doesn't seem to be any simple fix for this; write() itself is blocked,
and does not get interrupted with SIGPIPE as would normally occur.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: Misc. zpty tweaks, plus commentary

[Bart Schaefer]

On Nov 4, 11:29pm, Bart Schaefer wrote:
} Subject: PATCH: Misc. zpty tweaks, plus commentary
}
} The first hunk below simply adds strerror output to a couple of warning
} messages, so one has a better idea of why things went wrong.

Oh, bother.  I just realized that Sven's 13061 has never been committed.
I saw the ChangeLog entry for 13057 and confused the two patches.

I'll try to resolve the conflict and post a modified 13061.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net