* Re: read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116)
@ 2000-11-08 14:09 Sven Wischnowsky
2000-11-08 14:46 ` Andrej Borsenkow
0 siblings, 1 reply; 16+ messages in thread
From: Sven Wischnowsky @ 2000-11-08 14:09 UTC (permalink / raw)
To: zsh-workers
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-08 14:09 read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116) Sven Wischnowsky @ 2000-11-08 14:46 ` Andrej Borsenkow 2000-11-08 15:58 ` PATCH: read_poll " Bart Schaefer 0 siblings, 1 reply; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-08 14:46 UTC (permalink / raw) To: Sven Wischnowsky, zsh-workers > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* PATCH: read_poll Re: PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-08 14:46 ` Andrej Borsenkow @ 2000-11-08 15:58 ` Bart Schaefer 2000-11-08 17:00 ` Bart Schaefer 2000-11-08 19:10 ` Bug in cygwin with select and master pty Andrej Borsenkow 0 siblings, 2 replies; 16+ messages in thread From: Bart Schaefer @ 2000-11-08 15:58 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: PATCH: read_poll Re: PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-08 15:58 ` PATCH: read_poll " Bart Schaefer @ 2000-11-08 17:00 ` Bart Schaefer 2000-11-08 19:10 ` Bug in cygwin with select and master pty Andrej Borsenkow 1 sibling, 0 replies; 16+ messages in thread From: Bart Schaefer @ 2000-11-08 17:00 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Bug in cygwin with select and master pty 2000-11-08 15:58 ` PATCH: read_poll " Bart Schaefer 2000-11-08 17:00 ` Bart Schaefer @ 2000-11-08 19:10 ` Andrej Borsenkow 1 sibling, 0 replies; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-08 19:10 UTC (permalink / raw) To: zsh-workers > > } 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in cygwin with select and master pty @ 2000-11-12 20:19 cgf 2000-11-13 11:12 ` Andrej Borsenkow 0 siblings, 1 reply; 16+ messages in thread From: cgf @ 2000-11-12 20:19 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: Bug in cygwin with select and master pty 2000-11-12 20:19 cgf @ 2000-11-13 11:12 ` Andrej Borsenkow 2000-11-13 16:21 ` Chris Faylor 0 siblings, 1 reply; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-13 11:12 UTC (permalink / raw) To: cgf, zsh-workers > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Bug in cygwin with select and master pty 2000-11-13 11:12 ` Andrej Borsenkow @ 2000-11-13 16:21 ` Chris Faylor 2000-11-13 16:59 ` Andrej Borsenkow 0 siblings, 1 reply; 16+ messages in thread From: Chris Faylor @ 2000-11-13 16:21 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: Bug in cygwin with select and master pty 2000-11-13 16:21 ` Chris Faylor @ 2000-11-13 16:59 ` Andrej Borsenkow 0 siblings, 0 replies; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-13 16:59 UTC (permalink / raw) To: zsh-workers > > > >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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: PATCH: Zpty cleanup (merge 13061 with 13116) @ 2000-11-08 10:20 Sven Wischnowsky 2000-11-08 10:42 ` Andrej Borsenkow 0 siblings, 1 reply; 16+ messages in thread From: Sven Wischnowsky @ 2000-11-08 10:20 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-08 10:20 PATCH: Zpty cleanup (merge 13061 with 13116) Sven Wischnowsky @ 2000-11-08 10:42 ` Andrej Borsenkow 0 siblings, 0 replies; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-08 10:42 UTC (permalink / raw) To: zsh-workers > > > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* PATCH: Misc. zpty tweaks, plus commentary @ 2000-11-04 23:29 Bart Schaefer 2000-11-05 1:59 ` Bart Schaefer 2000-11-05 4:35 ` Bart Schaefer 0 siblings, 2 replies; 16+ messages in thread From: Bart Schaefer @ 2000-11-04 23:29 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: PATCH: Misc. zpty tweaks, plus commentary 2000-11-04 23:29 PATCH: Misc. zpty tweaks, plus commentary Bart Schaefer @ 2000-11-05 1:59 ` Bart Schaefer 2000-11-05 4:35 ` Bart Schaefer 1 sibling, 0 replies; 16+ messages in thread From: Bart Schaefer @ 2000-11-05 1:59 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: PATCH: Misc. zpty tweaks, plus commentary 2000-11-04 23:29 PATCH: Misc. zpty tweaks, plus commentary Bart Schaefer 2000-11-05 1:59 ` Bart Schaefer @ 2000-11-05 4:35 ` Bart Schaefer 2000-11-05 9:12 ` PATCH: Zpty cleanup (merge 13061 with 13116) Bart Schaefer 1 sibling, 1 reply; 16+ messages in thread From: Bart Schaefer @ 2000-11-05 4:35 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-05 4:35 ` Bart Schaefer @ 2000-11-05 9:12 ` Bart Schaefer 2000-11-08 14:03 ` read_poll " Andrej Borsenkow 0 siblings, 1 reply; 16+ messages in thread From: Bart Schaefer @ 2000-11-05 9:12 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116) 2000-11-05 9:12 ` PATCH: Zpty cleanup (merge 13061 with 13116) Bart Schaefer @ 2000-11-08 14:03 ` Andrej Borsenkow 0 siblings, 0 replies; 16+ messages in thread From: Andrej Borsenkow @ 2000-11-08 14:03 UTC (permalink / raw) To: Bart Schaefer, zsh-workers 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2000-11-13 16:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2000-11-08 14:09 read_poll RE: PATCH: Zpty cleanup (merge 13061 with 13116) Sven Wischnowsky 2000-11-08 14:46 ` Andrej Borsenkow 2000-11-08 15:58 ` PATCH: read_poll " Bart Schaefer 2000-11-08 17:00 ` Bart Schaefer 2000-11-08 19:10 ` Bug in cygwin with select and master pty Andrej Borsenkow -- strict thread matches above, loose matches on Subject: below -- 2000-11-12 20:19 cgf 2000-11-13 11:12 ` Andrej Borsenkow 2000-11-13 16:21 ` Chris Faylor 2000-11-13 16:59 ` Andrej Borsenkow 2000-11-08 10:20 PATCH: Zpty cleanup (merge 13061 with 13116) Sven Wischnowsky 2000-11-08 10:42 ` Andrej Borsenkow 2000-11-04 23:29 PATCH: Misc. zpty tweaks, plus commentary Bart Schaefer 2000-11-05 1:59 ` Bart Schaefer 2000-11-05 4:35 ` Bart Schaefer 2000-11-05 9:12 ` PATCH: Zpty cleanup (merge 13061 with 13116) Bart Schaefer 2000-11-08 14:03 ` read_poll " Andrej Borsenkow
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).