Bug 24775

Created attachment 39856 [details] [review]
missing implementation

Here's a patch that implements get_local_interfaces_ips() for Windows using  ioctlsocket() and SIO_GET_INTERFACE_LIST.

Review of attachment 39856 [details] [review]:

Thanks, this is superb! Some nitpicking on the implementation:

::: src/bytestream-socks5.c
@@ +1650,3 @@
 {
+  gint sockfd;
+  INTERFACE_INFO iflist[32];

MSDN says the ioctl fails with error WSAEFAULT if the buffer is too small. Would it be worth allocating this array dynamically, catching WSAEFAULT, and retrying with a larger buffer until it works? (Perhaps not.)

@@ +1653,3 @@
+  gulong bytes;
+  gint num = 0;
+  gint i;

I'd prefer these to have an unsigned type that's at least as large as bytes (gsize is guaranteed to be large enough for anything that'll fit in memory, so use that?)

@@ +1664,3 @@
+    }
+
+  if (WSAIoctl (sockfd, SIO_GET_INTERFACE_LIST, 0, 0, &iflist,

We prefer to spell null pointers as NULL, so the first 0 here should be NULL.

@@ +1665,3 @@
+
+  if (WSAIoctl (sockfd, SIO_GET_INTERFACE_LIST, 0, 0, &iflist,
+                sizeof (iflist), &bytes, 0, 0) == SOCKET_ERROR)

This parameter is documented as a LPDWORD, so you should declare 'bytes' as having type DWORD, unless DWORD is guaranteed by Windows to be the same thing as "unsigned long" in all versions and platforms.

If I'm reading MSDN correctly, these two 0 parameters would be better as NULL.

@@ +1674,3 @@
+
+  /* Loop throught the interface list and get the IP address of each IF */
+  for (i=0; i<num; ++i)

Our normal coding style would be (i = 0; i < num; i++)

(In reply to comment #2)

> MSDN says the ioctl fails with error WSAEFAULT if the buffer is too small.
> Would it be worth allocating this array dynamically, catching WSAEFAULT, and
> retrying with a larger buffer until it works? (Perhaps not.)

Sounds reasonable. I've implemented it in a way pretty similar to the non-HAVE_GETIFADDRS version of get_local_interfaces_ips().

> I'd prefer these to have an unsigned type that's at least as large as bytes
> (gsize is guaranteed to be large enough for anything that'll fit in memory, so
> use that?)

Done

> We prefer to spell null pointers as NULL, so the first 0 here should be NULL.

Ok, fixed

> +  if (WSAIoctl (sockfd, SIO_GET_INTERFACE_LIST, 0, 0, &iflist,
> +                sizeof (iflist), &bytes, 0, 0) == SOCKET_ERROR)
> 
> This parameter is documented as a LPDWORD, so you should declare 'bytes' as
> having type DWORD, unless DWORD is guaranteed by Windows to be the same thing
> as "unsigned long" in all versions and platforms.

DWORD is always defined as "unsigned long", see http://msdn.microsoft.com/en-us/library/aa383751%28VS.85%29.aspx

> If I'm reading MSDN correctly, these two 0 parameters would be better as NULL.

Yes you're right, fixed

> Our normal coding style would be (i = 0; i < num; i++)

Oops, it takes some time to get used to a different coding style.

Created attachment 39923 [details] [review]
revised patch

(In reply to comment #4)
> revised patch

Thanks, this looks good.

One remaining issue was that the commit message "fix fd.o#24775" wasn't ideal. I've changed it to "fd.o#24775: implement get_local_interfaces_ips() for Windows" and committed it.

Fixed in git for 0.10.4 and 0.11.0, which I'll hopefully release today.