GNOME Bugzilla – Bug 686786
g_socket_get_available_bytes() returns wrong value on Windows/OSX
Last modified: 2013-08-31 15:44:40 UTC
+++ This bug was initially created as a clone of Bug #610364 +++ Summary of that other bug: > On Windows, "FIONREAD returns the amount of data that can be read in a single > call to the recv function, which may not be the same as the total amount of > data queued on the socket. If s is message oriented (for example, type > SOCK_DGRAM), FIONREAD still returns the amount of pending data in the network > buffer, however, the amount that can actually be read in a single call to the > recv function is limited to the data size written in the send or sendto > function call.", which means this is not really what we want. > > On OSX, ioctl(FIONREAD) returns the total buffersize. Proposed change would be to use WSAIoctl(FIONREAD) on Windows and getsockopt(SO_NREAD) on OSX, which returns the same value as on Linux (but arguably is a small change in behaviour, OTOH the old behaviour is broken IMHO).
Created attachment 227139 [details] [review] Return correct value for g_socket_get_available_bytes() on Windows and OSX
Review of attachment 227139 [details] [review]: ::: gio/gsocket.c @@ +2398,3 @@ +#else + if (WSAIoctl (socket->priv->fd, FIONREAD, NULL, 0, &avail, sizeof (avail), 0, 0) == SOCKET_ERROR) + return -1; I'd prefer a cascade over a nested ifdef here: #if defined(...) #elif defined(...) #else #fi Can't really comment on the win32 code myself, other than the general comment that it would be great to have a test case that verifies this. Do we ?
I'll change the preprocessor directives before committing or when another change is required :) I should probably add that I didn't even test to compile the Windows/OSX changes because I don't have Windows/OSX machines here. A testcase would be nice, yes, but should probably be done by someone who can actually test this.
Technically the Windows/OS X behavior matches the g_socket_get_available_bytes() documentation better, but the Linux behavior is clearly more useful, so... patch is good but you should also fix the documentation to describe the actual (new) behavior better. (In reply to comment #3) > A testcase would be nice, yes, but should probably be done by someone who can > actually test this. You can test it under unix at least and then someone else can run it under the other OSes.
Created attachment 228553 [details] [review] Return correct value for g_socket_get_available_bytes() on Windows and OSX
Created attachment 228555 [details] [review] Return correct value for g_socket_get_available_bytes() on Windows and OSX
This should address all the comments and also adds a test for this.
this doesn't even compile on windows...
And it doesn't do what it's supposed to either. The WSAIoctl docs say: If the socket passed in the s parameter is message oriented (for example, type SOCK_DGRAM), FIONREAD returns the reports the total number of bytes available to read, not the size of the first datagram (message) queued on the socket. ie, exactly the same as ioctlsocket. (Note that it does *appear* to work under Wine [with either WSAIoctl or ioctlsocket], but that's because Wine just proxies the winsock call to the corresponding linux ioctl, so it gets the linux behavior.) I've reverted the patch for now, although it made it into 2.34.2 (which means that 2.34.2 doesn't compile on Windows, so you might want to do a 2.34.2.1, Matthias).
(In reply to comment #8) > this doesn't even compile on windows... Yeah as said above I didn't try to compile it on Windows/OSX because I have no access to any of those. I'm a bit surprised that it immediately went to the stable branch though Will try to get the correct way of doing it on Windows
2.34.2 does not compiles on MacOS (10.8) either. I don't know glib but it seems obvious why, doesn't it ? Under MacOS, G_OS_WIN32 is not defined but SO_NREAD is, right ? > gssize > g_socket_get_available_bytes (GSocket *socket) > { > #ifndef G_OS_WIN32 > gulong avail = 0; > #else > gint avail = 0; > gsize avail_len = sizeof (avail); > #endif So avail_len is not defined > g_return_val_if_fail (G_IS_SOCKET (socket), -1); > #if defined(G_OS_WIN32) > if (WSAIoctl (socket->priv->fd, FIONREAD, NULL, 0, &avail, sizeof (avail), 0, 0) == SOCKET_ERROR) > return -1; > #elif defined(SO_NREAD) > if (getsockopt (socket->priv->fd, SOL_SOCKET, SO_NREAD, &avail, &avail_len) < 0) > return -1; > #else but used. > if (ioctl (socket->priv->fd, FIONREAD, &avail) < 0) > return -1; > #endif > return avail; > }
Here's the patch I used in MacPorts to build glib 2.34.2: https://trac.macports.org/browser/trunk/dports/devel/glib2/files/patch-gio_gsocket.c.diff?rev=99676
(In reply to comment #10) > Will try to get the correct way of doing it on Windows It appears that the only way to do it is to do a recv with MSG_PEEK into a 64k buffer. (64k being the largest possible size for a single UDP packet, so you know that's enough that you won't end up dropping any data.) Since you don't actually care about the contents of the buffer after the recv, you could use a single global buffer (rather than needing one per thread). It would probably make sense to malloc it when needed (with g_once) rather than having a static 64k buffer. And of course, this is only needed for UDP sockets; FIONREAD will do the expected thing for TCP sockets. This is gross, and the docs should probably note that this function will do gross things on some platforms with UDP sockets, with a recommendation that if you know a (reasonable) upper bound on the packet size, you should just recv into a buffer of that size rather than using get_available_bytes().
Comment #12 allows me to compile on OS X 10.6, but the /socket/datagram_get_available self-test fails: /socket/datagram_get_available: ** GLib-GIO:ERROR:socket.c:853:test_datagram_get_available: assertion failed (g_socket_get_available_bytes (server) == sizeof (data)): (0 == 17) FAIL GTester: last random seed: R02S0ce15129c62049d5cab3ef35fae5f744 Same result whether I use 32-bit or 64-bit mode (-arch i386 or -arch x86_64 compiler flags, respectively).
Pushed an updated patch that's tested on linux, os x, and cygwin