On Mar 14 11:36, Åke Rehnman via Cygwin wrote:
> On 2020-03-14 11:23, Åke Rehnman wrote:
> > Your patch works (for my test case and screen). Question is if we have
> > to consider the case where ulen==0 ...

Thanks for testing!

> > > > BTW there is a gremlin in the "else if (ev)" line....
> > > A gremlin?  Would you mind to explain?  Btw., if you find a bug
> > > in the code, we do take patches :) https://cygwin.com/contrib.html
> > If we have an error event in ev it will make a blocking read even if
> > VTIME==0.

Ah, yeah, I was aware of that, I just ignored it for now since I'm not
sure what the best way to handle that is.

Two options come to mind, either ignoring these errors entirely, or
returning -1 with errno set to EIO, along the lines of the Linux test
for tty_io_error() at the start of tty_read.

However, I have a sinking feeling that the function needs a rewrite
anyway.

For instance, consider reading in blocking mode, which may result in
running the for loop more than once.  If the first loop successfully
read 4 bytes, and the second loop runs into an error from
ClearCommError, the function will return -1 with errno set, completely
ignoring the fact that 4 bytes have been read already.

It should return 4 in this case, and only the next run of
fhandler_serial::raw_read *might* return -1.

This code really shows its age...


> I forgot, also any CancelIo should be terminated with a blocking
> GetOverlappedResult() see this excellent blog post
> https://devblogs.microsoft.com/oldnewthing/20110202-00/?p=11613
> 
>    // took longer than 1 second - cancel it and give up
>    CancelIo(h);
>    WaitForSingleObject(o.hEvent, INFINITE); // added // Alternatively:
> GetOverlappedResult(h, &o, TRUE);
>    return WAIT_TIMEOUT;

Thanks, fixed in git.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer