win: map 0.0.0.0 and :: addresses to localhost #1515

bzoz · 2017-08-31T16:35:38Z

On Linux when connecting, IP addresses 0.0.0.0 and :: are automatically converted to localhost. This adds the same functionality to Windows.

Ref: nodejs/node#14900
Ref: nodejs/node#14111

On Linux when connecting IP addresses 0.0.0.0 and :: are automatically converted to localhost. This adds same functionality to Windows.

bzoz · 2017-08-31T16:36:31Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/419/

bzoz · 2017-08-31T16:58:37Z

Another run, with correct Makefile: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/420/

refack · 2017-08-31T19:15:21Z

Ideally I'd go the other way and throw on connect('0.0.0.0') on POSIX as well, since it literally reads as "connect to wherever", but I guess that will cause too much breakage...

So I'm +1 on this

saghul · 2017-09-02T08:32:26Z

Some bots had failed with Jenkins issues, I retriggered the build: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/421/

saghul · 2017-09-02T08:32:44Z

I'm +1 on this, as long as all Unixen behave like Linux.

bnoordhuis

I don't know if we should adopt this behavior on Windows because I don't know if it is universal across Unices.

bnoordhuis · 2017-09-03T08:36:24Z

src/win/winsock.c

+      uv_inet_pton(AF_INET6, "::1", &(dest->sin6_addr));
+      addr = (const struct sockaddr*) dest;
+    }
+  }


} else { abort(); // or: // return UV_EINVAL; }

There is no need for this function to return a pointer, it could return an int. In fact, I'd argue it somewhat obscures the fact that the second argument is used as an out parameter.

bnoordhuis · 2017-09-03T08:43:45Z

src/win/winsock.c

+    dest = (struct sockaddr_in*) storage;
+    if (src->sin_addr.S_un.S_addr == 0) {
+      memcpy(dest, src, sizeof(struct sockaddr_in));
+      uv_inet_pton(AF_INET, "127.0.0.1", &(dest->sin_addr.s_addr));


Superfluous parens. Ditto a few lines below.

bnoordhuis · 2017-09-03T08:45:44Z

src/win/winsock.c

+    src = (const struct sockaddr_in6*) addr;
+    dest = (struct sockaddr_in6*) storage;
+    if (memcmp(src->sin6_addr.u.Byte,
+               uv_addr_ip6_any_.sin6_addr.u.Byte,


Just &src->sin6_addr and &uv_addr_ip6_any_.sin6_addr? Or is there some subtlety I'm missing?

bnoordhuis · 2017-09-03T08:46:00Z

test/test-connect-unspecified.c

+
+  uv_tcp_init(loop, &socket4);
+  uv_ip4_addr("0.0.0.0", 80, &addr4);
+  uv_tcp_connect(&connect4, &socket4, (const struct sockaddr*)&addr4, connect_4);


bzoz · 2017-09-04T14:22:47Z

Updated, PTAL. New CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/434/

bzoz · 2017-09-05T09:18:48Z

The failures are unrelated, and the automatic mapping of 0.0.0.0 to localhost works on all tested platforms.

saghul

I'd say this needs a mention in the docs.

saghul · 2017-09-05T11:34:39Z

src/win/winsock.c

+    struct sockaddr_in* dest;
+    src = (const struct sockaddr_in*) addr;
+    dest = (struct sockaddr_in*) storage;
+	  memcpy(dest, src, sizeof(struct sockaddr_in));


style: incorrect indent

saghul · 2017-09-05T11:35:16Z

src/win/winsock.c

+    src = (const struct sockaddr_in*) addr;
+    dest = (struct sockaddr_in*) storage;
+	  memcpy(dest, src, sizeof(struct sockaddr_in));
+    if (src->sin_addr.S_un.S_addr == 0) {


incorrect capitalization?

What do you mean?

src->sin_addr.s_addr intead of the union.

saghul · 2017-09-05T11:35:26Z

src/win/winsock.c

+    dest = (struct sockaddr_in6*) storage;
+    memcpy(dest, src, sizeof(struct sockaddr_in6));
+    if (memcmp(&src->sin6_addr,
+		       &uv_addr_ip6_any_.sin6_addr,


style: indent

saghul · 2017-09-05T11:35:32Z

src/win/winsock.c

+		       sizeof(struct in6_addr)) == 0) {
+      uv_inet_pton(AF_INET6, "::1", &dest->sin6_addr);
+    }
+	  return 0;


saghul · 2017-09-05T11:35:37Z

src/win/winsock.c

+    }
+	  return 0;
+  } else {
+	  return UV_EINVAL;


saghul · 2017-09-05T11:36:03Z

test/test-connect-unspecified.c

+
+  loop = uv_default_loop();
+
+  uv_tcp_init(loop, &socket4);


assert for error in all these

bzoz · 2017-09-05T18:29:43Z

Updated, PTAL.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/442/

bzoz · 2017-09-14T08:08:35Z

ping?

bnoordhuis

Left some comments. I still don't know if this is something we should actually do.

bnoordhuis · 2017-09-14T18:54:45Z

docs/src/udp.rst

@@ -259,6 +263,8 @@ API

    :returns: 0 on success, or an error code < 0 on failure.

+    .. versionchanged:: 1.15.0 added ``0.0.0.0`` to ``localhost`` mapping


Also ::, right?

bnoordhuis · 2017-09-14T18:55:51Z

src/win/winsock.c

@@ -559,3 +559,34 @@ int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info_in,
    return SOCKET_ERROR;
  }
 }
+
+int uv__convert_to_localhost_if_unspecified(const struct sockaddr* addr,
+	                                          struct sockaddr_storage* storage) {


Indent error.

bnoordhuis · 2017-09-14T18:57:07Z

src/win/winsock.c

+    struct sockaddr_in* dest;
+    src = (const struct sockaddr_in*) addr;
+    dest = (struct sockaddr_in*) storage;
+    memcpy(dest, src, sizeof(struct sockaddr_in));


sizeof(*src) or sizeof(dest).

bnoordhuis · 2017-09-14T18:59:42Z

src/win/winsock.c

+    dest = (struct sockaddr_in*) storage;
+    memcpy(dest, src, sizeof(struct sockaddr_in));
+    if (src->sin_addr.s_addr == 0) {
+      uv_inet_pton(AF_INET, "127.0.0.1", &dest->sin_addr.s_addr);


dest->sin_addr.s_addr = INADDR_LOOPBACK;? Or doesn't that exist on Windows? You could use htonl(0x7F000001) instead.

bnoordhuis · 2017-09-14T19:00:32Z

src/win/winsock.c

+    struct sockaddr_in6* dest;
+    src = (const struct sockaddr_in6*) addr;
+    dest = (struct sockaddr_in6*) storage;
+    memcpy(dest, src, sizeof(struct sockaddr_in6));


sizeof(*src) or sizeof(dest).

bnoordhuis · 2017-09-14T19:00:40Z

src/win/winsock.c

+    memcpy(dest, src, sizeof(struct sockaddr_in6));
+    if (memcmp(&src->sin6_addr,
+               &uv_addr_ip6_any_.sin6_addr,
+               sizeof(struct in6_addr)) == 0) {


bnoordhuis · 2017-09-14T19:03:30Z

src/win/winsock.c

+    if (memcmp(&src->sin6_addr,
+               &uv_addr_ip6_any_.sin6_addr,
+               sizeof(struct in6_addr)) == 0) {
+      uv_inet_pton(AF_INET6, "::1", &dest->sin6_addr);


IN6ADDR_LOOPBACK_INIT or in6addr_loopback?

bnoordhuis · 2017-09-14T19:03:53Z

test/test-connect-unspecified.c

+#include "uv.h"
+#include "task.h"
+
+void connect_4(uv_connect_t* req, int status) {


bnoordhuis · 2017-09-14T19:03:59Z

test/test-connect-unspecified.c

+  ASSERT(status != UV_EADDRNOTAVAIL);
+}
+
+void connect_6(uv_connect_t* req, int status) {


bzoz · 2017-09-18T16:00:18Z

Updated, PTAL

bzoz · 2017-09-28T13:11:22Z

ping

bzoz · 2017-10-12T09:39:40Z

ping?

bzoz · 2017-10-19T13:02:23Z

ping?

@saghul @bnoordhuis

obsolete

bnoordhuis · 2017-10-19T20:03:32Z

@bzoz I've dismissed my review but like I said, I don't know if I agree that this is a change for the better. I won't block this PR but I can't say I wholeheartedly endorse it either.

@libuv/collaborators WDYT?

refack · 2017-10-19T22:48:48Z

I'm conflicted as well.
But it does bring uniformity, and enables the

server.bind(0, '0.0.0.0');
client.connect(server.port, server.address);

pattern which is useful for IPC (and testing which is also nice)...

imran-iq · 2017-10-23T21:03:47Z

src/win/winsock.c

@@ -559,3 +559,32 @@ int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info_in,
    return SOCKET_ERROR;
  }
 }
+
+int uv__convert_to_localhost_if_unspecified(const struct sockaddr* addr,


Couldnt this method could be cleaned up using a union and switch similar to the code found here?

bzoz · 2017-10-26T16:04:13Z

@imran-iq I've followed up on your suggestion, I think that indeed it looks simpler now.

PTAL

bzoz · 2017-10-26T16:04:25Z

new CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/538/

bzoz · 2017-10-27T11:00:01Z

Fixed the test, new CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/539/, PTAL

bzoz · 2017-11-03T10:25:59Z

@libuv/collaborators ping?

I know it might be controversial. But it eases out differences between platforms, and fixes real world use case.

CI, since last 404ed: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/555/

bzoz · 2017-12-21T11:15:48Z

ping?

bnoordhuis

I guess I'll sign off on this because it brings Windows closer to the out-of-the-box behavior on Linux.

bnoordhuis · 2017-12-21T12:13:25Z

docs/src/tcp.rst

    The callback is made when the connection has been established or when a
    connection error happened.

+    .. versionchanged:: 1.15.0 added ``0.0.0.0`` and ``::`` to ``localhost``


Version number needs an update.

bnoordhuis · 2017-12-21T12:13:32Z

docs/src/udp.rst

@@ -259,6 +263,9 @@ API

    :returns: 0 on success, or an error code < 0 on failure.

+    .. versionchanged:: 1.15.0 added ``0.0.0.0`` and ``::`` to ``localhost``


bzoz · 2017-12-28T09:42:06Z

CI after rebase: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/641

On Linux when connecting IP addresses 0.0.0.0 and :: are automatically converted to localhost. This adds same functionality to Windows. PR-URL: #1515 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bzoz · 2017-12-28T09:42:46Z

Landed in 2b32e77

bnoordhuis · 2017-12-28T10:36:40Z

Ho @bzoz, there's still a -1 from a collaborator. You'll have to revert this.

bzoz · 2017-12-28T11:01:39Z

@saghul commented he is generally +1 on the change and I've addressed his feedback here

bnoordhuis · 2017-12-28T11:11:46Z

@saghul Can you update your review if you're okay with this?

@bzoz As long as there's a negative review, you can't merge it.

bzoz · 2017-12-28T11:19:06Z

Should I make a PR for revert or just push a revert commit?

Also, @saghul did not respond to my pings for some time now, how can I get this merged then?

win: map 0.0.0.0 and :: addresses to localhost

a50defc

On Linux when connecting IP addresses 0.0.0.0 and :: are automatically converted to localhost. This adds same functionality to Windows.

fixup: correct order in uv.gyp, add to Makefile.am

219c262

bnoordhuis reviewed Sep 3, 2017

View reviewed changes

fixup: return int, style

69807ae

saghul requested changes Sep 5, 2017

View reviewed changes

fixup: nits, style, bug in test, docs

093ff55

bzoz added 2 commits September 6, 2017 14:06

fixup: use s_addr alias

3eb19c0

fixup: style

1a780f3

bnoordhuis previously requested changes Sep 14, 2017

View reviewed changes

fixup

9bfc63d

imran-iq reviewed Oct 23, 2017

View reviewed changes

fixup: simplify, use TEST_PORT

4399ad9

fixup: use htonl with INADDR_LOOPBACK

35fa38f

bnoordhuis approved these changes Dec 21, 2017

View reviewed changes

fixup: update version

ff5ddc7

bzoz closed this Dec 28, 2017

		@@ -259,6 +263,8 @@ API

		:returns: 0 on success, or an error code < 0 on failure.

		.. versionchanged:: 1.15.0 added ``0.0.0.0`` to ``localhost`` mapping

		@@ -259,6 +263,9 @@ API

		:returns: 0 on success, or an error code < 0 on failure.

		.. versionchanged:: 1.15.0 added ``0.0.0.0`` and ``::`` to ``localhost``

win: map 0.0.0.0 and :: addresses to localhost #1515

win: map 0.0.0.0 and :: addresses to localhost #1515

Conversation

bzoz commented Aug 31, 2017

bzoz commented Aug 31, 2017

bzoz commented Aug 31, 2017

refack commented Aug 31, 2017 • edited Loading

saghul commented Sep 2, 2017

saghul commented Sep 2, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Sep 4, 2017

bzoz commented Sep 5, 2017

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Sep 5, 2017

bzoz commented Sep 14, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Sep 18, 2017

bzoz commented Sep 28, 2017

bzoz commented Oct 12, 2017

bzoz commented Oct 19, 2017

bnoordhuis commented Oct 19, 2017

refack commented Oct 19, 2017

imran-iq Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

bzoz commented Oct 26, 2017

bzoz commented Oct 26, 2017

bzoz commented Oct 27, 2017

bzoz commented Nov 3, 2017

bzoz commented Dec 21, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Dec 28, 2017

bzoz commented Dec 28, 2017

bnoordhuis commented Dec 28, 2017

bzoz commented Dec 28, 2017

bnoordhuis commented Dec 28, 2017

bzoz commented Dec 28, 2017

refack commented Aug 31, 2017 •

edited

Loading

imran-iq Oct 23, 2017 •

edited

Loading