PATCH: Misc. zpty tweaks, plus commentary

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   

PATCH: Zpty cleanup (merge 13061 with 13116)

[Bart Schaefer]

On Nov 5,  4:35am, Bart Schaefer wrote:
}
} Oh, bother.  I just realized that Sven's 13061 has never been committed.
} I'll try to resolve the conflict and post a modified 13061.

OK, I've resolved the conflict, but I've also got some comments about
read_poll() both before and after 13061.

Sven wrote:

> There were some things in [read_poll()] I think were really wrong,
> like testing `select() > 1' and like trying all possible tests if the
> first tests worked but returned zero.

Yes, `select() > 1' was almost certainly wrong.  That test should always
fail on a system that correctly implements select(), because there's only
one descriptor in the `foofd' set; it returns the *number of* descriptors
(*not* the number *of the* descriptor) for which the test succeeds.

However, that does not mean that the rest of the tests should be skipped
when select() returns 0.  A return of 0 means the select() timed out,
which (apparently) might happen under Cygwin even if there actually are
characters available to be read.  Peter/Andrej, is that the case?

On the other hand (hmm, does that make three hands?), reading from the
master side of a pty is not the same as reading from a tty.  Setting
VMIN to 0 doesn't cause read() to behave in a non-blocking fashion in
that case -- which is implied by the big comment in read_poll() that
mentions isatty(): isatty() is false on the master side of a pty, and
(IIRC) calling ioctl() on the master side actually changes the settings
on the slave side, which is entirely not what we want.  So I'm pretty
sure that if zpty is going to use read_poll() at all, it should always
pass polltty=0, not =1 as it presently does.  (The 13116 patch partly
changed that, but 13061 adds other calls to read_poll().)

[Late-breaking note:  *Somebody* should have noticed that read_poll()
calls gettyinfo() and settyinfo(), which of course affects SHTTY, and
has nothing whatever to do with the pty.  So polltty=1 is just a very
expensive no-op for zpty, and it really must pass 0.]

Finally, I note that read_poll() always sets VMIN to 1 at the end, which
is also wrong: it should remember the original setting and restore it.

Other stuff:

FIONREAD is compiled in only when not HAVE_SELECT, so the `if (ret < 0)'
test around the ioctl() in 13061 will always succeed; I removed the test.
On the other hand, the success of ioctl() should be tested before the
value it stores in `val' can be trusted.

The master side of the pty should be closed before sending the HUP to
the child process, not after.

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.

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.
I haven't attempted to fix this yet; it may be that doing a true non-
blocking read is the only way even when not under cygwin -- either that,
or actually treat the pty command as a job and notice the SIGCHLD when
it exits.)

There didn't seem to be any reason not to break up the documentation a
bit.  I think it's clearer this way.

Here's the resulting patch.  I no longer see any reason not to commit it;
the completion and compmatch tests still work, so I must not have broken
zpty in any especially horrible way.  The only thing that makes me a bit
uncomfortable is reversing the meaning of `-b' ... I've left it, but I
wonder if we shouldn't use a different option instead.

Index: Doc/Zsh/mod_zpty.yo
===================================================================
@@ -5,42 +5,73 @@
 
 startitem()
 findex(zpty)
