From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.133]) by sourceware.org (Postfix) with ESMTPS id 9510E3857028 for ; Fri, 17 Jul 2020 19:21:44 +0000 (GMT) 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@cygwin.com Received: from calimero.vinschen.de ([217.91.18.234]) by mrelayeu.kundenserver.de (mreue010 [212.227.15.167]) with ESMTPSA (Nemesis) id 1MFbBO-1k4hoI3x5q-00H3iS; Fri, 17 Jul 2020 21:21:40 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id 5E74FA8070B; Fri, 17 Jul 2020 21:21:40 +0200 (CEST) Date: Fri, 17 Jul 2020 21:21:40 +0200 From: Corinna Vinschen To: Marc Hoersken Cc: cygwin@cygwin.com Subject: Re: [PATCH] Fix poll/select signal socket as write ready on connect failure Message-ID: <20200717192140.GC16360@calimero.vinschen.de> Reply-To: cygwin@cygwin.com Mail-Followup-To: Marc Hoersken , cygwin@cygwin.com References: <20200716092553.GA3784@calimero.vinschen.de> <8092a464-53a1-b74a-04f1-4d95a242c2b3@marc-hoersken.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: General Cygwin discussions and problem reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Jul 2020 19:21:46 -0000 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