* zpty non-functional? @ 2013-08-24 12:44 Valodim Skywalker 2013-08-24 20:10 ` Bart Schaefer 0 siblings, 1 reply; 10+ messages in thread From: Valodim Skywalker @ 2013-08-24 12:44 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 308 bytes --] Hey there, I wanted to use zpty, but I can't even seem to get a trivial case using cat to work: $ zpty x cat $ echo hello | zpty -w x $ zpty -r x I tested on 4.3.11 and 5.0.2 with and without -b, and couldn't get any sort of data through the cat process. Am I missing something? Or is zpty borked? - V [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-24 12:44 zpty non-functional? Valodim Skywalker @ 2013-08-24 20:10 ` Bart Schaefer 2013-08-24 21:48 ` Peter Stephenson 0 siblings, 1 reply; 10+ messages in thread From: Bart Schaefer @ 2013-08-24 20:10 UTC (permalink / raw) To: Valodim Skywalker, zsh-workers On Aug 24, 2:44pm, Valodim Skywalker wrote: } } I tested on 4.3.11 and 5.0.2 with and without -b, and couldn't get any } sort of data through the cat process. Am I missing something? Or is zpty } borked? The completion system tests in "make check" rely on zpty, so it's quite definitely not borked in the general case. However ... } $ zpty x cat } $ echo hello | zpty -w x } $ zpty -r x A quick check with "ps"/"strace" indicates that "cat" is being stopped with a TTIN signal. That's kind of odd, it means that in spite of being on a new pty it is not being "attached" to that tty as group leader. I would say this is in fact a bug in zpty. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-24 20:10 ` Bart Schaefer @ 2013-08-24 21:48 ` Peter Stephenson 2013-08-25 0:01 ` Bart Schaefer 0 siblings, 1 reply; 10+ messages in thread From: Peter Stephenson @ 2013-08-24 21:48 UTC (permalink / raw) To: zsh-workers On Sat, 24 Aug 2013 13:10:41 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > A quick check with "ps"/"strace" indicates that "cat" is being stopped with > a TTIN signal. That's kind of odd, it means that in spite of being on a > new pty it is not being "attached" to that tty as group leader. I would say > this is in fact a bug in zpty. Hmm... this is well outside my comfort zone. I thought the following existing code: #ifdef TIOCSCTTY ioctl(slave, TIOCSCTTY, 0); #endif should have that effect (man tty_ioctl, all the following on Linux, actually Fedora 15): TIOCSCTTY int arg Make the given terminal the controlling terminal of the calling process. But it apparently doesn't, and sure enough with the patch below... % zpty x cat % zpty -w x this is some text % zpty -r x var % print $var this is some text (I think you need that "var" there, otherwise zpty waits for the process to exit, which isn't what you want. This is not the only place the syntax is a bit wayward...) The following is after the setsid() (or setpgrp() if that fails or doesn't exist). Maybe it should be conditional on one of those succeeding (and do we *really* want to do setpgrp() if setsid() fails, even with a warning)? Or maybe I'm missing the point entirely and just got lucky? diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c index 25ec7df..f31e0ba 100644 --- a/Src/Modules/zpty.c +++ b/Src/Modules/zpty.c @@ -344,6 +344,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock) if (get_pty(0, &slave)) exit(1); + attachtty(mypid); #ifdef TIOCGWINSZ /* Set the window size before associating with the terminal * * so that we don't get hit with a SIGWINCH. I'm paranoid. */ -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-24 21:48 ` Peter Stephenson @ 2013-08-25 0:01 ` Bart Schaefer 2013-08-25 18:59 ` Peter Stephenson 0 siblings, 1 reply; 10+ messages in thread From: Bart Schaefer @ 2013-08-25 0:01 UTC (permalink / raw) To: zsh-workers On Aug 24, 10:48pm, Peter Stephenson wrote: } } Hmm... this is well outside my comfort zone. I thought the following } existing code: } } #ifdef TIOCSCTTY } ioctl(slave, TIOCSCTTY, 0); } #endif } } TIOCSCTTY int arg } Make the given terminal the controlling terminal of the calling } process. } } But it apparently doesn't It makes the terminal the controlling terminal, but it doesn't make the process the group leader for that terminal. I suspect we also need the ioctl(slave, TIOCSPGRP, mypid); [or equivalent, e.g. tcsetpgrp()] that attachtty() is doing. I'm a bit surprised that calling attachtty() works, because it relies on SHTTY having been set to the right thing, and I don't see that anything in zpty.c is making sure that slave and SHTTY refer to the same fd; but perhaps it always is for some indirect reason ... Is it safe/reasonable to add SHTTY = slave; right before the attachtty() call in your patch? } The following is after the setsid() (or setpgrp() if that fails or } doesn't exist). Maybe it should be conditional on one of those } succeeding (and do we *really* want to do setpgrp() if setsid() fails, } even with a warning)? Or maybe I'm missing the point entirely and just } got lucky? The manual page for setsid says: The only error which can happen is EPERM. It is returned when the process group ID of any process equals the PID of the calling process. Thus, in particular, setsid fails if the calling process is already a process group leader. A check of the setpgrp() manual page indicates pretty much the same thing; there is no defined error that can result from setprp(0L, getpid()); the only errors are from attempting to set the process group of a different process than the caller. So only some completely unexpected error (interrupted system call?) could cause setsid() to fail, in which case trying again with setpgrp() is no worse off. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-25 0:01 ` Bart Schaefer @ 2013-08-25 18:59 ` Peter Stephenson 2013-08-25 22:01 ` Bart Schaefer 0 siblings, 1 reply; 10+ messages in thread From: Peter Stephenson @ 2013-08-25 18:59 UTC (permalink / raw) To: zsh-workers On Sat, 24 Aug 2013 17:01:57 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > I'm a bit > surprised that calling attachtty() works, because it relies on SHTTY > having been set to the right thing, and I don't see that anything in > zpty.c is making sure that slave and SHTTY refer to the same fd; but > perhaps it always is for some indirect reason ... > > Is it safe/reasonable to add > > SHTTY = slave; > > right before the attachtty() call in your patch? That seems to work, anyway. I added a test, but if instead of using the pipe I add a string as arguments to zpty -w it usually fails: I get some of the string followed by the whole of the string in the output. This suggests the write is doing something weird. I added the trick to make it use _exit() (as we haven't forked) at the end of the slave code, but that didn't help --- which it probably shouldn't anyway for various reasons, but I left it in since it looked logically correct anyway. So there's something else to investigate. diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c index 25ec7df..3821194 100644 --- a/Src/Modules/zpty.c +++ b/Src/Modules/zpty.c @@ -344,6 +344,8 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock) if (get_pty(0, &slave)) exit(1); + SHTTY = slave; + attachtty(mypid); #ifdef TIOCGWINSZ /* Set the window size before associating with the terminal * * so that we don't get hit with a SIGWINCH. I'm paranoid. */ @@ -398,6 +400,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock) opts[INTERACTIVE] = 0; execode(prog, 1, 0, "zpty"); stopmsg = 2; + mypid = 0; /* trick to ensure we _exit() */ zexit(lastval, 0); } master = movefd(master); diff --git a/Test/.distfiles b/Test/.distfiles index 689b695..ab92153 100644 --- a/Test/.distfiles +++ b/Test/.distfiles @@ -40,6 +40,7 @@ V04features.ztst V05styles.ztst V06parameter.ztst V07pcre.ztst +V08zpty.ztst Y01completion.ztst Y02compmatch.ztst Y03arguments.ztst diff --git a/Test/V08zpty.ztst b/Test/V08zpty.ztst new file mode 100644 index 0000000..d9d24c5 --- /dev/null +++ b/Test/V08zpty.ztst @@ -0,0 +1,20 @@ +# zpty is required by tests of interactive modes of the shell itself. +# This tests some extra things. + +%prep + + if ! zmodload zsh/zpty 2>/dev/null + then + ZTST_unimplemented="the zsh/zpty module is not available" + return 0 + fi + +%test + + zpty cat cat + print a line of text | zpty -w cat + var= + zpty -r cat var && print -r -- ${var%%$'\r\n'} + zpty -d cat +0:zpty with a process that does not set up the terminal +>a line of text -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-25 18:59 ` Peter Stephenson @ 2013-08-25 22:01 ` Bart Schaefer 2013-08-26 19:33 ` Peter Stephenson 0 siblings, 1 reply; 10+ messages in thread From: Bart Schaefer @ 2013-08-25 22:01 UTC (permalink / raw) To: zsh-workers On Aug 25, 7:59pm, Peter Stephenson wrote: } } I added a test, but if instead of using the pipe I add a string as } arguments to zpty -w it usually fails: I get some of the string } followed by the whole of the string in the output. This suggests the } write is doing something weird. I modified the test to use "zpty cat 'strace cat 2>file'" and to use "zpty -w cat sample strings" instead of the pipe, then examined the strace output. The "cat" process always reads/writes exactly what it is meant to, even when the test fails. I then suspected that the problem is with "ztpy -r" not with -w. If I change the test to write two lines: print a line of text | zpty -w cat var=; zpty -r cat var zpty -w cat sample strings var=; zpty -r cat var then it always succeeds. If I swap the order so that the pipe is the second of the two writes, then it always *fails*, repeating the same output ("sample strings") twice, but strace still shows cat reading and writing the correct two lines in the correct order. Further testing by using "cat > /dev/null" shows that when "zpty -w" is used, the pty acts as if it is in echo mode, so "zpty -r" reads back what zpty -w wrote. In the pipe mode, for some reason, this does not happen. ptywrite() is called just once in either case, and I confirmed that ptysettyinfo() is being called at pty setup. If I put in some debugging statements such as zwarn calls in ptywrite, then I sometimes get only partial echo behavior, e.g., I'll get back "samplesample strings" and then "a line of text". I also tried adding a pid > 0 branch to the zfork conditional in which close(slave) is done [which seems like the right thing anyway] but that did not fix the behavior, although I thought briefly that it changed it. Finally I tried putting a "sleep 1" in the test before the first write to the pty. This makes it succeed 100% of the time, regardless of the order of the pipe or direct write. This leads me to conclude that it's a race condition: The master writer may read back what it wrote, up until the point that there is a process on the slave side consuming it instead. Whether this means we want to copy the synch[] pipe hack from exec.c to prevent zpty from returning before the child process is running, or just document the possible race, I will leave up to consensus (or PWS). ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-25 22:01 ` Bart Schaefer @ 2013-08-26 19:33 ` Peter Stephenson 2013-08-27 0:54 ` Bart Schaefer 0 siblings, 1 reply; 10+ messages in thread From: Peter Stephenson @ 2013-08-26 19:33 UTC (permalink / raw) To: zsh-workers On Sun, 25 Aug 2013 15:01:04 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > This leads me to conclude that it's a race condition: The master writer > may read back what it wrote, up until the point that there is a process > on the slave side consuming it instead. That all sounds entirely convincing, thanks for the work. > Whether this means we want to copy the synch[] pipe hack from exec.c to > prevent zpty from returning before the child process is running, or just > document the possible race, I will leave up to consensus (or PWS). Here's a synch, as well as a test that was showing up the problem (though not every time). I've tried to catch as many funny cases for read/write as I could think of; this may be overkill but should be at worst harmless (unless there's still something I've missed). diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c index 3821194..fca0cc1 100644 --- a/Src/Modules/zpty.c +++ b/Src/Modules/zpty.c @@ -293,8 +293,8 @@ static int newptycmd(char *nam, char *pname, char **args, int echo, int nblock) { Ptycmd p; - int master, slave, pid, oineval = ineval; - char *oscriptname = scriptname; + int master, slave, pid, oineval = ineval, ret; + char *oscriptname = scriptname, syncch; Eprog prog; /* code borrowed from bin_eval() */ @@ -398,6 +398,20 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock) setsparam("TTY", ztrdup(ttystrname)); opts[INTERACTIVE] = 0; + + syncch = 0; + do { + ret = write(1, &syncch, 1); + } while (ret != 1 && ( +#ifdef EWOULDBLOCK + errno == EWOULDBLOCK || +#else +#ifdef EAGAIN + errno == EAGAIN || +#endif +#endif + errno == EINTR)); + execode(prog, 1, 0, "zpty"); stopmsg = 2; mypid = 0; /* trick to ensure we _exit() */ @@ -433,6 +447,18 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock) scriptname = oscriptname; ineval = oineval; + do { + ret = read(master, &syncch, 1); + } while (ret != 1 && ( +#ifdef EWOULDBLOCK + errno == EWOULDBLOCK || +#else +#ifdef EAGAIN + errno == EAGAIN || +#endif +#endif + errno == EINTR)); + return 0; } diff --git a/Test/V08zpty.ztst b/Test/V08zpty.ztst index d9d24c5..5b08fc2 100644 --- a/Test/V08zpty.ztst +++ b/Test/V08zpty.ztst @@ -12,9 +12,17 @@ %test zpty cat cat + zpty -w cat a line of text + var= + zpty -r cat var && print -r -- ${var%%$'\r\n'} + zpty -d cat +0:zpty with a process that does not set up the terminal: internal write +>a line of text + + zpty cat cat print a line of text | zpty -w cat var= zpty -r cat var && print -r -- ${var%%$'\r\n'} zpty -d cat -0:zpty with a process that does not set up the terminal +0:zpty with a process that does not set up the terminal: write via stdin >a line of text -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-26 19:33 ` Peter Stephenson @ 2013-08-27 0:54 ` Bart Schaefer 2013-08-27 12:01 ` Peter Stephenson 0 siblings, 1 reply; 10+ messages in thread From: Bart Schaefer @ 2013-08-27 0:54 UTC (permalink / raw) To: zsh-workers On Aug 26, 8:33pm, Peter Stephenson wrote: } } Here's a synch, as well as a test that was showing up the problem It just occurred to me that if the syncch [why two c?] trick solves the race, then it's probably not even necessary to write a byte on the pipe -- just close it in the child, and the parent will get EOF. On the other hand it also occurred to me that this might not always solve the race. It'd be better if syncch could be close-on-exec ... except that execode() might not actually do an exec() ... So this is probably the best we can do. -- Barton E. Schaefer ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-27 0:54 ` Bart Schaefer @ 2013-08-27 12:01 ` Peter Stephenson 2013-08-27 14:31 ` Bart Schaefer 0 siblings, 1 reply; 10+ messages in thread From: Peter Stephenson @ 2013-08-27 12:01 UTC (permalink / raw) To: zsh-workers On Mon, 26 Aug 2013 17:54:21 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Aug 26, 8:33pm, Peter Stephenson wrote: > } > } Here's a synch, as well as a test that was showing up the problem > > It just occurred to me that if the syncch [why two c?] trick solves > the race, then it's probably not even necessary to write a byte on > the pipe -- just close it in the child, and the parent will get EOF. syncch is "sync ch[ar]". All I've added is the read and write --- the pipe was there anyway as the key part of the master/slave interface. So this is about as minimal as we get, up to tests and loops I may have added that we don't really need. pws ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: zpty non-functional? 2013-08-27 12:01 ` Peter Stephenson @ 2013-08-27 14:31 ` Bart Schaefer 0 siblings, 0 replies; 10+ messages in thread From: Bart Schaefer @ 2013-08-27 14:31 UTC (permalink / raw) To: zsh-workers On Aug 27, 1:01pm, Peter Stephenson wrote: } } } All I've added is the read and write --- the pipe was there anyway as } the key part of the master/slave interface. Ah, sorry; I misread the patch. In exec.c synch is the array that holds the pair of pipe descriptors, so I was expecting a new pipe here. I spent a while wondering whether it could ever matter to either side of the pty that it had been used to transmit an extra byte before the real conversation started, but I didn't come up with any objection. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-08-27 14:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-24 12:44 zpty non-functional? Valodim Skywalker 2013-08-24 20:10 ` Bart Schaefer 2013-08-24 21:48 ` Peter Stephenson 2013-08-25 0:01 ` Bart Schaefer 2013-08-25 18:59 ` Peter Stephenson 2013-08-25 22:01 ` Bart Schaefer 2013-08-26 19:33 ` Peter Stephenson 2013-08-27 0:54 ` Bart Schaefer 2013-08-27 12:01 ` Peter Stephenson 2013-08-27 14:31 ` Bart Schaefer
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).