Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tcp: add flags argument to uv_tcp_bind[6]() #749

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

indutny commented Mar 18, 2013

Possible flag values 0 or UV_TCP_IPV6ONLY.

This is important for https://github.com/indutny/node/compare/feature-dualstack. And this PR requires changes on windows side.

/cc @bnoordhuis @piscisaureus.

Contributor

indutny commented Mar 18, 2013

Probably, it should be the other way there. Instead of providing flag to disable dual stack, we should provide flag to enable it.

Contributor

indutny commented Mar 19, 2013

@bnoordhuis updated.

@indutny indutny referenced this pull request in nodejs/node-v0.x-archive Mar 19, 2013

Closed

Dual stack for node.js #5087

@tjfontaine tjfontaine commented on an outdated diff Apr 11, 2013

src/win/tcp.c
@@ -266,6 +266,19 @@ static int uv__bind(uv_tcp_t* handle,
}
}
+#ifdef IPV6_V6ONLY
+ if (family == AF_INET6) {
+ int on;
+
+ on = (tcp->flags & UV_HANDLE_TCP_DUALSTACK) ? 0 : 1;
@tjfontaine

tjfontaine Apr 11, 2013

Contributor

src\win\tcp.c(273): error C2065: 'tcp' : undeclared identifier [g:\jenkins\workspace\libuv-pullrequest\label\windows\libuv.vcxproj]

Contributor

saghul commented Dec 20, 2013

@indutny is this still relevant?

Contributor

txdv commented Dec 20, 2013

This patch might not, but generally Dual Stack is still relevant.

Contributor

saghul commented Dec 20, 2013

@txdv sure. I meant this patch, since it was partially reviewed, and looks like rebasing against current master wouldn't be much trouble.

Contributor

txdv commented Dec 20, 2013

This adds uv_tcp_dualstack(handle, enable) as a function while the udp variant is a flag set in uv_udp_bind (UV_UDP_IPV6ONLY).

I think the direction of making it an optional dualstack instead of optional ipv6only is right, but we need a consistent API.

Contributor

indutny commented Dec 20, 2013

@txdv I disagree, it should be optional ipv6only. This is a brave new world, man! Regarding this patch, I think it is still relevant. One small fix on windows side, renaming tcp to handle should make it work.

Contributor

txdv commented Dec 20, 2013

@indutny Is dualstack on by default or off by default? Because the name dualstack implies (to me personally) that it is always off and you have to turn it on.

Contributor

saghul commented Dec 20, 2013

@indutny API-wise, wouldn't it make sense to have:

int uv_tcp_bind(uv_tcp_t* handle const struct sockaddr* addr, unsigned int flags)

like @txdv suggested? So, we'd be dualstack by default, unless the flags indicate that IPv6-only should be used.

Contributor

indutny commented Dec 20, 2013

@saghul agree

Contributor

txdv commented Dec 21, 2013

NO! I suggest that dual stack is off by default and has to be turned on with a UV_DUALSTACK const. Well, I actually don't care as long as it gets into the lib.

@indutny indutny tcp: uv_tcp_dualstack()
Explicitly disable/enable dualstack depending on presence of flag set by
uv_tcp_dualstack() function.
c17af3e
Contributor

indutny commented Dec 25, 2013

@saghul @txdv force pushed branch!

Contributor

txdv commented Dec 25, 2013

Looks like a mirror version of udp so looks good to me.

Contributor

txdv commented Dec 25, 2013

Maybe rename the title of the commit, because there is no uv_tcp_dualstack anymore.

Contributor

indutny commented Dec 25, 2013

Right, will do right before landing it.

@saghul saghul commented on the diff Dec 27, 2013

include/uv.h
@@ -783,6 +783,11 @@ UV_EXTERN int uv_tcp_keepalive(uv_tcp_t* handle,
*/
UV_EXTERN int uv_tcp_simultaneous_accepts(uv_tcp_t* handle, int enable);
+enum uv_tcp_flags {
+ /* Disables dual stack mode. Used with uv_tcp_bind6(). */
@saghul

saghul Dec 27, 2013

Contributor

"Used with uv_tcp_bind, when an IPv6 address is used", uv_tcp_bind6 is no longer a thing :-)

Contributor

saghul commented Dec 27, 2013

LGTM, just a minor nit. We don't seem to have any test for this, since all tests seem to pass 0 as flags. Do you think we can have a test similar to test-udp-ipv6, which actually tests the functionality?

Contributor

indutny commented Jan 5, 2014

@saghul sounds like a plan.

Contributor

indutny commented Jan 20, 2014

Landed in 8f15aae , waiting for @txdv to help me with tests! ;)

@swishy swishy pushed a commit to swishy/camin that referenced this pull request Feb 11, 2014

Dale Anderson Fix error due to libuv API change ( see joyent/libuv#749 ) , this sho…
…uld now build against shipped builds also ( ie ubuntu packaged version ). start setting eo_error rather than useless logging, Add TODO to XML_SAX_BASE.
bf5b3b2
Contributor

saghul commented Mar 3, 2014

Closing since this was already merged, I created #1176, when someone has time a test would be appreciated :-)

@saghul saghul closed this Mar 3, 2014

So I started working on #1176, but ended up being a bit confused about this pull request. As I understand, for bound sockets, IPV6_V6ONLY disables the functionality of accepting IPv4 clients. However, we don't seem to provide functionality to disable connecting to IPv4 endpoints (done with the same flag), as we only accept the UV_TCP_IPV6ONLY flag in uv_tcp_bind.

Is this a valid concern? If so, should we perhaps expose a uv_tcp_ipv6only function, similar to how we do with uv_tcp_nodelay?

@indutny indutny deleted the unknown repository branch Jul 31, 2014

Contributor

saghul commented Jul 31, 2014

@mmalecki if you bind using that flag and then try to connect to an IPv4 address you should get an error. Are you observing otherwise?

Contributor

saghul commented Jul 31, 2014

Also, not sure how much having a toggleable would help, given: http://stackoverflow.com/questions/5587935/cant-turn-off-socket-option-ipv6-v6only

@saghul hm, I haven't tested it like that because it seemed counter-intuitive to call bind when connecting. I'll verify that this works and use it in the test then, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment