Re: PATCH] ws2_32: Allow user to enable IP dual stack.

[Bruno Jesus]

On Mon, Jul 4, 2016 at 6:35 AM, Matthieu Nottale
 wrote:
> IP dual stack (v4+v6) should be disabled by default, but previous code
> was setting IPV6_V6ONLY in bind() which prevented user to override it.
> This patch moves setting IPV6_V6ONLY to socket creation time.

Hi, thanks for the updated version. I believe the patch failed to
apply because you added text after it and also an attachment which is
inlined while the email is being parsed. The result you can see at
[1].

The subject should show how many attempts the patch was sent to, so
next subject should be:
[PATCH] ws2_32: Allow user to enable IP dual stack (v3)

Please see some nitpicks below.

[1] http://source.winehq.org/patches/data/123921
> Signed-off-by: Matthieu Nottale
>
> dlls/ws2_32/socket.c     | 22 ++++++++--------------
>  dlls/ws2_32/tests/sock.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
> index b0af3d7..c29bd2b 100644
> --- a/dlls/ws2_32/socket.c
> +++ b/dlls/ws2_32/socket.c
> @@ -3240,20 +3240,6 @@ int WINAPI WS_bind(SOCKET s, const struct
> WS_sockaddr* name, int namelen)
>              }
>              else
>              {
> -#ifdef IPV6_V6ONLY
> -
>            const struct sockaddr_in6 *in6 = (const struct
>
> sockaddr_in6*) &uaddr;
> -
>            if (name->sa_family == WS_AF_INET6 &&
> -
>                !memcmp(&in6->sin6_addr, &in6addr_any, sizeof(struct
>
> in6_addr)))
> -
>            {
> -
>                int enable = 1;
> -
>                if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable,
>
> sizeof(enable)) == -1)
> -
>                {
> -
>                    release_sock_fd( s, fd );
> -
>                    SetLastError(WSAEAFNOSUPPORT);
> -
>                    return SOCKET_ERROR;
> -
>                }
> -
>            }
>
> -#endif
>                  if (name->sa_family == WS_AF_INET)
>                  {
>                      struct sockaddr_in *in4 = (struct sockaddr_in*) &uaddr;
> @@ -7163,6 +7149,14 @@ SOCKET WINAPI WSASocketW(int af, int type, int
> protocol,
>          TRACE("\tcreated %04lx\n", ret );
>          if (ipxptype > 0)
>              set_ipx_packettype(ret, ipxptype);
> +#ifdef IPV6_V6ONLY
> -
>    if (unixaf == AF_INET6)
> -
>    {
> -
>      /* Winsock documentation stipulates that IPV6_V6ONLY is enabled
>
> by default*/

The documentation is not always useful (and sometimes wrong) and now
we should have a test to back this up, I believe a better comment to
be:
/* AF_INET6 sockets have IPV6_V6ONLY enabled by default. */

Watch out for missing space between the last comment word and the */
> -
>      int enable = 1;
> -
>      WS_setsockopt(ret, WS_IPPROTO_IPV6, WS_IPV6_V6ONLY, &enable,
>
> sizeof(enable));
> -
>    }
>
> +#endif
>         return ret;
>      }
>
> diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
> index 6853279..7c8d834 100644
> --- a/dlls/ws2_32/tests/sock.c
> +++ b/dlls/ws2_32/tests/sock.c
> @@ -6114,6 +6114,46 @@ static void test_ipv6only(void)
>      ok(!ret, "Could not bind IPv4 address (LastError: %d; %d expected if
> IPv6 binds to IPv4 as well).\n",
>          WSAGetLastError(), WSAEADDRINUSE);
> - if (v4 != INVALID_SOCKET)
> -
>    closesocket(v4);
> - if (v6 != INVALID_SOCKET)
> -
>    closesocket(v6);
> -
> - /* Test again, this time disabling V6ONLY*/
> - v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP);
> - if (v6 == INVALID_SOCKET) {
> -
>    skip("Could not create IPv6 socket (LastError: %d; %d expected if
>
> IPv6 not available).\n",
> -
>        WSAGetLastError(), WSAEAFNOSUPPORT);
> -
>    goto end;
> - }

Do a getsockopt at this point to check what is the default enabled
value. Set enabled to the opposite of what you expected before the
call and test its value after.
> - DWORD enabled = 0;
> - int len = sizeof(enabled);

You cannot declare variables like this in C89 (?) so do it in the
function beginning.
> - ret = setsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, &enabled, len);
> - if (ret) {
> -
>  ok(!ret, "Failed to disable IPV6_V6ONLY (LastError: %d).\n",
>
> WSAGetLastError());
> -
>  goto end;
> - }

I understand that you are trying to skip when IPV6_V6ONLY is not
present. It will be better if you catch the explicit error case and
bail out, something like:

WSASetLastError(0xdeadbeef);
    ret = setsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, &enabled, len);
    error = WSAGetLastError();
    if (ret) {
      ok(error == EXPECTED_ERROR, "Failed to disable IPV6_V6ONLY
(LastError %d, %d expected).\n", error, EXPECTED_ERROR);
      goto end;
    }
> ...

Best wishes,
Bruno