unix, win: extend uv_udp_set_membership for IPv6 multicast. #692

Closed
wants to merge 1 commit into from

6 participants

@snoj

Added uv_udp_set_membership6.
Modified uv_udp_set_membership to call uv_udp_set_membership6
when IPv6 is used.

Google groups discussion...not much there mainly easy access for myself. https://groups.google.com/forum/?fromgroups=#!topic/libuv/TxZmEnH2wP4

@snoj

Ahhh shoot. Previous comments you guys put in are gone.

Anyway, made the changes that were recommended and refined the multicast binding. Now it doesn't send out on all interfaces if one is given. In order to do this though, I had to get the interface index which I did by extending uv_interface_address_s.

I'll start working on the Linux versions if there is nothing more.

@txdv

If you come to the irc channel right now, I will aid you in this task.

@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/udp.c
+ int interfaces_count;
+
+ if (!(handle->flags & UV_HANDLE_IPV6)) {
+ return uv__set_artificial_error(handle->loop, UV_EINVAL);
+ }
+
+ if (!(handle->flags & UV_HANDLE_BOUND) &&
+ uv_udp_bind6(handle, uv_addr_ip6_any_, 0) < 0) {
+ return -1;
+ }
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if(interface_addr) {

Style: space between keyword and paren.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/udp.c
+ }
+
+ if (!(handle->flags & UV_HANDLE_BOUND) &&
+ uv_udp_bind6(handle, uv_addr_ip6_any_, 0) < 0) {
+ return -1;
+ }
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if(interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {
+ uv__set_sys_error(handle->loop, WSAGetLastError());
+ return -1;

Fold into previous line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/udp.c
+ }
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if(interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {
+ uv__set_sys_error(handle->loop, WSAGetLastError());
+ return -1;
+ }
+
+ while(interfaces_count--) {
+ if(interfaces[interfaces_count].address.address4.sin_family == AF_INET6) {
+ if(memcmp(&interfaces[interfaces_count].address.address6.sin6_addr, &interface_addr_n, sizeof interface_addr_n) == 0) {

Long line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/udp.c
+ if (!(handle->flags & UV_HANDLE_IPV6)) {
+ return uv__set_artificial_error(handle->loop, UV_EINVAL);
+ }
+
+ if (!(handle->flags & UV_HANDLE_BOUND) &&
+ uv_udp_bind6(handle, uv_addr_ip6_any_, 0) < 0) {
+ return -1;
+ }
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if(interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {

Leaking interfaces?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/util.c
@@ -839,6 +839,9 @@ uv_err_t uv_interface_addresses(uv_interface_address_t** addresses_ptr,
win_address->FirstUnicastAddress == NULL)
continue;
+ /* Add four bytes for the interface index */
+ uv_address_buf_size += 4;

Use sizeof(fieldname) here.

By the way, is it necessary? It gets incremented with sizeof(uv_interface_address_t) further down below, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
src/win/util.c
@@ -901,6 +904,10 @@ uv_err_t uv_interface_addresses(uv_interface_address_t** addresses_ptr,
(int) max_name_size,
NULL,
FALSE);
+
+ uv_address->ifIndex = 0;
+ uv_address->ifIndex = (win_address->IfIndex) ? win_address->IfIndex : win_address->Ipv6IfIndex;

Superfluous assignment, superfluous parens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 26, 2013
@@ -1433,6 +1450,7 @@ struct uv_cpu_info_s {
struct uv_interface_address_s {
char* name;
int is_internal;
+ ULONG ifIndex;

Use snake_case, not camelCase.

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

Super special thanks to @txdv for fixing the bad code style!

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 27, 2013
src/unix/internal.h
@@ -244,3 +244,6 @@ static void uv__update_time(uv_loop_t* loop) {
}
#endif /* UV_UNIX_INTERNAL_H_ */
+
+/* */
+#define UV_HANDLE_IPV6 0x01000000

What's that doing there?

@snoj
snoj added a note Jan 27, 2013

A copy of UV_HANDLE_IPV6 from src/win/internal.h. It was otherwise not available on my linux build.

Okay, but look closely where you put it in src/unix/internal.h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
@@ -524,6 +525,11 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
int optname;
struct ip_mreq mreq;
+
+ if (handle->flags & UV_HANDLE_IPV6) {
+ return uv_udp_set_membership6(handle, multicast_addr, interface_addr, membership);

If we're doing an implicit call then uv_udp_set_membership6 doesn't have to be public (and should be renamed to uv__udp_set_membership6 to indicate it's internal.)

EDIT: it should probably be static as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
@@ -554,6 +560,69 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
return 0;
}
+int uv_udp_set_membership6(uv_udp_t* handle, const char* multicast_addr,
+ const char* interface_addr, uv_membership membership) {

Arguments should line up, i.e.:

int uv_udp_set_membership6(uv_udp_t* handle,
                           const char* multicast_addr,
                           const char* interface_addr,
                           uv_membership membership
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
@@ -554,6 +560,69 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
return 0;
}
+int uv_udp_set_membership6(uv_udp_t* handle, const char* multicast_addr,
+ const char* interface_addr, uv_membership membership) {
+
+ int optname;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+

Style: No newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+
+ uv_interface_address_t* interfaces;
+ int interfaces_count;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {
+ uv__set_sys_error(handle->loop, UV_EINVAL);
+ return -1;

Style: fold into a single-line statement (return uv__set_sys_error(handle->loop, UV_EINVAL);) and drop the braces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
+
+ uv_interface_address_t* interfaces;
+ int interfaces_count;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {
+ uv__set_sys_error(handle->loop, UV_EINVAL);
+ return -1;
+ }
+
+ while (interfaces_count--) {

Micro-optimization and micro-style: use a for (i = 0; i < interfaces_count; i++) loop here.

Micro-optimization: CPU prefetchers are good at recognizing memory access patterns in ascending order but generally not when they're in descending order.

Micro-style: Prefer not mutating variables. It's a kind of future-proofing; if someone changes the code later on and assumes interfaces_count is factual, things will go badly wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
+ int interfaces_count;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count).code != UV_OK) {
+ uv__set_sys_error(handle->loop, UV_EINVAL);
+ return -1;
+ }
+
+ while (interfaces_count--) {
+ if (interfaces[interfaces_count].address.address4.sin_family == AF_INET6) {
+ if (memcmp(&interfaces[interfaces_count].address.address6.sin6_addr, &interface_addr_n, sizeof interface_addr_n) == 0) {

Style: Long line. Wrap at 80 characters max. I believe I pointed that out last night too. :-/

Also, using memcmp() here is potentially wrong. The struct may have holes in it that contain garbage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
src/unix/udp.c
+int uv_udp_set_membership6(uv_udp_t* handle, const char* multicast_addr,
+ const char* interface_addr, uv_membership membership) {
+
+ int optname;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+
+ uv_interface_address_t* interfaces;
+ int interfaces_count;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr) {

Style: interface_addr != NULL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 27, 2013
src/unix/udp.c
+
+ mreq.ipv6mr_multiaddr = multicast_addr_n;
+
+ switch (membership) {
+ case UV_JOIN_GROUP:
+ optname = IPV6_ADD_MEMBERSHIP;
+ break;
+ case UV_LEAVE_GROUP:
+ optname = IPV6_DROP_MEMBERSHIP;
+ break;
+ default:
+ uv__set_sys_error(handle->loop, UV_EINVAL);
+ return -1;
+ }
+
+ if (setsockopt(handle->io_watcher.fd, IPPROTO_IPV6, optname, (void*) &mreq, sizeof mreq) == -1) {

Superfluous cast to void*.

@txdv
txdv added a note Jan 27, 2013
  • space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Jan 27, 2013
test/test-udp-multicast-join6.c
+#include <stdlib.h>
+#include <string.h>
+
+#define CHECK_HANDLE(handle) \
+ ASSERT((uv_udp_t*)(handle) == &server || (uv_udp_t*)(handle) == &client)
+
+static uv_udp_t server;
+static uv_udp_t client;
+
+static int cl_recv_cb_called;
+
+static int sv_send_cb_called;
+
+static int close_cb_called;
+
+static uv_buf_t alloc_cb(uv_handle_t* handle, size_t suggested_size) {

Style: Two newlines between declarations and functions.

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

I believe the new forced commit takes care of most of the comments. I couldn't figure out a way compare ip addresses without more variables and conversions, perhaps I'll need to resort to that that.

@diversario

I've tried to use multicast with IPv6 address but I keep getting addMembership EINVAL. Does this PR address that?

@diversario diversario referenced this pull request in diversario/node-disco Jul 9, 2013
Closed

Consider using IPv6 #10

@snoj

@diversario, When I started this, node/libuv did not support ipv6 multicast. I don't recall that EINVAL (see google group link at the top), but they may have changed the error codes since.

Since you're trying to use ipv6 multicasting, I think something like this would work for you. This code probably several version behind by now though. I or someone would need to review it.

@saghul
Joyent member

Sorry this was left hanging. Can yoou please rebase it against current master?

@indutny indutny commented on an outdated diff Dec 18, 2013
src/unix/linux/linux-core.c
@@ -717,6 +717,7 @@ uv_err_t uv_interface_addresses(uv_interface_address_t** addresses,
#else
struct ifaddrs *addrs, *ent;
char ip[INET6_ADDRSTRLEN];
+
@indutny
indutny added a note Dec 18, 2013

Unnecessary whitespace change.

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

@saghul I'll attempt to rebase and run tests by sometime this coming weekend. But with it being christmas and with all the family get togethers, I can't guarantee anything.

@saghul
Joyent member
@snoj

Definitely not happening this weekend. To many changes have happened since writing this.

@saghul
Joyent member
@mprokopowicz

Any progress on this? Can't wait for got this in node release.
It's critical for my project :)

@snoj

Been working the last couple/few days with @mprokopowicz on this. Appears to be working, but when I rebased on the latest master, node broke. Tonight I'm hoping to rebase on the version node is using and going from there.

@snoj snoj unix, windows: extend uv_udp_set_membership for IPv6 multicast.
Add uv__udp_set_membership6.
Modify uv_udp_set_membership to call uv__udp_set_membership6
  when IPv6 is used.
Add if_index to struct uv_interface_address_s for IPv6 multicasting.
Add UV_HANDLE_IPV6 to unix/interna.h

Signed-off-by: snoj <josh@snoj.us>
3c711be
@snoj

Done? Code compiles and ipv6 multicast tests completes without errors.

!ping @saghul ?

@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
@@ -35,6 +35,10 @@
static void uv__udp_recvmsg(uv_loop_t* loop, uv__io_t* w, unsigned int revents);
static void uv__udp_sendmsg(uv_loop_t* loop, uv__io_t* w, unsigned int revents);
static int uv__udp_maybe_deferred_bind(uv_udp_t* handle, int domain);
+static int uv__udp_set_membership6(uv_udp_t* handle,
@indutny
indutny added a note Jan 26, 2014

Whitespace at the end of the line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
@@ -35,6 +35,10 @@
static void uv__udp_recvmsg(uv_loop_t* loop, uv__io_t* w, unsigned int revents);
static void uv__udp_sendmsg(uv_loop_t* loop, uv__io_t* w, unsigned int revents);
static int uv__udp_maybe_deferred_bind(uv_udp_t* handle, int domain);
+static int uv__udp_set_membership6(uv_udp_t* handle,
+ const char* multicast_addr,
+ const char* interface_addr,
@indutny
indutny added a note Jan 26, 2014

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
@@ -585,6 +585,87 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
}
+int uv_udp_set_membership6(uv_udp_t* handle,
+ const char* multicast_addr,
+ const char* interface_addr,
+ uv_membership membership) {
+ int optname;
+ int interfaces_count;
+ int i;
+ int err;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+ uv_interface_address_t* interfaces;
+
@indutny
indutny added a note Jan 26, 2014

Ditto

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

Got this on OSX:

../../src/unix/udp.c:549:15: error: use of undeclared identifier 'IPV6_ADD_MEMBERSHIP'
    optname = IPV6_ADD_MEMBERSHIP;
              ^
../../src/unix/udp.c:552:15: error: use of undeclared identifier 'IPV6_DROP_MEMBERSHIP'
    optname = IPV6_DROP_MEMBERSHIP;
              ^
@indutny

Perhaps it is called IPV6_JOIN_GROUP and IPV6_LEAVE_GROUP on osx?

@indutny

I'd suggest adding UV__IPV6_JOIN_GROUP and UV__IPV6_LEAVE_GROUP in src/unix/internal.h and setting them to the appropriate value

@indutny indutny commented on the diff Jan 26, 2014
test/test-udp-multicast-join6.c
+ ASSERT(!memcmp("PING", buf->base, nread));
+
+ /* we are done with the client handle, we can close it */
+ uv_close((uv_handle_t*) &client, close_cb);
+}
+
+
+TEST_IMPL(udp_multicast_join6) {
+ int r;
+ uv_udp_send_t req;
+ uv_buf_t buf;
+ struct sockaddr_in6 addr;
+
+ ASSERT(0 == uv_ip6_addr("::1", TEST_PORT, &addr));
+
+ r = uv_udp_init(uv_default_loop(), &server);
@indutny
indutny added a note Jan 26, 2014

Please do ASSERT(0 == ...) everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ int interfaces_count;
+ int i;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+ uv_interface_address_t* interfaces;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr != NULL) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count) != 0)
+ return -1;
+ /*return uv__set_sys_error(handle->loop, UV_EINVAL);*/
@indutny
indutny added a note Jan 26, 2014

This is a bad idea :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ int optname;
+ int interfaces_count;
+ int i;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+ uv_interface_address_t* interfaces;
+
+ memset(&mreq, 0, sizeof mreq);
+ memset(&multicast_addr_n, 0, sizeof multicast_addr_n);
+ memset(&interface_addr_n, 0, sizeof interface_addr_n);
+
+ if (interface_addr != NULL) {
+ uv_inet_pton(AF_INET6, interface_addr, &interface_addr_n);
+ if (uv_interface_addresses(&interfaces, &interfaces_count) != 0)
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Just return -errno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ /*return uv__set_sys_error(handle->loop, UV_EINVAL);*/
+
+ for (i = 0; i < interfaces_count; i++) {
+ if (interfaces[i].address.address6.sin6_family == AF_INET6) {
+ if (memcmp(&interfaces[i].address.address6.sin6_addr,
+ &interface_addr_n,
+ sizeof interface_addr_n) == 0) {
+ mreq.ipv6mr_interface = interfaces[i].if_index;
+ break;
+ }
+ }
+ }
+
+ if (mreq.ipv6mr_interface == 0) {
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ sizeof interface_addr_n) == 0) {
+ mreq.ipv6mr_interface = interfaces[i].if_index;
+ break;
+ }
+ }
+ }
+
+ if (mreq.ipv6mr_interface == 0) {
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
+ }
+ }
+
+ if (uv_inet_pton(AF_INET6, multicast_addr, &multicast_addr_n) != 0) {
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ if (memcmp(&interfaces[i].address.address6.sin6_addr,
+ &interface_addr_n,
+ sizeof interface_addr_n) == 0) {
+ mreq.ipv6mr_interface = interfaces[i].if_index;
+ break;
+ }
+ }
+ }
+
+ if (mreq.ipv6mr_interface == 0) {
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
+ }
+ }
+
+ if (uv_inet_pton(AF_INET6, multicast_addr, &multicast_addr_n) != 0) {
@indutny
indutny added a note Jan 26, 2014

And no need in braces if there will be one line

@indutny
indutny added a note Jan 26, 2014

Also in all other places in diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
+ }
+
+ mreq.ipv6mr_multiaddr = multicast_addr_n;
+
+ switch (membership) {
+ case UV_JOIN_GROUP:
+ optname = IPV6_ADD_MEMBERSHIP;
+ break;
+ case UV_LEAVE_GROUP:
+ optname = IPV6_DROP_MEMBERSHIP;
+ break;
+ default:
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Ditto

@indutny
indutny added a note Jan 26, 2014

Probably should be an assertion, or UV_ENOTSUP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/unix/udp.c
+
+ switch (membership) {
+ case UV_JOIN_GROUP:
+ optname = IPV6_ADD_MEMBERSHIP;
+ break;
+ case UV_LEAVE_GROUP:
+ optname = IPV6_DROP_MEMBERSHIP;
+ break;
+ default:
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
+ }
+
+ if (setsockopt(handle->io_watcher.fd, IPPROTO_IPV6, optname, &mreq, sizeof mreq) != 0) {
+ /*uv__set_artificial_error(handle->loop, UV_EINVAL);*/
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Dito

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
@@ -539,7 +539,7 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
struct ip_mreq mreq;
/* If the socket is unbound, bind to inaddr_any. */
- if (!(handle->flags & UV_HANDLE_BOUND)) {
+ if (!(handle->flags & UV_HANDLE_IPV6) && !(handle->flags & UV_HANDLE_BOUND)) {
@indutny
indutny added a note Jan 26, 2014

!(handle->flags & (UV_HANDLE_IPV6 | UV_HANDLE_BOUND))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
@@ -570,7 +570,7 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
optname = IP_DROP_MEMBERSHIP;
break;
default:
- return UV_EINVAL;
+ return UV__EINVAL;
@indutny
indutny added a note Jan 26, 2014

Why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
@@ -585,6 +585,87 @@ int uv_udp_set_membership(uv_udp_t* handle, const char* multicast_addr,
}
+int uv_udp_set_membership6(uv_udp_t* handle,
+ const char* multicast_addr,
+ const char* interface_addr,
+ uv_membership membership) {
+ int optname;
+ int interfaces_count;
+ int i;
+ int err;
+ struct ipv6_mreq mreq;
+ struct in6_addr multicast_addr_n;
+ struct in6_addr interface_addr_n;
+ uv_interface_address_t* interfaces;
+
+ if (!(handle->flags & UV_HANDLE_IPV6)) {
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
+ return uv_translate_sys_error(WSAGetLastError());
+ }
+
+ for (i = 0; i < interfaces_count; i++) {
+ if (interfaces[i].address.address6.sin6_family == AF_INET6) {
+ if (memcmp(&interfaces[i].address.address6.sin6_addr,
+ &interface_addr_n,
+ sizeof interface_addr_n) == 0) {
+ mreq.ipv6mr_interface = interfaces[i].if_index;
+ break;
+ }
+ }
+ }
+
+ if (mreq.ipv6mr_interface == 0) {
+ return -1;
@indutny
indutny added a note Jan 26, 2014

Should return some meaningful error here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 26, 2014
src/win/udp.c
+ }
+ if (uv_inet_pton(AF_INET6, multicast_addr, &multicast_addr_n) != 0) {
+ return uv_translate_sys_error(WSAGetLastError());
+ }
+
+ mreq.ipv6mr_multiaddr = multicast_addr_n;
+
+ switch (membership) {
+ case UV_JOIN_GROUP:
+ optname = IPV6_ADD_MEMBERSHIP;
+ break;
+ case UV_LEAVE_GROUP:
+ optname = IPV6_DROP_MEMBERSHIP;
+ break;
+ default:
+ return -1;
@indutny
indutny added a note Jan 26, 2014

And there

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

As another note, I do get EADDRNOTAVAIL on OSX and test is failing.

@indutny

Please fix all that things and I will finish reviewing

@indutny

And generally it looks quite neat ;)

@saghul
Joyent member

The UV_HANDLE_IPV6 flag is only used for UDP, maybe you could rename it to something like UV_UDP_IPV6?

@saghul
Joyent member

I'd name the extra field you added to uv_interface_address_s just "index". Also, AFAIS it's only set on Linux and Windows. Don't other platforms need it as well?

@saghul
Joyent member

Instead of branching out to a ipv6 version of the function and continuing on the same otherwise, I'd split uv_udp_set_membership into 2: uv__udp_set_membership4/6.

PS: sorry for the brevity, I'm on mobile.

@mprokopowicz

Hi,

i was mailing with snoj and unfortunately he won't have time to finish this commit. Is any one here who can handle it?

@saghul
Joyent member

I'll take over.

@mprokopowicz

@saghul Awsome news :) Can U provide any estimates? (No pressure, just curious... )

@saghul
Joyent member
@saghul
Joyent member

For those interested, I reworked the Unix part here: https://github.com/saghul/libuv/tree/set-membership-v6

I'll send a proper PR for review once I'm done with the Windows part.

@saghul
Joyent member

Superseded by #1202. Lets continue the discussion there.

@saghul saghul closed this Mar 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment