Re: [PATCH 1/1] ws2_32: Selectively bind UDP sockets to interfaces while still allowing broadcast packets (try 2, resend).

[Juan Lang]

Hi Erich,

+ * Bind the given socket exclusively to a specific interface.

I know the style around here is to comment minimally, but I didn't
find it clear what the behavior of this function is.  For one thing,

+static int interface_bind( int fd, struct sockaddr *addr )
(snip)
+    int ret = FALSE;
(snip)
+                ret = TRUE;

You're using #defines used with BOOL, so a BOOL return value would be
a little clearer IMO.  int, in the context of socket functions, often
uses 0 => success, -1 => failure.

Still, even with that change,

+    if (in_sock->sin_addr.s_addr == htonl(WS_INADDR_ANY))
+    {
+        /* Not binding to specific interface, use default route */
+        goto cleanup;
+    }
+    optlen = sizeof(sock_type);
+    if (getsockopt(fd, SOL_SOCKET, SO_TYPE, &sock_type, &optlen) == -1
+        || sock_type != SOCK_DGRAM)
+    {
+        /* Not a UDP socket, interface-specific bind is irrelevant. */
+        goto cleanup;

These two blocks will use the default return value, FALSE, and it
wasn't clear to me from the function name or comment that it should
return FALSE in this case.  Not that doing so is incorrect:  just that
it isn't obvious that that's the way it should behave.  In this case,
I feel a comment indicating that the function returns TRUE iff the
function bound to a specific device, and that FALSE doesn't
necessarily mean failure, is merited.

Also,

+    /* Obtain the size of the IPv4 adapter list and allocate
structure memory */
+    if (GetAdaptersInfo(NULL, &adap_size) != ERROR_BUFFER_OVERFLOW)

Why do all this outside the #ifdef SO_BINDTODEVICE guard?  I'd rather
you just had a different implementation of the function when
SO_BINDTODEVICE isn't present that issues the warning and does nothing
else.

I'll let Alexandre comment on whether the approach itself is appropriate.

Thanks,
--Juan

[PATCH 1/1] ws2_32: Selectively bind UDP sockets to interfaces while still allowing broadcast packets (try 2, resend).

[Erich E. Hoover]

On Wed, Nov 16, 2011 at 10:14 AM, Juan Lang  wrote:
> I know the style around here is to comment minimally, but I didn't
> find it clear what the behavior of this function is.  ...

The intended behavior of the function is to take bind() calls on any
name corresponding to a local network adapter and restrict the given
socket to operating only on the specified interface.  If the function
succeeds then the name for the bind() is changed to INADDR_ANY in
order to permit the receipt of broadcast sockets on the interface.
This behavior is only relevant to UDP sockets and is needed for
applications that expect to receive broadcast packets on a socket that
is bound to a specific interface.  Does that help?
> ...
> I feel a comment indicating that the function returns TRUE iff the
> function bound to a specific device, and that FALSE doesn't
> necessarily mean failure, is merited.

Fair enough, I think that by better describing the operation of the
function (like my explanation above) then that will be made clear.
> ...
> Why do all this outside the #ifdef SO_BINDTODEVICE guard?  I'd rather
> you just had a different implementation of the function when
> SO_BINDTODEVICE isn't present that issues the warning and does nothing
> else.

The idea was to only present the warning in a case where the passed
address would actually trigger an interface-specific bind.  I can
easily move the guard outside of the GetAdaptersInfo portion, but I
think at the very least the warning should only occur if the socket is
of type SOCK_DGRAM and the name is not INADDR_ANY.
> I'll let Alexandre comment on whether the approach itself is appropriate.

I believe I've addressed all the concerns he expressed with my
IP_PKTINFO approach, so I'm really hoping he'll like this approach.

Erich Hoover
[e-mail]