Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v5).

[Dmitry Timoshkov]

Roman Pišl <rpisl(a)seznam.cz> 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.
>
> V5 - fixes failing test on Windows
>
> Based on https://www.winehq.org/pipermail/wine-patches/2016-July/151919.html
>
> Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io>
> Signed-off-by: Roman Pišl <rpisl(a)seznam.cz>

Probably you shouldn't add someone's else sign-off without the author's
agreement, or mention that you've got an explicit agreement from the patch
author otherwise.
> +#ifdef IPV6_V6ONLY
> +        if (unixaf == AF_INET6)
> +        {
> +            int enable = 1;
> +            int fd = get_sock_fd(ret, 0, NULL);
> +            setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable, sizeof(enable));
> +        }
> +#endif

It's better to verify that get_sock_fd() succeded before using its result,
and call release_sock_fd() at the end.

-- 
Dmitry.

[PATCH] ws2_32: Allow user to enable IP dual stack (v5).

[Roman Pišl]

Ok, sorry, I wanted to give him a credit as my patch is only a fine-tuning of 
tests.

Is it really possible that get_sock_fd() can fail after a successfull 
create_socket request? What should be done if it fails, just skip setting the 
option?

Are these the only objections and should I send v6?

Thanks.

Roman

On Tuesday 18 of October 2016 19:30:53 Dmitry Timoshkov wrote:
> Roman Pišl <rpisl(a)seznam.cz> 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.
> >
> > V5 - fixes failing test on Windows
> >
> > Based on
> > https://www.winehq.org/pipermail/wine-patches/2016-July/151919.html
> >
> > Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io>
> > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz>
>
> Probably you shouldn't add someone's else sign-off without the author's
> agreement, or mention that you've got an explicit agreement from the patch
> author otherwise.
> > +#ifdef IPV6_V6ONLY
> > +        if (unixaf == AF_INET6)
> > +        {
> > +            int enable = 1;
> > +            int fd = get_sock_fd(ret, 0, NULL);
> > +            setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable,
> > sizeof(enable)); +        }
> > +#endif
>
> It's better to verify that get_sock_fd() succeded before using its result,
> and call release_sock_fd() at the end.

[PATCH] ws2_32: Allow user to enable IP dual stack (v5).

[Dmitry Timoshkov]

Roman Pišl <rpisl(a)seznam.cz> wrote:
> Ok, sorry, I wanted to give him a credit as my patch is only a fine-tuning of 
> tests.

Then it's probably better to add From: at the beginng of your patch with
the original author's name, and add your sign-off.
> Is it really possible that get_sock_fd() can fail after a successfull 
> create_socket request?

It's better to be on the safe side.
> What should be done if it fails, just skip setting the 
> option?

Probably. Have a look at other places that use this combo.
> Are these the only objections and should I send v6?

I have no objections other than the ones mentioned above.

-- 
Dmitry.