win: map 0.0.0.0 and :: addresses to localhost by bzoz · Pull Request #1515 · libuv/libuv

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

+      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

+    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 = (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

+
+  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

+    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 = (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

+    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

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


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

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


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

+
+  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


    :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

 }
+
+int uv__convert_to_localhost_if_unspecified(const struct sockaddr* addr,
+	                                          struct sockaddr_storage* storage) {


Indent error.

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

+    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

+    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

+    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

+    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

+    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

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


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

+  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

  }
 }
+
+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

    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


    :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


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

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

Conversation

bzoz commented Aug 31, 2017

Uh oh!

bzoz commented Aug 31, 2017

Uh oh!

bzoz commented Aug 31, 2017

Uh oh!

refack commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saghul commented Sep 2, 2017

Uh oh!

saghul commented Sep 2, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Sep 4, 2017

Uh oh!

bzoz commented Sep 5, 2017

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Sep 5, 2017

Uh oh!

bzoz commented Sep 14, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Sep 18, 2017

Uh oh!

bzoz commented Sep 28, 2017

Uh oh!

bzoz commented Oct 12, 2017

Uh oh!

bzoz commented Oct 19, 2017

refack commented Aug 31, 2017 •

edited

Loading

imran-iq Oct 23, 2017 •

edited

Loading