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

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

[Sven Wischnowsky]

Bart Schaefer wrote:

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

Ouch. Right, hadn't thought about the change to polltty. Sorry.

Bye
 Sven


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

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

[Sven Wischnowsky]

Bart Schaefer wrote:

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

And it still hasn't. Hm, that's weird, makes me wonder what happened
with the other mails I sent yesterday afternoon (not to the list).

I'll just change the `?????' in the ChangeLog to `unposted' (and
change it again if the mail re-appears).

Bye
 Sven


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

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)

[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