-xitem(tt(zpty) [ tt(-e) ] [ tt(-b) ] var(name) var(command) [ var(args ...) ])
-xitem(tt(zpty) tt(-d) [ var(names) ... ])
-xitem(tt(zpty) tt(-w) [ tt(-n) ] var(name) var(strings ...))
-xitem(tt(zpty) tt(-r) var(name) [ var(param) [ var(pattern) ] ])
-xitem(tt(zpty) tt(-t) var(name))
-item(tt(zpty) [ tt(-L) ])(
+item(tt(zpty) [ tt(-e) ] [ tt(-b) ] var(name) var(command) [ var(args ...) ])(
 In the first form, the var(command) is started with the var(args) as
 arguments.  The command runs under a newly assigned pseudo-terminal; this
 is useful for running commands non-interactively which expect an
-interactive environment.  The var(name) given is used to refer to this
-command in later calls to tt(pty).  With the tt(-e) option given, the
-pseudo-terminal will be set up so that input characters are echoed and 
-with the tt(-b) option given, input and output from and to the
-pseudo-terminal will be blocking.
+interactive environment.  The var(name) is used to refer to this command
+in later calls to tt(zpty).
+
+With the tt(-e) option, the pseudo-terminal is set up so that input
+characters are echoed.
 
-The second form with the tt(-d) option is used to delete commands
-previously started by supplying a list of their var(name)s.  If no
+With the tt(-b) option, input to and output from the pseudo-terminal are
+made non-blocking.
+)
+item(tt(zpty) tt(-d) [ var(names) ... ])(
+The second form, with the tt(-d) option, is used to delete commands
+previously started, by supplying a list of their var(name)s.  If no
 var(names) are given, all commands are deleted.  Deleting a command causes
 the HUP signal to be sent to the corresponding process.
-
-The tt(-w) option can be used to send the command var(name) the given
+)
+item(tt(zpty) tt(-w) [ tt(-n) ] var(name) [ var(strings ...) ])(
+The tt(-w) option can be used to send the to command var(name) the given
 var(strings) as input (separated by spaces).  If the tt(-n) option is
-not given, a newline will be added at the end.
+em(not) given, a newline is added at the end.
+
+If no var(strings) are provided, the standard input is copied to the
+pseudo-terminal; this may stop before copying the full input if the
+pseudo-terminal is non-blocking.
 
-The tt(-r) option can be used to read the output of the command
-var(name).  Without a var(param) argument, the string read will be
-printed to standard output.  With a var(param) argument, the string
-read will be put in the parameter named var(param).  If the
-var(pattern) is also given, output will be read until the whole string 
-read matches the var(pattern).
-
-The tt(-t) option can be used to test whether the command var(name) is 
-still running.  It returns a zero value if the command is running and
-a non-zero value otherwise.
+Note that the command under the pseudo-terminal sees this input as if it
+were typed, so beware when sending special tty driver characters such as
+word-erase, line-kill, and end-of-file.
+)
+item(tt(zpty) tt(-r) [ tt(-t) ] var(name) [ var(param) [ var(pattern) ] ])(
+The tt(-r) option can be used to read the output of the command var(name).
+With only a var(name) argument, the output read is copied to the standard
+output.  Unless the pseudo-terminal is non-blocking, copying continues
+until the command under the pseudo-terminal exits; when non-blocking, only
+as much output as is immediately available is copied.  The return value is
+zero if any output is copied.
 
-The last form without any arguments is used to list the commands
+When also given a var(param) argument, at most one line is read and stored
+in the parameter named var(param).  Less than a full line may be read if
+the pseudo-terminal is non-blocking.  The return value is zero if at least
+one character is stored in var(param).
+
+If a var(pattern) is given as well, output is read until the whole string
+read matches the var(pattern), even in the non-blocking case.  The return
+value is zero if the string read matches the pattern, or if the command
+has exited but at least one character could still be read.  As of this
+writing, a maximum of one megabyte of output can be consumed this way; if
+a full megabyte is read without matching the pattern, the return value is
+non-zero.
+
+In all cases, the return value is non-zero if nothing could be read, and
+is tt(2) if this is because the command has finished.
+
+If the tt(-r) option is combined with the tt(-t) option, tt(zpty) tests
+whether output is available before trying to read.  If no output is
+available, tt(zpty) immediately returns the value tt(1).
+)
+item(tt(zpty) tt(-t) var(name))(
+The tt(-t) option without the tt(-r) option can be used to test
+whether the command var(name) is still running.  It returns a zero
+value if the command is running and a non-zero value otherwise.
+)
+item(tt(zpty) [ tt(-L) ])(
+The last form, without any arguments, is used to list the commands
 currently defined.  If the tt(-L) option is given, this is done in the
 form of calls to the tt(zpty) builtin.
 )
Index: Functions/Misc/nslookup
===================================================================
@@ -24,7 +24,7 @@
     [[ -z "$pager" ]] && pager="${opager:-more}"
 (( $#pmpt )) || pmpt=(-p '> ')
 
-zpty -b nslookup nslookup "$@"
+zpty nslookup nslookup "$@"
 
 zpty -r nslookup line '*
 > '
Index: Src/utils.c
===================================================================
@@ -1315,7 +1315,7 @@
 mod_export int
 read_poll(int fd, int *readchar, int polltty)
 {
-    int ret = 0;
+    int ret = -1;
     long mode = -1;
     char c;
 #ifdef HAVE_SELECT
@@ -1353,38 +1353,32 @@
      */
     if (polltty) {
 	gettyinfo(&ti);
-	ti.tio.c_cc[VMIN] = 0;
-	settyinfo(&ti);
+	if ((polltty = ti.tio.c_cc[VMIN])) {
+	    ti.tio.c_cc[VMIN] = 0;
+	    settyinfo(&ti);
+	}
     }
 #else
     polltty = 0;
 #endif
 #ifdef HAVE_SELECT
-    if (!ret) {
-	expire_tv.tv_sec = expire_tv.tv_usec = 0;
-	FD_ZERO(&foofd);
-	FD_SET(fd, &foofd);
-	if (select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv)
-	    > 1)
-	    ret = 1;
-    }
+    expire_tv.tv_sec = expire_tv.tv_usec = 0;
+    FD_ZERO(&foofd);
+    FD_SET(fd, &foofd);
+    ret = select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv);
 #else
 #ifdef FIONREAD
-    if (!ret) {
-	ioctl(fd, FIONREAD, (char *)&val);
-	if (val)
-	    ret = 1;
-    }
+    if (ioctl(fd, FIONREAD, (char *) &val) == 0)
+	ret = (val > 0);
 #endif
 #endif
 
-    if (!ret) {
+    if (ret <= 0) {
 	/*
 	 * Final attempt: set non-blocking read and try to read a character.
 	 * Praise Bill, this works under Cygwin (nothing else seems to).
 	 */
-	if ((polltty || setblock_fd(0, fd, &mode))
-	    && read(fd, &c, 1) > 0) {
+	if ((polltty || setblock_fd(0, fd, &mode)) && read(fd, &c, 1) > 0) {
 	    *readchar = STOUC(c);
 	    ret = 1;
 	}
@@ -1397,7 +1391,7 @@
 	settyinfo(&ti);
     }
 #endif
-    return ret;
+    return (ret > 0);
 }
 
 /**/
Index: Src/Modules/zpty.c
===================================================================
@@ -34,7 +34,6 @@
  * upper bound on the number of bytes we read (even if we are give a
  * pattern). */
 
-#define READ_LEN 1024
 #define READ_MAX (1024 * 1024)
 
 typedef struct ptycmd *Ptycmd;
@@ -46,9 +45,10 @@
     int fd;
     int pid;
     int echo;
-    int block;
+    int nblock;
     int fin;
     int read;
+    char *old;
 };
 
 static Ptycmd ptycmds;
@@ -264,7 +264,7 @@
 #endif /* __SVR4 */
 
 static int
-newptycmd(char *nam, char *pname, char **args, int echo, int block)
+newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 {
     Ptycmd p;
     int master, slave, pid;
@@ -380,14 +380,15 @@
     p->fd = master;
     p->pid = pid;
     p->echo = echo;
-    p->block = block;
+    p->nblock = nblock;
     p->fin = 0;
     p->read = -1;
+    p->old = NULL;
 
     p->next = ptycmds;
     ptycmds = p;
 
-    if (!block)
+    if (nblock)
 	ptynonblock(master);
 
     return 0;
@@ -411,12 +412,12 @@
     zsfree(p->name);
     freearray(p->args);
 
+    zclose(cmd->fd);
+
     /* We kill the process group the command put itself in. */
 
     kill(-(p->pid), SIGHUP);
 
-    zclose(cmd->fd);
-
     zfree(p, sizeof(*p));
 }
 
@@ -438,7 +439,7 @@
 {
     if (cmd->read != -1)
 	return;
-    if (!read_poll(cmd->fd, &cmd->read, !cmd->block) &&
+    if (!read_poll(cmd->fd, &cmd->read, 0) &&
 	kill(cmd->pid, 0) < 0) {
 	cmd->fin = 1;
 	zclose(cmd->fd);
@@ -448,8 +449,8 @@
 static int
 ptyread(char *nam, Ptycmd cmd, char **args)
 {
-    int blen = 256, used = 0, seen = 0, ret = 1;
-    char *buf = (char *) zhalloc((blen = 256) + 1);
+    int blen, used, seen = 0, ret = 0;
+    char *buf;
     Patprog prog = NULL;
 
     if (*args && args[1]) {
@@ -469,10 +470,20 @@
     } else
 	fflush(stdout);
 
+    if (cmd->old) {
+	used = strlen(cmd->old);
+	buf = (char *) zhalloc((blen = 256 + used) + 1);
+	strcpy(buf, cmd->old);
+	zsfree(cmd->old);
+	cmd->old = NULL;
+    } else {
+	used = 0;
+	buf = (char *) zhalloc((blen = 256) + 1);
+    }
     if (cmd->read != -1) {
-	buf[0] = (char) cmd->read;
-	buf[1] = '\0';
-	used = 1;
+	buf[used] = (char) cmd->read;
+	buf[used + 1] = '\0';
+	seen = used = 1;
 	cmd->read = -1;
     }
     do {
@@ -495,36 +506,60 @@
 	}
 	buf[used] = '\0';
 
-	if (!prog && ret <= 0)
+	if (!prog && (ret <= 0 || (*args && buf[used - 1] == '\n')))
 	    break;
-    } while (!errflag && !breaks && !retflag && !contflag &&
-	     (prog ? (used < READ_MAX && (!ret || !pattry(prog, buf))) :
-	      (used < READ_LEN)));
+    } while (!(errflag || breaks || retflag || contflag) &&
+	     used < READ_MAX && !(prog && ret && pattry(prog, buf)));
+
+    if (prog && ret < 0 &&
+#ifdef EWOULDBLOCK
+	errno == EWOULDBLOCK
+#else
+#ifdef EAGAIN
+	errno == EAGAIN
+#endif
+#endif
+	) {
+	cmd->old = ztrdup(buf);
+	used = 0;
 
+	return 1;
+    }
     if (*args)
 	setsparam(*args, ztrdup(metafy(buf, used, META_HREALLOC)));
-    else
+    else if (used)
 	write(1, buf, used);
 
-    return !seen;
+    return (seen ? 0 : cmd->fin + 1);
 }
 
 static int
 ptywritestr(Ptycmd cmd, char *s, int len)
 {
-    int written;
+    int written, all = 0;
 
-    for (; len; len -= written, s += written) {
-	if ((written = write(cmd->fd, s, len)) < 0)
-	    return 1;
+    for (; !errflag && !breaks && !retflag && !contflag && len;
+	 len -= written, s += written) {
+	if ((written = write(cmd->fd, s, len)) < 0 && cmd->nblock &&
+#ifdef EWOULDBLOCK
+	    errno == EWOULDBLOCK
+#else
+#ifdef EAGAIN
+	    errno == EAGAIN
+#endif
+#endif
+	    )
+	    return !all;
 	if (written < 0) {
 	    checkptycmd(cmd);
 	    if (cmd->fin)
 		break;
 	    written = 0;
 	}
+	if (written > 0)
+	    all += written;
     }
-    return 0;
+    return (all ? 0 : cmd->fin + 1);
 }
 
 static int
@@ -559,8 +594,9 @@
 bin_zpty(char *nam, char **args, char *ops, int func)
 {
     if ((ops['r'] && ops['w']) ||
-	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] || ops['t'] ||
+	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] ||
 				    ops['b'] || ops['L'])) ||
+	(ops['w'] && ops['t']) ||
 	(ops['n'] && (ops['b'] || ops['e'] || ops['r'] || ops['t'] ||
 		      ops['d'] || ops['L'])) ||
 	(ops['d'] && (ops['b'] || ops['e'] || ops['L'] || ops['t'])) ||
@@ -579,7 +615,10 @@
 	    return 1;
 	}
 	if (p->fin)
+	    return 2;
+	if (ops['t'] && p->read == -1 && !read_poll(p->fd, &p->read, 0))
 	    return 1;
+
 	return (ops['r'] ?
 		ptyread(nam, p, args + 1) :
 		ptywrite(p, args + 1, ops['n']));
@@ -629,7 +668,7 @@
 	    checkptycmd(p);
 	    if (ops['L'])
 		printf("%s %s%s%s ", nam, (p->echo ? "-e " : ""),
-		       (p->block ? "-b " : ""), p->name);
+		       (p->nblock ? "-b " : ""), p->name);
 	    else if (p->fin)
 		printf("(finished) %s: ", p->name);
 	    else

