Re: windows sockets vs. file descriptors bugs in Guile

[Scott McPeak <smcpeak@coverity.com>]

> 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