X-Recipient:	archive-cygwin AT delorie DOT com
DKIM-Filter:	OpenDKIM Filter v2.11.0 sourceware.org 838903840C1B
DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com;
	s=default; t=1595013707;
	bh=2CkWnUf0pkS47hcWMBExlgE/lZR4qcp0PgMfLgt1iTU=;
	h=Date:From:To:Subject:References:In-Reply-To:List-Id:
	List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe:
	Reply-To:Cc:From;
	b=Y28zkaAk2zBwOWQxIjoz52puGYPJV+j+CKkUCEXMvxHN8uvGZz8IUp9K+U5UAAmQu
	LsCyj8MQm2E99TL3VJPs2l78tQDy2pFKMAbVwgcqnoZQV5YwsjdKxrTJAOK8eXTJsH
	OKhgEo8rHXEIShmM0G5iQoLJPXm1H+oWhDZ0mCKk=
X-Original-To:	cygwin AT cygwin DOT com
Delivered-To:	cygwin AT cygwin DOT com
DMARC-Filter:	OpenDMARC Filter v1.3.2 sourceware.org 9510E3857028
Authentication-Results:	sourceware.org;
	dmarc=none (p=none dis=none) header.from=cygwin.com
Authentication-Results:	sourceware.org;
	spf=fail smtp.mailfrom=corinna-cygwin AT cygwin DOT com
Date:	Fri, 17 Jul 2020 21:21:40 +0200
From:	Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To:	Marc Hoersken <info AT marc-hoersken DOT de>
Subject:	Re: [PATCH] Fix poll/select signal socket as write ready on connect
	failure
Message-ID:	<20200717192140.GC16360@calimero.vinschen.de>
Mail-Followup-To:	Marc Hoersken <info AT marc-hoersken DOT de>, 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>
	<8092a464-53a1-b74a-04f1-4d95a242c2b3 AT marc-hoersken DOT de>
MIME-Version:	1.0
In-Reply-To:	<8092a464-53a1-b74a-04f1-4d95a242c2b3@marc-hoersken.de>
X-Provags-ID:	V03:K1:Bo7FQPWSC1DjopcYO9dijibK1Gj7HpZ/quNG2brVd/lImfrtapp
	wBMNX6ZcrJyAipwbi89iDT7DY4ltgmv0CgbYMIxMG6D1UsipMiteehQxZczkQRkH3NePY6E
	cRG2+R8RoVi/e76PUpQm9U9gluZM5gKhpBWZoWY2nEFz58u7k7QF1yLFTf13heW7jb5vtit
	QcAUuXkR30EpkYbfqHa2g==
X-UI-Out-Filterresults:	notjunk:1;V03:K0:JcWbUAyxZkM=:pIBEpT0t5gBuf37ZkiFhuc
	ZWX6oPbNYYobXcftOoyn9mw5cnMAUROwB9eIfRz9kP8RAJe5wBbN6Ef/mJADsAS1VdVTpr8Qo
	WbTJIMBiI3NA2QVmDEQAmywZO0cGnKprfuyqVLchT6xZXTmOxj3aBgXZIs8yfw3GdkuUujZe2
	FJRzHlSv+3r1AGMp0ult7bpSy+dCADHf5kcfjwVYW3/9eILUrkmow7Yp3o+mthiSumjU0o6Yh
	rVCVHM5cBSYR+F6BOX++ANTNriCv7zsFOBMJzfAws0MZhCGaCwePpxp5+VLD/A3DLxj2vElln
	Qlyk0IaEy1PSvmDRZ4SaGsd3C/x7/mVVmtDGEHvTchnGdR9oMEst9aEl5lGNxiAbttLGrgZZp
	/COhLzk14qMsswpOVA76HAZrfapOiKe5mkv+YiC9mzP0A+xb8nBPjdo/t/iejhJZB0AxaoEYv
	2hrH4rp8/ngjtZoYsoIaW8FUdxItyGRfsB4ouZxw0cYzoLOJZZOKt6Wyu/KjWuqTpcs/4oe5H
	hkpYpq80TzMOy7HAIjW90+C5b3NOuJAD4i8fJcAyq5gfHL0jHAMaXbQ1ljBmA6kODHilpgv3x
	88j4J1t+j00NMHxomMz6iZoRcfmanOEpWf17A5ahLPjhvIvuYvc51Xt0Ly1eVw9a8ygudiSXQ
	0qmfBYjmm4afmL8qqY00Gqb8bflcsCdxq8FOmJ8iHkL6MCwHCEcIlTzT6Xdur7dJjLCXdql0Z
	3hp6OjrvVxnO/HdBkvE6OMMI7SpfFbla7jF00JXmBJ8bjZyHCU5ylD2pXLd587xNkwQ76RoKG
	LbBsz4LQzy7RDYTliOTPfh7GiphRS54TjymEyRcCFbSxkX4cTEHRm2o/q+ENaGUv5HRo/sk1Z
	IHto7EUAL2rz+HflvK/Q==
X-Spam-Status:	No, score=-104.1 required=5.0 tests=BAYES_00, GIT_PATCH_0,
	GOOD_FROM_CORINNA_CYGWIN, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE,
	RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL,
	TXREP autolearn=ham autolearn_force=no version=3.4.2
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>
Reply-To:	cygwin AT cygwin DOT com
Cc:	cygwin AT cygwin DOT com
Errors-To:	cygwin-bounces AT cygwin DOT com
Sender:	"Cygwin" <cygwin-bounces AT cygwin DOT com>

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

- Raw text -