Hi Marc,
On Jul 17 20:56, Marc Hoersken via Cygwin wrote:
> Hi Corinna,
>
> Am 16.07.2020 um 11:25 schrieb Corinna Vinschen:
> [...]
> > So, rather than setting FD_WRITE at some later point in the code, what
> > about handling this where the other FD_CONNECT stuff is handled, by
> > not erasing the FD_CONNECT bit, just like with FD_CLOSE?
>
> I think this makes more sense, yes. I am just not sure if the socket should
> also be write ready in case of a socket error. Looking at the description of
> EINPROGRESS on the man page of connect [1], it seems like writability is
> given regardless of the connection being successful or not, but as soon as
> the connection attempt is no longer pending. For successful connections
> FD_WRITE will be given already, so we will only need to set it for failed
> connections regardless of a socket error in wsock_events->connect_errorcode.
> Therefore I suggest to move the line setting FD_WRITE [2] one level up
> outside of the else branch.
Sounds right to me.
> [...]
> I already tested your diff successfully, so this could be an alternative
> approach to the issue. I just think the wsock_events->connect_errorcode
> should also only be reset if FD_CONNECT is removed, right? So the if branch
> would need to be extended to include the second line [3] as well.
I don't agree here. The sole purpose for connect_errorcode is to set
SOL_SOCKET/SO_ERROR in case a caller requests FD_CONNECT and FD_CONNECT
is available. After being set once, SOL_SOCKET/SO_ERROR should not be
rewritten, given the description of SO_ERROR in `man 7 socket':
SO_ERROR
Get and clear the pending socket error. This socket option is
^^^^^^^^^
read-only. Expects an integer.
Therefore I'm inclined to push this:
diff --git a/winsup/cygwin/fhandler_socket_inet.cc
b/winsup/cygwin/fhandler_socket_inet.cc
index e5b0d2d1443e..2b50671e533d 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -352,9 +352,13 @@ fhandler_socket_wsock::evaluate_events (const long
event_mask, long &events,
WSASetLastError (wsa_err);
ret = SOCKET_ERROR;
}
- else
- wsock_events->events |= FD_WRITE;
- wsock_events->events &= ~FD_CONNECT;
+ /* Since FD_CONNECT is only given once, we have to keep FD_CONNECT
+ for connection failed sockets to have consistent behaviour in
+ programs calling poll/select multiple times. Example test to
+ non-listening port: curl -v 127.0.0.1:47 */
+ if (connect_state () != connect_failed)
+ wsock_events->events &= ~FD_CONNECT;
+ wsock_events->events |= FD_WRITE;
wsock_events->connect_errorcode = 0;
}
/* This test makes accept/connect behave as on Linux when accept/connect
@@ -376,12 +380,6 @@ fhandler_socket_wsock::evaluate_events (const long
event_mask, long &events,
if (erase)
wsock_events->events &= ~(events & ~(FD_WRITE | FD_CLOSE));
}
- /* Since FD_CONNECT is only given once, we manually need to set
- FD_WRITE for connection failed sockets to have consistent
- behaviour in programs calling poll/select multiple times.
- Example test to non-listening port: curl -v 127.0.0.1:47 */
- if ((connect_state () == connect_failed) && (event_mask & FD_WRITE))
- wsock_events->events |= FD_WRITE;
UNLOCK_EVENTS;
return ret;
Make sense?
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
--
Problem reports: https://cygwin.com/problems.html
FAQ: https://cygwin.com/faq/
Documentation: https://cygwin.com/docs.html
Unsubscribe info: https://cygwin.com/ml/#unsubscribe-simple