-- 
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: Zpty cleanup (merge 13061 with 13116)

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> > There were some things in [read_poll()] I think were really wrong,
> > like testing `select() > 1' and like trying all possible tests if the
> > first tests worked but returned zero.
> 
> ...
> 
> However, that does not mean that the rest of the tests should be skipped
> when select() returns 0.  A return of 0 means the select() timed out,
> which (apparently) might happen under Cygwin even if there actually are
> characters available to be read.  Peter/Andrej, is that the case?

I don't know about Cygwin, but that blocking read (line 1381) is
exactly the test I wanted to avoid. When building the patch I had a
case where zpty was waiting in checkptycmd() because of that (select()
correctly had returned zero, saying that there wasn't anything to be
read, the `timeout' being zero after all means that the select should
just do a poll, not wait).

I don't know if I can reproduce it, maybe I'll try at the weekend.

> ...
> 
> [Late-breaking note:  *Somebody* should have noticed that read_poll()
> calls gettyinfo() and settyinfo(), which of course affects SHTTY, and
> has nothing whatever to do with the pty.  So polltty=1 is just a very
> expensive no-op for zpty, and it really must pass 0.]

Ahem. Oops.

Hmhm. I wasn't too happy with calling read_poll() in zpty.c
anyway. The problem I wanted to solve is, of course, that a zpty
command should be marked as `finished' if there is still input to be
read even though the OS tells us that the process has exited (which
can happen and is very ugly).

I don't have any particular comments or patches for the other things. Yet.


Bye
 Sven


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

RE: PATCH: Zpty cleanup (merge 13061 with 13116)

[Andrej Borsenkow]

> >
> > However, that does not mean that the rest of the tests should be skipped
> > when select() returns 0.  A return of 0 means the select() timed out,
> > which (apparently) might happen under Cygwin even if there actually are
> > characters available to be read.  Peter/Andrej, is that the case?
>

I had not much time recently. Casual test shows, that zpty is currently
completely broken on current Cygwin (1.1.5-4 as of this writing); I do not
claim that it is zsh problem, but recent changes may have made it worse.

About select - so far there seems to be no known problems under cygwin (that
does not mean, there are no problems).

-andrej

read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116)

[Andrej Borsenkow]

Unless I completely miss something:

read_poll tries to read from fd and stores the character it has read in
readchar; that is cmd->read when called from checkptycmd. But this value does
not seem to be used anywhere in read loop that looks like read_poll simply
eats up this readahead char ...

-andrej

Re: read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116)

[Sven Wischnowsky]

Andrej Borsenkow wrote:

> Unless I completely miss something:
> 
> read_poll tries to read from fd and stores the character it has read in
> readchar; that is cmd->read when called from checkptycmd. But this value does
> not seem to be used anywhere in read loop that looks like read_poll simply
> eats up this readahead char ...

See zpty.c:474 and following lines.

Bye
 Sven


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

RE: read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116)

[Andrej Borsenkow]

>
> Andrej Borsenkow wrote:
>
> > Unless I completely miss something:
> >
> > read_poll tries to read from fd and stores the character it has read in
> > readchar; that is cmd->read when called from checkptycmd. But
> this value does
> > not seem to be used anywhere in read loop that looks like read_poll simply
> > eats up this readahead char ...
>
> See zpty.c:474 and following lines.
>

But this is before main loop. So, cmd->read should be -1 (it was not ever
changed yet), and ret == 0 (as set on entry into ptyread). The first thing
executed is then read_poll and then immediately read.

-andrej

PATCH: read_poll Re: PATCH: Zpty cleanup (merge 13061 with 13116)

[Bart Schaefer]

On Nov 8, 11:20am, Sven Wischnowsky wrote:
}
} Bart Schaefer wrote:
} 
} > However, that does not mean that the rest of the tests should be skipped
} > when select() returns 0.  A return of 0 means the select() timed out,
} > which (apparently) might happen under Cygwin even if there actually are
} > characters available to be read.  Peter/Andrej, is that the case?
} 
} I don't know about Cygwin, but that blocking read (line 1381) is
} exactly the test I wanted to avoid.

The read on line 1381 is *non-*blocking.  Since polltty is now always zero
when called from zpty, the setblock_fd() call is made before the read() is
attempted.  That was the patch I first committed which started me down the
road of merging in your changes.

On Nov 8,  1:42pm, Andrej Borsenkow wrote:
} Subject: RE: PATCH: Zpty cleanup (merge 13061 with 13116)
}
} I had not much time recently. Casual test shows, that zpty is currently
} completely broken on current Cygwin (1.1.5-4 as of this writing); I do not
} claim that it is zsh problem, but recent changes may have made it worse.

More on this in the other zpty thread ...

} About select - so far there seems to be no known problems under cygwin
} (that does not mean, there are no problems).

OK, if there are no known problems with select, then the test on line
1376 can change from `ret <= 0' to `ret < 0' -- and in that case it's
possible that we could split read_poll() into two cases, HAVE_SELECT v.
all the other code that's there.  I won't do that for now, though.

