* windows sockets vs. file descriptors bugs in Guile
@ 2009-10-01 4:15 Scott McPeak
2009-10-01 7:43 ` Ludovic Courtès
2009-10-02 22:33 ` Neil Jerram
0 siblings, 2 replies; 6+ messages in thread
From: Scott McPeak @ 2009-10-01 4:15 UTC (permalink / raw)
To: bug-guile
Hi,
Guile-1.8.7 appears to have socket support and Windows support, but
they don't seem to work together, despite the existence of things like
win32-socket.c. (I'm building Guile for Windows using the mingw cross
compiler on linux. Maybe it's different with Cygwin?)
Specifically, Guile assumes the POSIX rule that a socket is just a
file descriptor, but on Windows that does not work; socket functions
only accept sockets, and file functions only accept file descriptors.
Several Guile functions are affected; I don't think this is an
exhaustive list, but it's what I ran into while trying to get a simple
client and server working (see testcase below):
* fport_fill_input: Passes a socket to 'read'.
* write_all: Passes a socket to 'write'.
* fport_close: Passes a socket to 'close'. The EBADF error message is
then silently discarded (...), but the bug still manifests, e.g., as a
server that never closes its connections.
* scm_std_select: Passes a pipe file descriptor to 'select'.
I'm not sure what the best solution is in the Guile framework. I see
that ports have some sort of dynamic dispatch table, so perhaps
sockets should be made a distinct kind of port from a file port using
that mechanism. However, for expedience, I basically hacked the file
port functions by recognizing sockets as ports with a SCM_FILENAME
that is sym_socket and handling them differently. However, that is
certainly not perfect since 'set-port-filename!' can be used to change
the file name after creation.
Since there should be no harm in calling the socket-specific functions
for socket file descriptors on any operating system, and differences
in behavior among platforms make good hiding places for bugs, my patch
makes most of the hacks regardless of the platform. I've tested it on
linux/x86 and win32/x86.
For 'select', I don't understand the purpose of the sleep pipe, and,
whatever it does, it would likely be a lot of work to code it in a
Windows-compatible way, so I just removed it on Windows.
My testcase and patch are below.
-Scott
------------------------- testcase --------------------------
; testsockets.scm
; little sockets test client and server
; convenience
(define (printf format . args)
(apply simple-format (cons #t (cons format args))))
(define (sprintf format . args)
(apply simple-format (cons #f (cons format args))))
(define (fprintf port format . args)
(apply simple-format (cons port (cons format args))))
; script entry point
(define (main args)
(if (not (equal? (length args) 3))
(error (sprintf "usage: ~A (client|server) portnum" (car args))))
(let ((mode (cadr args))
(portnum (string->number (caddr args))))
(cond ((equal? mode "client")
(let ((sock (socket PF_INET SOCK_STREAM 0))
(addr (make-socket-address AF_INET INADDR_LOOPBACK
portnum)))
(connect sock addr)
(let ((c (read-char sock)))
(while (not (eof-object? c))
(display c)
(set! c (read-char sock))))
(close-port sock)))
((equal? mode "server")
(let ((listener (socket PF_INET SOCK_STREAM 0))
(addr (make-socket-address AF_INET INADDR_ANY portnum)))
(setsockopt listener SOL_SOCKET SO_REUSEADDR 1)
(bind listener addr)
(listen listener 5)
(printf "listening on port ~A\n" portnum)
; Process each connection sequentially.
(while #t
(let* ((accept-result (accept listener))
(sock (car accept-result))
(peer (cdr accept-result)))
(catch #t
(lambda ()
(printf "received connection from ~A\n" peer)
(fprintf sock "hi\n"))
(lambda (key . args)
; On Windows, if the client terminates quickly without
; reading any data, 'write-port' throws ECONNRESET.
(printf "exception: ~A ~S\n" key args)))
(false-if-exception (close-port sock))))))
(else
(error (sprintf "unknown mode: \"~A\"" mode))))))
; EOF
------------------------------ patch -----------------------------
diff --git a/libguile/fports.c b/libguile/fports.c
index 1d61191..2a2197b 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -26,6 +26,7 @@
#include <stdio.h>
#include <fcntl.h>
#include "libguile/_scm.h"
+#include "libguile/socket.h" /* scm_is_socket_port, etc. */
#include "libguile/strings.h"
#include "libguile/strports.h" /* scm_open_input_string */
#include "libguile/validate.h"
@@ -614,7 +615,13 @@ fport_fill_input (SCM port)
#ifndef __MINGW32__
fport_wait_for_input (port);
#endif /* !__MINGW32__ */
- SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+
+ if (scm_is_socket_port (port))
+ SCM_SYSCALL (count =
+ scm_socket_read_via_recv (fp->fdes, pt->read_buf,
pt->read_buf_size));
+ else
+ SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+
if (count == -1)
scm_syserror ("fport_fill_input");
if (count == 0)
@@ -734,12 +741,16 @@ scm_i_fport_truncate (SCM port, SCM length)
static void write_all (SCM port, const void *data, size_t remaining)
{
int fdes = SCM_FSTREAM (port)->fdes;
+ int is_socket = scm_is_socket_port (port);
while (remaining > 0)
{
size_t done;
- SCM_SYSCALL (done = write (fdes, data, remaining));
+ if (is_socket)
+ SCM_SYSCALL (done = scm_socket_write_via_send (fdes, data,
remaining));
+ else
+ SCM_SYSCALL (done = write (fdes, data, remaining));
if (done == -1)
SCM_SYSERROR;
@@ -897,7 +908,11 @@ fport_close (SCM port)
int rv;
fport_flush (port);
- SCM_SYSCALL (rv = close (fp->fdes));
+ if (scm_is_socket_port (port))
+ SCM_SYSCALL (rv = scm_socket_close (fp->fdes));
+ else
+ SCM_SYSCALL (rv = close (fp->fdes));
+
if (rv == -1 && errno != EBADF)
{
if (scm_gc_running_p)
diff --git a/libguile/socket.c b/libguile/socket.c
index b51e4f1..ce204f7 100644
--- a/libguile/socket.c
+++ b/libguile/socket.c
@@ -1642,6 +1642,42 @@ SCM_DEFINE (scm_sendto, "sendto", 3, 1, 1,
\f
+/* The functions in this section support using sockets on Windows,
+ * where the file descriptor functions cannot be used on sockets.
+ * However, in the interest of avoiding unnecessary platform
+ * differences, these functions are used on all platforms. */
+
+/* Return true if 'port' is a socket port. */
+int scm_is_socket_port(SCM port)
+{
+ return SCM_FILENAME (port) == sym_socket;
+}
+
+/* Do what POSIX 'read' system call would do, except for a file
+ * descriptor that is a socket. */
+int scm_socket_read_via_recv(int fd, void *buf, size_t len)
+{
+ return recv (fd, buf, len, 0 /*flags*/);
+}
+
+/* Like 'write' but for a socket. */
+int scm_socket_write_via_send(int fd, void const *buf, size_t len)
+{
+ return send (fd, buf, len, 0 /*flags*/);
+}
+
+/* Like 'close' but for a socket. */
+int scm_socket_close(int fd)
+{
+#ifdef __MINGW32__
+ return closesocket(fd);
+#else
+ return close(fd);
+#endif
+}
+
+
+\f
void
scm_init_socket ()
{
diff --git a/libguile/socket.h b/libguile/socket.h
index 146d283..525b2f8 100644
--- a/libguile/socket.h
+++ b/libguile/socket.h
@@ -64,6 +64,13 @@ SCM_API struct sockaddr *scm_c_make_socket_address
(SCM family, SCM address,
size_t *address_size);
SCM_API SCM scm_make_socket_address (SCM family, SCM address, SCM args);
+/* Socket functions on file descriptors, exported from socket.c so
+ * that fports.c does not have to know about system socket headers. */
+SCM_API int scm_is_socket_port(SCM port);
+SCM_API int scm_socket_read_via_recv(int fd, void *buf, size_t len);
+SCM_API int scm_socket_write_via_send(int fd, void const *buf, size_t len);
+SCM_API int scm_socket_close(int fd);
+
#endif /* SCM_SOCKET_H */
/*
diff --git a/libguile/threads.c b/libguile/threads.c
index 95a905c..b1c0bc4 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1419,6 +1419,17 @@ scm_threads_mark_stacks (void)
/*** Select */
+/* On Windows, we cannot use 'select' with non-socket file
+ * descriptors, so we cannot use the sleep pipe. I prefer to do this
+ * using regular 'if' rather than '#ifdef' since the latter is so
+ * disruptive to code comprehensibility, so I define a symbol that is
+ * 0 or 1. */
+#ifdef __MINGW32__
+# define SELECT_USE_SLEEP_PIPE 0
+#else
+# define SELECT_USE_SLEEP_PIPE 1
+#endif
+
int
scm_std_select (int nfds,
SELECT_TYPE *readfds,
@@ -1437,36 +1448,56 @@ scm_std_select (int nfds,
readfds = &my_readfds;
}
- while (scm_i_setup_sleep (t, SCM_BOOL_F, NULL, t->sleep_pipe[1]))
- SCM_TICK;
+ if (SELECT_USE_SLEEP_PIPE)
+ {
+ while (scm_i_setup_sleep (t, SCM_BOOL_F, NULL, t->sleep_pipe[1]))
+ SCM_TICK;
+
+ wakeup_fd = t->sleep_pipe[0];
+ }
- wakeup_fd = t->sleep_pipe[0];
ticket = scm_leave_guile ();
- FD_SET (wakeup_fd, readfds);
- if (wakeup_fd >= nfds)
- nfds = wakeup_fd+1;
+
+ if (SELECT_USE_SLEEP_PIPE)
+ {
+ FD_SET (wakeup_fd, readfds);
+ if (wakeup_fd >= nfds)
+ nfds = wakeup_fd+1;
+ }
+
res = select (nfds, readfds, writefds, exceptfds, timeout);
- t->sleep_fd = -1;
eno = errno;
- scm_enter_guile (ticket);
- scm_i_reset_sleep (t);
+ /* XXX: Is it important to set 'sleep_fd' before re-entering Guile
+ * mode? If not, then this assignment should move down into the
+ * next 'if' block for simplicity. But in that case, there is no
+ * point since 'scm_i_reset_sleep' also sets the field to -1. */
+ if (SELECT_USE_SLEEP_PIPE)
+ t->sleep_fd = -1;
- if (res > 0 && FD_ISSET (wakeup_fd, readfds))
- {
- char dummy;
- size_t count;
+ scm_enter_guile (ticket);
- count = read (wakeup_fd, &dummy, 1);
+ if (SELECT_USE_SLEEP_PIPE)
+ {
+ scm_i_reset_sleep (t);
- FD_CLR (wakeup_fd, readfds);
- res -= 1;
- if (res == 0)
+ if (res > 0 && FD_ISSET (wakeup_fd, readfds))
{
- eno = EINTR;
- res = -1;
+ char dummy;
+ size_t count;
+
+ count = read (wakeup_fd, &dummy, 1);
+
+ FD_CLR (wakeup_fd, readfds);
+ res -= 1;
+ if (res == 0)
+ {
+ eno = EINTR;
+ res = -1;
+ }
}
}
+
errno = eno;
return res;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: windows sockets vs. file descriptors bugs in Guile
2009-10-01 4:15 windows sockets vs. file descriptors bugs in Guile Scott McPeak
@ 2009-10-01 7:43 ` Ludovic Courtès
2009-10-02 22:33 ` Neil Jerram
1 sibling, 0 replies; 6+ messages in thread
From: Ludovic Courtès @ 2009-10-01 7:43 UTC (permalink / raw)
To: bug-guile
Hi Scott,
Thanks for the bug reports!
To make sure we don’t forget about them, can you submit them to the bug
tracker at http://savannah.gnu.org/bugs/?group=guile? You can just link
to or copy/paste your messages.
Ludo’.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: windows sockets vs. file descriptors bugs in Guile
2009-10-01 4:15 windows sockets vs. file descriptors bugs in Guile Scott McPeak
2009-10-01 7:43 ` Ludovic Courtès
@ 2009-10-02 22:33 ` Neil Jerram
2009-10-03 0:03 ` Scott McPeak
1 sibling, 1 reply; 6+ messages in thread
From: Neil Jerram @ 2009-10-02 22:33 UTC (permalink / raw)
To: Scott McPeak; +Cc: bug-guile
[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]
Scott McPeak <smcpeak@coverity.com> writes:
> Hi,
>
> Guile-1.8.7 appears to have socket support and Windows support, but
> they don't seem to work together, despite the existence of things like
> win32-socket.c.
I hit this too about two years ago, but never checked in my patches.
Sorry!
> (I'm building Guile for Windows using the mingw cross
> compiler on linux.
I was building on Windows with MSVC at the time - which I believe is
quite similar to mingw cross compiling.
> Maybe it's different with Cygwin?)
Yes, Cygwin has a thicker emulation layer underneath what Guile thinks
is the C library API.
> Specifically, Guile assumes the POSIX rule that a socket is just a
> file descriptor, but on Windows that does not work; socket functions
> only accept sockets, and file functions only accept file descriptors.
> Several Guile functions are affected; I don't think this is an
> exhaustive list, but it's what I ran into while trying to get a simple
> client and server working (see testcase below):
>
> * fport_fill_input: Passes a socket to 'read'.
>
> * write_all: Passes a socket to 'write'.
>
> * fport_close: Passes a socket to 'close'. The EBADF error message is
> then silently discarded (...), but the bug still manifests, e.g., as a
> server that never closes its connections.
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?
> * 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.
Regards,
Neil
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-socket-specific-operations-for-socket-ports-on-W.patch --]
[-- Type: text/x-diff, Size: 3186 bytes --]
From ea9af46454f044bf01094ea15b3eb0a58717bf58 Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Fri, 2 Oct 2009 23:13:05 +0100
Subject: [PATCH] Use socket-specific operations for socket ports on Windows
* libguile/socket.c (sym_socket): Made global, for use in identifying
socket ports in other files.
* libguile/fports.c (fport_fill_input): If port is a socket, and
#ifdef __MINGW32__, use recv to read from it instead of read.
(write_all, fport_flush): Similarly, use send instead of write.
(fport_close): Similarly, use closesocket instead of close.
---
libguile/fports.c | 34 ++++++++++++++++++++++++++++++----
libguile/socket.c | 2 +-
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/libguile/fports.c b/libguile/fports.c
index 5d37495..4bc5075 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -58,6 +58,7 @@
#ifdef __MINGW32__
# include <sys/stat.h>
# include <winsock2.h>
+extern SCM sym_socket;
#endif /* __MINGW32__ */
#include <full-write.h>
@@ -604,7 +605,14 @@ fport_fill_input (SCM port)
#ifndef __MINGW32__
fport_wait_for_input (port);
#endif /* !__MINGW32__ */
- SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+
+#ifdef __MINGW32__
+ if (scm_is_eq (SCM_FILENAME (port), sym_socket))
+ SCM_SYSCALL (count = recv (fp->fdes, pt->read_buf, pt->read_buf_size, 0));
+ else
+#endif
+ SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+
if (count == -1)
scm_syserror ("fport_fill_input");
if (count == 0)
@@ -689,7 +697,12 @@ static void write_all (SCM port, const void *data, size_t remaining)
{
size_t done;
- SCM_SYSCALL (done = write (fdes, data, remaining));
+#ifdef __MINGW32__
+ if (scm_is_eq (SCM_FILENAME (port), sym_socket))
+ SCM_SYSCALL (done = send (fdes, data, remaining, 0));
+ else
+#endif
+ SCM_SYSCALL (done = write (fdes, data, remaining));
if (done == -1)
SCM_SYSERROR;
@@ -774,7 +787,13 @@ fport_flush (SCM port)
{
long count;
- SCM_SYSCALL (count = write (fp->fdes, ptr, remaining));
+#ifdef __MINGW32__
+ if (scm_is_eq (SCM_FILENAME (port), sym_socket))
+ SCM_SYSCALL (count = send (fp->fdes, ptr, remaining, 0));
+ else
+#endif
+ SCM_SYSCALL (count = write (fp->fdes, ptr, remaining));
+
if (count < 0)
{
/* error. assume nothing was written this call, but
@@ -846,7 +865,14 @@ fport_close (SCM port)
int rv;
fport_flush (port);
- SCM_SYSCALL (rv = close (fp->fdes));
+
+#ifdef __MINGW32__
+ if (scm_is_eq (SCM_FILENAME (port), sym_socket))
+ SCM_SYSCALL (rv = closesocket (fp->fdes));
+ else
+#endif
+ SCM_SYSCALL (rv = close (fp->fdes));
+
if (rv == -1 && errno != EBADF)
{
if (scm_gc_running_p)
diff --git a/libguile/socket.c b/libguile/socket.c
index ea2ba5c..2cbb7ce 100644
--- a/libguile/socket.c
+++ b/libguile/socket.c
@@ -439,7 +439,7 @@ SCM_DEFINE (scm_inet_ntop, "inet-ntop", 2, 0, 0,
#endif /* HAVE_IPV6 */
-SCM_SYMBOL (sym_socket, "socket");
+SCM_GLOBAL_SYMBOL (sym_socket, "socket");
#define SCM_SOCK_FD_TO_PORT(fd) scm_fdes_to_port (fd, "r+0", sym_socket)
--
1.5.6.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: windows sockets vs. file descriptors bugs in Guile
2009-10-02 22:33 ` Neil Jerram
@ 2009-10-03 0:03 ` Scott McPeak
2010-03-27 22:06 ` Neil Jerram
0 siblings, 1 reply; 6+ messages in thread
From: Scott McPeak @ 2009-10-03 0:03 UTC (permalink / raw)
To: Neil Jerram; +Cc: bug-guile
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: windows sockets vs. file descriptors bugs in Guile
2009-10-03 0:03 ` Scott McPeak
@ 2010-03-27 22:06 ` Neil Jerram
2010-03-27 22:24 ` Neil Jerram
0 siblings, 1 reply; 6+ messages in thread
From: Neil Jerram @ 2010-03-27 22:06 UTC (permalink / raw)
To: Scott McPeak; +Cc: bug-guile
[-- Attachment #1: Type: text/plain, Size: 576 bytes --]
Scott McPeak <smcpeak@coverity.com> writes:
>> 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.
Hi Scott,
First, I'm sorry for leaving this hanging for so long. I'd like to
finalise this, first for 1.8.x and then for master, and the patch that I
now propose for 1.8.x is attached.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-windows-sockets-vs.-file-descriptors-bugs.patch --]
[-- Type: text/x-diff, Size: 9052 bytes --]
From 56a69318b55f0a231cf1531d2b7035c994df6579 Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Sat, 27 Mar 2010 21:41:54 +0000
Subject: [PATCH] Fix windows sockets vs. file descriptors bugs
Patch is from Scott McPeak, modified by me to be as conservative as
possible with respect to existing 1.8.x non-MinGW operation.
Scott writes:
"Guile-1.8.7 appears to have socket support and Windows support, but
they don't seem to work together, despite the existence of things like
win32-socket.c. (I'm building Guile for Windows using the mingw cross
compiler on linux. Maybe it's different with Cygwin?)
Specifically, Guile assumes the POSIX rule that a socket is just a
file descriptor, but on Windows that does not work; socket functions
only accept sockets, and file functions only accept file descriptors.
Several Guile functions are affected; I don't think this is an
exhaustive list, but it's what I ran into while trying to get a simple
client and server working (see testcase below):
* fport_fill_input: Passes a socket to 'read'.
* write_all: Passes a socket to 'write'.
* fport_close: Passes a socket to 'close'. The EBADF error message is
then silently discarded (...), but the bug still manifests, e.g., as a
server that never closes its connections.
* scm_std_select: Passes a pipe file descriptor to 'select'.
I'm not sure what the best solution is in the Guile framework. I see
that ports have some sort of dynamic dispatch table, so perhaps
sockets should be made a distinct kind of port from a file port using
that mechanism. However, for expedience, I basically hacked the file
port functions by recognizing sockets as ports with a SCM_FILENAME
that is sym_socket and handling them differently. However, that is
certainly not perfect since 'set-port-filename!' can be used to change
the file name after creation.
Since there should be no harm in calling the socket-specific functions
for socket file descriptors on any operating system, and differences
in behavior among platforms make good hiding places for bugs, my patch
makes most of the hacks regardless of the platform. I've tested it on
linux/x86 and win32/x86.
For 'select', I don't understand the purpose of the sleep pipe, and,
whatever it does, it would likely be a lot of work to code it in a
Windows-compatible way, so I just removed it on Windows."
Regarding the sleep pipe, as I've written in a comment in threads.c...
The consequence of not implementing the sleep pipe (on MinGW) is that
threads cannot receive signals, or more generally any asyncs, while
they are blocked waiting for I/O (select etc.), or for a mutex or
condition variable. But that is better than those
operations (i.e. I/O, mutexes and condition variables) not working at
all!
* libguile/fports.c: Include libguile/socket.h.
(fport_fill_input): On MinGW, use scm_is_socket_port and
scm_socket_read_via_recv.
(write_all): On MinGW, use scm_is_socket_port and
scm_socket_write_via_send.
(fport_close): On MinGW, use scm_is_socket_port and
scm_socket_close.
* libguile/socket.c (scm_is_socket_port, scm_socket_read_via_recv,
scm_socket_write_via_send, scm_socket_close): New functions for
MinGW.
* libguile/socket.h: Corresponding declarations.
* libguile/threads.c (scm_std_select): On MinGW, suppress all the bits
to do with the sleep pipe.
---
libguile/fports.c | 29 +++++++++++++++++++++++++----
libguile/socket.c | 31 +++++++++++++++++++++++++++++++
libguile/socket.h | 9 +++++++++
libguile/threads.c | 29 ++++++++++++++++++++++++++++-
4 files changed, 93 insertions(+), 5 deletions(-)
diff --git a/libguile/fports.c b/libguile/fports.c
index 007ee3f..c047a4e 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -26,6 +26,7 @@
#include <stdio.h>
#include <fcntl.h>
#include "libguile/_scm.h"
+#include "libguile/socket.h"
#include "libguile/strings.h"
#include "libguile/validate.h"
#include "libguile/gc.h"
@@ -596,8 +597,14 @@ fport_fill_input (SCM port)
#ifndef __MINGW32__
fport_wait_for_input (port);
-#endif /* !__MINGW32__ */
- SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+#else /* __MINGW32__ */
+ if (scm_is_socket_port (port))
+ SCM_SYSCALL (count =
+ scm_socket_read_via_recv (fp->fdes, pt->read_buf, pt->read_buf_size));
+ else
+#endif /* __MINGW32__ */
+ SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+
if (count == -1)
scm_syserror ("fport_fill_input");
if (count == 0)
@@ -717,12 +724,20 @@ scm_i_fport_truncate (SCM port, SCM length)
static void write_all (SCM port, const void *data, size_t remaining)
{
int fdes = SCM_FSTREAM (port)->fdes;
+#ifdef __MINGW32__
+ int is_socket = scm_is_socket_port (port);
+#endif
while (remaining > 0)
{
size_t done;
- SCM_SYSCALL (done = write (fdes, data, remaining));
+#ifdef __MINGW32__
+ if (is_socket)
+ SCM_SYSCALL (done = scm_socket_write_via_send (fdes, data, remaining));
+ else
+#endif
+ SCM_SYSCALL (done = write (fdes, data, remaining));
if (done == -1)
SCM_SYSERROR;
@@ -880,7 +895,13 @@ fport_close (SCM port)
int rv;
fport_flush (port);
- SCM_SYSCALL (rv = close (fp->fdes));
+#ifdef __MINGW32__
+ if (scm_is_socket_port (port))
+ SCM_SYSCALL (rv = scm_socket_close (fp->fdes));
+ else
+#endif
+ SCM_SYSCALL (rv = close (fp->fdes));
+
if (rv == -1 && errno != EBADF)
{
if (scm_gc_running_p)
diff --git a/libguile/socket.c b/libguile/socket.c
index cb954f4..b41f839 100644
--- a/libguile/socket.c
+++ b/libguile/socket.c
@@ -1642,7 +1642,38 @@ SCM_DEFINE (scm_sendto, "sendto", 3, 1, 1,
#undef FUNC_NAME
\f
+#ifdef __MINGW32__
+/* The functions in this section support using sockets on Windows,
+ * where the file descriptor functions cannot be used on sockets. */
+
+/* Return true if 'port' is a socket port. */
+int scm_is_socket_port(SCM port)
+{
+ return SCM_FILENAME (port) == sym_socket;
+}
+
+/* Do what POSIX 'read' system call would do, except for a file
+ * descriptor that is a socket. */
+int scm_socket_read_via_recv(int fd, void *buf, size_t len)
+{
+ return recv (fd, buf, len, 0 /*flags*/);
+}
+
+/* Like 'write' but for a socket. */
+int scm_socket_write_via_send(int fd, void const *buf, size_t len)
+{
+ return send (fd, buf, len, 0 /*flags*/);
+}
+
+/* Like 'close' but for a socket. */
+int scm_socket_close(int fd)
+{
+ return closesocket(fd);
+}
+#endif
+
+\f
void
scm_init_socket ()
{
diff --git a/libguile/socket.h b/libguile/socket.h
index 146d283..c8213eb 100644
--- a/libguile/socket.h
+++ b/libguile/socket.h
@@ -64,6 +64,15 @@ SCM_API struct sockaddr *scm_c_make_socket_address (SCM family, SCM address,
size_t *address_size);
SCM_API SCM scm_make_socket_address (SCM family, SCM address, SCM args);
+#ifdef __MINGW32__
+/* Socket functions on file descriptors, exported from socket.c so
+ * that fports.c does not have to know about system socket headers. */
+SCM_API int scm_is_socket_port(SCM port);
+SCM_API int scm_socket_read_via_recv(int fd, void *buf, size_t len);
+SCM_API int scm_socket_write_via_send(int fd, void const *buf, size_t len);
+SCM_API int scm_socket_close(int fd);
+#endif
+
#endif /* SCM_SOCKET_H */
/*
diff --git a/libguile/threads.c b/libguile/threads.c
index f2bb556..06aeb3c 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1419,6 +1419,15 @@ scm_threads_mark_stacks (void)
/*** Select */
+/* On Windows, we cannot use 'select' with non-socket file
+ * descriptors, so we cannot use the sleep pipe. The consequence of
+ * this is that threads cannot receive signals, or more generally any
+ * asyncs, while they are blocked waiting for I/O (select etc.), or
+ * for a mutex or condition variable. But that is better than those
+ * operations (i.e. I/O, mutexes and condition variables) not working
+ * at all!
+ */
+
int
scm_std_select (int nfds,
SELECT_TYPE *readfds,
@@ -1437,19 +1446,35 @@ scm_std_select (int nfds,
readfds = &my_readfds;
}
+#ifndef __MINGW32__
while (scm_i_setup_sleep (t, SCM_BOOL_F, NULL, t->sleep_pipe[1]))
SCM_TICK;
wakeup_fd = t->sleep_pipe[0];
+#endif
+
ticket = scm_leave_guile ();
+
+#ifndef __MINGW32__
FD_SET (wakeup_fd, readfds);
if (wakeup_fd >= nfds)
nfds = wakeup_fd+1;
+#endif
+
res = select (nfds, readfds, writefds, exceptfds, timeout);
- t->sleep_fd = -1;
eno = errno;
+
+#ifndef __MINGW32__
+ /* XXX: Is it important to set 'sleep_fd' before re-entering Guile
+ * mode? If not, then this assignment should move down into the
+ * next 'if' block for simplicity. But in that case, there is no
+ * point since 'scm_i_reset_sleep' also sets the field to -1. */
+ t->sleep_fd = -1;
+#endif
+
scm_enter_guile (ticket);
+#ifndef __MINGW32__
scm_i_reset_sleep (t);
if (res > 0 && FD_ISSET (wakeup_fd, readfds))
@@ -1467,6 +1492,8 @@ scm_std_select (int nfds,
res = -1;
}
}
+#endif
+
errno = eno;
return res;
}
--
1.5.6.5
[-- Attachment #3: Type: text/plain, Size: 4868 bytes --]
Most of that patch is yours, so we will need a copyright assignment from
you. Please say if you need help finding and submitting the papers for
that (or if it's a problem).
For master I expect the changes will be different, as I think there is
some support in Gnulib that we can use. I'll work on that shortly.
In response to your points about the differences between your patch and
a similar one that I had made...
> 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!'.
Good point; agreed.
> * 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.
Again, good point, thanks.
> * 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.
Those are good arguments, and for master (i.e. current and future
development) I'm happy to accept them (in principle; as already noted, I
expect the exact changes to be different). For 1.8.x, though, I'd
prefer to make this change in a way that minimises any possible risk to
the existing 1.8.x code, which is already quite well tested on a few
platforms. Hence in the attached patch there are quite a lot of
#ifdefs; I don't think we need to worry too much about those (and their
maintainability), because this isn't the codebase of the future.
One particular thing is that I didn't like your uses of
if (SELECT_USE_SLEEP_PIPE)
In fact I'm sure this will cause "conditional expression is constant"
warnings with several compilers. I've just used more #ifdefs instead.
>>> * 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.
All makes sense; thanks for explaining. (I can't remember what I had in
mind when I said above that we might not need scm_std_select any more.)
> 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.
Yes. I added a note in the patch about the consequence of not
implementing the sleep pipe - and the upshot is that I agree with you
that this is a useful compromise. Please let me know if you have any
comments on that.
Regards,
Neil
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: windows sockets vs. file descriptors bugs in Guile
2010-03-27 22:06 ` Neil Jerram
@ 2010-03-27 22:24 ` Neil Jerram
0 siblings, 0 replies; 6+ messages in thread
From: Neil Jerram @ 2010-03-27 22:24 UTC (permalink / raw)
To: Scott McPeak; +Cc: bug-guile
[-- Attachment #1: Type: text/plain, Size: 579 bytes --]
Neil Jerram <neil@ossau.uklinux.net> writes:
> Most of that patch is yours, so we will need a copyright assignment from
> you. Please say if you need help finding and submitting the papers for
> that (or if it's a problem).
I'm sorry, on rereading your previous email I see that you already said
"let me know what I need to do". So here's the answer.
If you think that you might contribute more Guile patches in future, you
could sign an assignment that will cover both this and any future
contributions. In this case please see the attached file
"request-assign.future".
[-- Attachment #2: request-assign.future --]
[-- Type: text/plain, Size: 971 bytes --]
Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.
Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
[What is the name of the program or package you're contributing to?]
[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]
[Do you have an employer who might have a basis to claim to own
your changes? Do you attend a school which might make such a claim?]
[For the copyright registration, what country are you a citizen of?]
[What year were you born?]
[Please write your email address here.]
[Please write your postal address here.]
[Which files have you changed so far, and which new files have you written
so far?]
[-- Attachment #3: Type: text/plain, Size: 265 bytes --]
Alternatively, to cover just the contributions that you already have
pending (i.e. including also the sleep and scm_copy_file patches), there
is a simpler copyright disclaimer. If you'd prefer this route, please
see the attached file "request-disclaim.changes".
[-- Attachment #4: request-disclaim.changes --]
[-- Type: text/plain, Size: 1327 bytes --]
Please email the following information to assign@gnu.org, and we will
send you the disclaimer form for your changes. This form is preferred
when your changes are small, they do not add any nontrivial new
files, and you are finished making them (aside perhaps from small bug
fixes).
If you would like to make further contributions to the same package,
and you would like to avoid the need to sign more papers when you
contribute them, you have another option: to sign a copyright
assignment covering your future changes. If that is what you want to
do, please tell the maintainer you would prefer to sign an assignment
of past and future changes.
Please use your full legal name (in ASCII characters) as the subject
line of the message.
-----------------------------------------------------------------------
REQUEST: SEND DISCLAIMER FORM
[What is the name of the program or package you're contributing to?]
[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]
[Do you have an employer who might have a basis to claim to own
your changes?]
[Please write your email address here.]
[Please write your snail address here.]
[Please list the files involved, or give a brief description of the changes
being disclaimed.]
[-- Attachment #5: Type: text/plain, Size: 25 bytes --]
Many thanks!
Neil
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-27 22:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-01 4:15 windows sockets vs. file descriptors bugs in Guile Scott McPeak
2009-10-01 7:43 ` Ludovic Courtès
2009-10-02 22:33 ` Neil Jerram
2009-10-03 0:03 ` Scott McPeak
2010-03-27 22:06 ` Neil Jerram
2010-03-27 22:24 ` Neil Jerram
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).