From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Scott McPeak Newsgroups: gmane.lisp.guile.bugs Subject: Re: windows sockets vs. file descriptors bugs in Guile Date: Fri, 02 Oct 2009 17:03:02 -0700 Message-ID: <4AC694B6.2030900@coverity.com> References: <4AC42CD2.2060507@coverity.com> <87iqexbf3e.fsf@ossau.uklinux.net> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1254528293 16802 80.91.229.12 (3 Oct 2009 00:04:53 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 3 Oct 2009 00:04:53 +0000 (UTC) Cc: bug-guile@gnu.org To: Neil Jerram Original-X-From: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Sat Oct 03 02:04:45 2009 Return-path: Envelope-to: guile-bugs@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1Mts7B-00064w-8R for guile-bugs@m.gmane.org; Sat, 03 Oct 2009 02:04:45 +0200 Original-Received: from localhost ([127.0.0.1]:48946 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mts7A-0008MY-At for guile-bugs@m.gmane.org; Fri, 02 Oct 2009 20:04:44 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mts75-0008IQ-3n for bug-guile@gnu.org; Fri, 02 Oct 2009 20:04:39 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mts70-000864-70 for bug-guile@gnu.org; Fri, 02 Oct 2009 20:04:38 -0400 Original-Received: from [199.232.76.173] (port=36999 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mts70-00085m-2U for bug-guile@gnu.org; Fri, 02 Oct 2009 20:04:34 -0400 Original-Received: from smtp3.coverity.net ([38.99.42.225]:24628 helo=sf-exowa-1.migcoverity.net) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mts6z-0007JS-Gf for bug-guile@gnu.org; Fri, 02 Oct 2009 20:04:33 -0400 Original-Received: from d-linux64-03.sf.coverity.com ([10.0.6.3]) by sf-exowa-1.migcoverity.net over TLS secured channel with Microsoft SMTPSVC(6.0.3790.3959); Fri, 2 Oct 2009 17:03:02 -0700 User-Agent: Thunderbird 2.0.0.18 (X11/20081113) In-Reply-To: <87iqexbf3e.fsf@ossau.uklinux.net> X-OriginalArrivalTime: 03 Oct 2009 00:03:02.0211 (UTC) FILETIME=[DFDBB130:01CA43BC] X-detected-operating-system: by monty-python.gnu.org: Windows XP SP1+, 2000 SP3 X-BeenThere: bug-guile@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Bug reports for GUILE, GNU's Ubiquitous Extension Language" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Errors-To: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.bugs:4340 Archived-At: > I've attached my patch for these. It's a bit simpler than yours, and > also avoids needing copyright assignment from you. Can you take a look, > see if you notice any disadvantages compared with your version, and if > possible try it out? Except for the modifications to 'scm_std_select', the two patches are nearly equivalent. The differences are: * I exported 'scm_is_socket_port()' rather than proliferate the hack of checking for equality with 'sym_socket'. That seemed like a good idea because I know the hack is buggy: it is defeated by 'set-port-filename!'. * I avoided calling socket functions from fports.c, since translation units that use socket functions typically have to do a lot of gyrations at the beginning to pull in the right headers on the right systems. See the top of socket.c. Since fports.c does not have all that, there is a strong chance that it wouldn't compile on some systems, if it were not for the next point. * I made changes that affect all systems, rather than using #ifdefs to make it Windows-only. As indicated in the comments, there are two reasons for this: a. Syntax: Code is far less readable when sprinkled with #ifdefs. I've had the displeasure of reading and maintaining code with heavy #ifdefs, and it's not fun. Guile is currently relatively free of such hacks, because it does a good job of factoring system-specific functionality so it's only exposed to a limited set of modules, and I was trying to keep it that way. b. Semantics: Whenever a program explicitly does something different depending on the platform, that's an invitation to a platform-specific bug. If indeed it is correct to call to 'recv(_,_,_,0)' on a socket instead of 'read(_,_,_)' (an equivalence POSIX guarantees) on Windows, then it should be correct to do so on every platform. And if it's wrong or inefficient on some platform, we'd like to know ASAP, right? These differences are mainly stylistic and future-proofing. You're certainly free to ignore them, I just wanted to elaborate on the reasons in case they weren't clear. >> * scm_std_select: Passes a pipe file descriptor to 'select'. > > I have no record of hitting this one; I suspect my code at the time just > didn't use `sleep'. I'm wondering if we still need scm_std_select in > Guile now anyway. I'll write again about that. As for the change to 'scm_std_select', without that change, the code does not work. Using my original testcase (plus a couple of calls to 'flush-all-ports', necessary to see the "listening" message, that I inadvertently omitted in my original post), Guile-1.8.7 with my changes removed and yours added instead says: > ./guile.exe -e main -s testsockets.scm server 8888 listening on port 8888 ERROR: In procedure accept: ERROR: Socket operation on non-socket (exit code of 1) The reason for this is that 'scm_accept' calls 'scm_std_select', which sticks a pipe file descriptor into the set of file descriptors to be checked for reading ('readfds'), which Windows does not like. My program never calls 'sleep'--Guile adds the sleep pipe unconditionally. Once I add just my changes to threads.c on top of your changes to socket.c, the test case works perfectly. While the changes to 'scm_std_select' are certainly the ugliest of those I proposed, some sort of change is required in order to get socket server processes to work in Guile-1.8.7 on Windows, as my test case shows. As for copyright on my patch, I'm happy to assign copyright; just let me know what I need to do. -Scott