This still doesn't address my complaint that select() on my linux box
returns "ready for reading" when in fact the result of read() will be
an I/O error.  That makes no difference to `read -t' (for which the
read_poll() code was originally written) but it screws up the return
value of `zpty -r'.

On Nov 8,  5:03pm, Andrej Borsenkow wrote:
} Subject: read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116)
}
} read_poll tries to read from fd and stores the character it has read in
} readchar; that is cmd->read when called from checkptycmd. But this value does
} not seem to be used anywhere in read loop that looks like read_poll simply
} eats up this readahead char ...

You're correct.  The code Sven referenced at line 474 needs to be repeated
after line 484.

Index: Src/utils.c
===================================================================
@@ -1373,7 +1373,7 @@
 #endif
 #endif
 
-    if (ret <= 0) {
+    if (ret < 0) {
 	/*
 	 * Final attempt: set non-blocking read and try to read a character.
 	 * Praise Bill, this works under Cygwin (nothing else seems to).
Index: Src/Modules/zpty.c
===================================================================
@@ -482,6 +482,11 @@
 	    checkptycmd(cmd);
 	    if (cmd->fin)
 		break;
+	    if (cmd->read != -1) {
+		buf[++used] = (char) cmd->read;
+		seen = 1;
+		cmd->read = -1;
+	    }
 	}
 	if ((ret = read(cmd->fd, buf + used, 1)) == 1) {
 	    seen = 1;


-- 
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: read_poll Re: PATCH: Zpty cleanup (merge 13061 with 13116)

[Bart Schaefer]

On Nov 8,  3:58pm, Bart Schaefer wrote:
} Subject: PATCH: read_poll Re: PATCH: Zpty cleanup (merge 13061 with 13116)
}
} You're correct.  The code Sven referenced at line 474 needs to be repeated
} after line 484.

Sven's apparently also sent a patch for this which hasn't propagated
through the mailing list yet -- he committed the change.

There's a bug in my patch (++used should be used++) so I'll leave Sven's
instead.

-- 
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   

Bug in cygwin with select and master pty

[Andrej Borsenkow]

>
> } About select - so far there seems to be no known problems under cygwin
> } (that does not mean, there are no problems).
>
> OK, if there are no known problems with select, then the test on line
> 1376 can change from `ret <= 0' to `ret < 0' -- and in that case it's
> possible that we could split read_poll() into two cases, HAVE_SELECT v.
> all the other code that's there.  I won't do that for now, though.
>

Life is fun. 5 minutes later I discovered a bug that made blocking multiline
reads (with or without pattern) hang at the end of the first line. I sent sort
of a patch to cygwin list, but I am not sure if it really does the right thing
(it fixed the bug I've seen, O.K. The question is whether it introduced
another bugs :-)

Anyway, fiddling with currently released versions of cygwin is pretty useless
wrt to zpty (all of them are buggy in some way). Unfortunately (to us) zsh
seems to be the only program that has these problems. With all implications.

The pgrp problem still remains.

-andrej

RE: Bug in cygwin with select and master pty

[Andrej Borsenkow]

> >
> >Yes. Build problem with termcap, EOF problem with pty, one more problem in
> >pre-1.1.5 snapshots and the above select problem.
> 
> None of these statements are adequate to track any problems down, 
> unfortunately.
> 

All of them are fixed currently. 

-andrej

Re: Bug in cygwin with select and master pty

[Chris Faylor]

On Mon, Nov 13, 2000 at 02:12:50PM +0300, Andrej Borsenkow wrote:
>> I didn't see any followup to this but the new (1.1.5) release of cygwin
>> should not have this problem.
>
>Chris, are you hearing on this list too?

I've been reading the zsh-workers list for about four or five years, yes.

>> >Anyway, fiddling with currently released versions of cygwin is pretty
>> >useless wrt to zpty (all of them are buggy in some way).  Unfortunately
>> >(to us) zsh seems to be the only program that has these problems.  With
>> >all implications.
>>
>> Have all of these problems been reported to the cygwin mailing list?  Are
>> there still problems in 1.1.5?
>
>Yes. Build problem with termcap, EOF problem with pty, one more problem in
>pre-1.1.5 snapshots and the above select problem.

None of these statements are adequate to track any problems down, unfortunately.

>The select problem is not fixed in 1.1.5-6.

1.1.5-6 is history.

cgf

RE: Bug in cygwin with select and master pty

[Andrej Borsenkow]

> -----Original Message-----
> From: cgf@redhat.com [mailto:cgf@redhat.com]
> Sent: Sunday, November 12, 2000 11:20 PM
> To: zsh-workers@sunsite.auc.dk
> Subject: Re: Bug in cygwin with select and master pty
>
>
> In article <001e01c049b7$7f159ea0$21c9ca95@mow.siemens.ru>,
> Andrej Borsenkow <Andrej.Borsenkow@mow.siemens.ru> wrote:
> >> } About select - so far there seems to be no known problems under cygwin
> >> } (that does not mean, there are no problems).
> >>
> >>OK, if there are no known problems with select, then the test on line
> >>1376 can change from `ret <= 0' to `ret < 0' -- and in that case it's
> >>possible that we could split read_poll() into two cases, HAVE_SELECT v.
> >>all the other code that's there.  I won't do that for now, though.
> >
> >Life is fun.  5 minutes later I discovered a bug that made blocking
> >multiline reads (with or without pattern) hang at the end of the first
> >line.  I sent sort of a patch to cygwin list, but I am not sure if it
> >really does the right thing (it fixed the bug I've seen, O.K.  The
> >question is whether it introduced another bugs :-)
>
> I didn't see any followup to this but the new (1.1.5) release of cygwin
> should not have this problem.
>

Chris, are you hearing on this list too?

> >Anyway, fiddling with currently released versions of cygwin is pretty
> >useless wrt to zpty (all of them are buggy in some way).  Unfortunately
> >(to us) zsh seems to be the only program that has these problems.  With
> >all implications.
>
> Have all of these problems been reported to the cygwin mailing list?  Are
> there still problems in 1.1.5?
>

Yes. Build problem with termcap, EOF problem with pty, one more problem in
pre-1.1.5 snapshots and the above select problem.

The select problem is not fixed in 1.1.5-6.

> >The pgrp problem still remains.
>
> What is the pgrp problem?  Was this reported to the cygwin mailing list?
>

Some zpty code was changed and now zsh cannot attach to allocated pty (with
error message cannot set pgrp). No, this was not reported to cygwin list
because

- the problem resulted from zsh code change, so I first had to make sure it
was cygwin problem
- I have had enough "this is open source" replies. I'm sorry to say, but
reporting anything to cygwin list is pretty useless unless one can either
provide a patch or two-line program to reproduce the problem. I currently
cannot do either.

If you do not mind to update zsh CVS, you can see the last problem. Just do
something like

zmodload zsh/zpty
zpty sh /bin/sh

and now we have:

mw1g017@MW1G17C% zmodload zsh/zpty
mw1g017@MW1G17C% zpty sh /bin/sh
mw1g017@MW1G17C% ps -l
      PID    PPID    PGID     WINPID TTY  UID    STIME COMMAND
     1592       1    1592       1592   0 1006 14:05:53 /usr/bin/zsh
     1596    1592    1596       1596  -1 1006 14:06:14 /usr/bin/zsh
     1620    1592    1620       1624   0 1006 14:06:33 /usr/bin/ps

where provess 1596 is a child that tried to attach to allocated pty. If you
read the output you get:

mw1g017@MW1G17C% zpty -r sh foo \*$'\n'
mw1g017@MW1G17C% print -r $foo
zsh: can't set tty pgrp: not owner

This did work before last change; the main change was that zsh now opens both
master and slave fd's of pty with O_NOCTTY. This works on Unix.

-andrej

Re: Bug in cygwin with select and master pty

[cgf]

In article <001e01c049b7$7f159ea0$21c9ca95@mow.siemens.ru>,
Andrej Borsenkow <Andrej.Borsenkow@mow.siemens.ru> wrote:
>> } About select - so far there seems to be no known problems under cygwin
>> } (that does not mean, there are no problems).
>>
>>OK, if there are no known problems with select, then the test on line
>>1376 can change from `ret <= 0' to `ret < 0' -- and in that case it's
>>possible that we could split read_poll() into two cases, HAVE_SELECT v.
>>all the other code that's there.  I won't do that for now, though.
>
>Life is fun.  5 minutes later I discovered a bug that made blocking
>multiline reads (with or without pattern) hang at the end of the first
>line.  I sent sort of a patch to cygwin list, but I am not sure if it
>really does the right thing (it fixed the bug I've seen, O.K.  The
>question is whether it introduced another bugs :-)

I didn't see any followup to this but the new (1.1.5) release of cygwin
should not have this problem.

>Anyway, fiddling with currently released versions of cygwin is pretty
>useless wrt to zpty (all of them are buggy in some way).  Unfortunately
>(to us) zsh seems to be the only program that has these problems.  With
>all implications.

Have all of these problems been reported to the cygwin mailing list?  Are
there still problems in 1.1.5?

>The pgrp problem still remains.

What is the pgrp problem?  Was this reported to the cygwin mailing list?

cgf