X-Recipient:	archive-cygwin AT delorie DOT com
DKIM-Filter:	OpenDKIM Filter v2.11.0 sourceware.org 0C20139DEC32
DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com;
	s=default; t=1595012187;
	bh=++wf6UtnzWRYCL6HnbBFJl066MYvLhT3RkUAQvdrIUU=;
	h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe:
	List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:
	From;
	b=QoeYcrxFdakjCvqowYlO7MJzG3MuM0oPLf8AFazNRYx6ZOhMG4yUcacTdQU0cJ48j
	LU9Zk5I8Byj03OaQcn5AoBVnAKlaaZwYQZNpDULe0+pPNqZ3Cndn6+sUc29/DDIYca
	1Vaw7Nq7xXeWL4ZEc9E63AoGiDNlhTK3RMvdzYO0=
X-Original-To:	cygwin AT cygwin DOT com
Delivered-To:	cygwin AT cygwin DOT com
DMARC-Filter:	OpenDMARC Filter v1.3.2 sourceware.org A96EF39DEC1F
X-Virus-Scanned:	Debian amavisd-new at policy02-mors.netcup.net
X-Spam-Score:	-3.1
X-Spam-Level:
X-Spam-Status:	No, score=-10.3 required=5.0 tests=BAYES_00, BODY_8BITS,
	DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0,
	RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS,
	TXREP autolearn=ham autolearn_force=no version=3.4.2
Received-SPF:	pass (mx2f80: connection is authenticated)
Subject:	Re: [PATCH] Fix poll/select signal socket as write ready on connect
	failure
To:	cygwin AT cygwin DOT com
References:	<efcd7b07-ccc3-a8a4-04c1-94f63012f042 AT marc-hoersken DOT de>
	<20200716092553 DOT GA3784 AT calimero DOT vinschen DOT de>
Message-ID:	<8092a464-53a1-b74a-04f1-4d95a242c2b3@marc-hoersken.de>
Date:	Fri, 17 Jul 2020 20:56:29 +0200
User-Agent:	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
	Thunderbird/68.10.0
MIME-Version:	1.0
In-Reply-To:	<20200716092553.GA3784@calimero.vinschen.de>
X-PPP-Message-ID:	<159501217816 DOT 30496 DOT 6776479110986724273 AT mx2f80 DOT netcup DOT net>
X-PPP-Vhost:	marc-hoersken.de
X-NC-CID:	T5Hz7bV/SplOtMo8yBn7egVu9glTxwsya2eAnVH21+1ljybx3Q==
X-Spam-Checker-Version:	SpamAssassin 3.4.2 (2018-09-13) on
	server2.sourceware.org
X-BeenThere:	cygwin AT cygwin DOT com
X-Mailman-Version:	2.1.29
List-Id:	General Cygwin discussions and problem reports <cygwin.cygwin.com>
List-Unsubscribe:	<http://cygwin.com/mailman/options/cygwin>,
	<mailto:cygwin-request AT cygwin DOT com?subject=unsubscribe>
List-Archive:	<https://cygwin.com/pipermail/cygwin/>
List-Post:	<mailto:cygwin AT cygwin DOT com>
List-Help:	<mailto:cygwin-request AT cygwin DOT com?subject=help>
List-Subscribe:	<http://cygwin.com/mailman/listinfo/cygwin>,
	<mailto:cygwin-request AT cygwin DOT com?subject=subscribe>
From:	Marc Hoersken via Cygwin <cygwin AT cygwin DOT com>
Reply-To:	Marc Hoersken <info AT marc-hoersken DOT de>
Errors-To:	cygwin-bounces AT cygwin DOT com
Sender:	"Cygwin" <cygwin-bounces AT cygwin DOT com>
X-MIME-Autoconverted:	from base64 to 8bit by delorie.com id 06HJIDTg018433

Hi Corinna,

Am 16.07.2020 um 11:25 schrieb Corinna Vinschen:
> Thanks for the patch.  I pushed it.


thanks for pushing it already. Please excuse my delayed response, family 
live kept me busy.


> But then I got second thoughts in terms of how to fix the issue.


Yes, I also got second thoughts yesterday about my initial approach.


> The reason is that the FD_CLOSE problem shouldn't exist,
> simply for the fact that we never remove FD_CLOSE from
> the events mask, see
>
> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;hb=HEAD#l377


Thanks, I also understood in the meantime, that some flags/events are 
not removed from wsock_events->event. FD_CLOSE seems not to be affected 
as you described and I was unable to produce an issue in case the 
connection was closed from the server side. So only the FD_CONNECT to 
FD_WRITE handling remained problematic (before the patch).


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


> diff --git a/winsup/cygwin/fhandler_socket_inet.cc b/winsup/cygwin/fhandler_socket_inet.cc
> index e5b0d2d1443e..b64d96225db1 100644
> --- a/winsup/cygwin/fhandler_socket_inet.cc
> +++ b/winsup/cygwin/fhandler_socket_inet.cc
> @@ -354,7 +354,12 @@ fhandler_socket_wsock::evaluate_events (const long event_mask, long &events,
>   	    }
>   	  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->connect_errorcode = 0;
>   	}
>         /* This test makes accept/connect behave as on Linux when accept/connect
> @@ -376,12 +381,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;
>
> What do you think?


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.


Everything together, I think our suggestions together would look like this:


diff --git a/winsup/cygwin/fhandler_socket_inet.cc 
b/winsup/cygwin/fhandler_socket_inet.cc
index e5b0d2d14..84cd63698 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -352,10 +352,15 @@ 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;
-         wsock_events->connect_errorcode = 0;
+         wsock_events->events |= FD_WRITE;
+         /* Since FD_CONNECT is only given once, we have to keep FD_CONNECT
+            for connection failed sockets to have consistent behaviour
+            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->connect_errorcode = 0;
+         }
         }
        /* This test makes accept/connect behave as on Linux when 
accept/connect
           is called on a socket for which shutdown has been called.  
The second
@@ -376,12 +381,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;



Best regards,
Marc

[1] https://www.man7.org/linux/man-pages/man2/connect.2.html#ERRORS

[2] 
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;h=e5b0d2d1443ecc4430104f6cfb78bf580a8116e5;hb=aa86784937ec7868c358dd90ea5e5324f0be750d#l356

[3] 
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;h=e5b0d2d1443ecc4430104f6cfb78bf580a8116e5;hb=aa86784937ec7868c358dd90ea5e5324f0be750d#l358

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

- Raw text -