udp: add source-specific multicast support by falanger · Pull Request #964 · libuv/libuv

falanger · 2016-08-01T17:19:08Z

Add RFC 4607 support for IPv4 and IPv6.

saghul · 2016-08-02T09:45:32Z

 *.xcodeproj
 *.xcworkspace

+CMakeLists.txt


unneeded change, please drop it

saghul · 2016-08-02T09:53:15Z

Thanks for contributing @falanger! Mostly LGTM, left some comments.

falanger · 2016-08-07T07:04:09Z

Hi, @saghul! Thanks for your time! I've fixed all the comments.

saghul · 2016-08-08T09:31:47Z

Changes look good, though I'd like to add IPv6 support if possible. /cc @libuv/collaborators any other reviews?

santigimeno · 2016-08-16T18:50:57Z

+    optname = IP_DROP_SOURCE_MEMBERSHIP;
+    break;
+  default:
+    return -EINVAL;


I think one level of indentation is missing here.

It's consistent with other "switch..case" instructions in this file.

Even though the style is not consistent in the project, I would add indentation here as it's already the most commonly used format in the source and I think consistency is good. Anyway, let's see what others think.

falanger · 2016-09-05T17:29:33Z

@saghul sorry, I was wrong about IPv6 support (thanks, @santigimeno).

falanger · 2016-09-10T15:44:15Z

@saghul @santigimeno any comments?

santigimeno · 2016-09-12T06:52:13Z

+    err = uv_ip6_addr(multicast_addr, 0, &mcast_addr6);
+    if (err)
+        return err;
+    err = uv_ip6_addr(multicast_addr, 0, &src_addr6);


shouldn't this be source_addr?

cjihrig · 2016-09-18T19:42:41Z

+  if (err)
+    return err;
+
+  memset(&mreq, 0, sizeof mreq);


Could you use consistent parentheses with sizeof.

LPardue · 2017-09-18T13:27:22Z

I was hoping to do some source-specific multicast in node.js (which uses libuv). This PR would introduce that capability but it is unfortunately stalled, is there any way to unblock it? It seemed like the last review identified some minor style issues but otherwise things were ok.

cjihrig · 2017-09-18T14:15:42Z

+  }
+
+  return 0;
+#else


Can you add a comment here that mentions what #ifdef this corresponds to.

cjihrig · 2017-09-18T14:22:48Z


+.. c:function:: int uv_udp_set_source_membership(uv_udp_t* handle, const char* multicast_addr, const char* interface_addr, const char* source_addr, uv_membership membership)
+
+    Set membership for a source-specific multicast group


Missing period at the end of the line.

cjihrig · 2017-09-18T14:23:30Z

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


This version number needs to be updated. Currently, this would be 1.15.0 at the earliest.

cjihrig · 2017-09-18T14:28:02Z

@falanger it's been quite a while. Sorry about that. Are you still interested in pursuing this PR?

falanger · 2017-09-19T07:23:21Z

@cjihrig Hi! Yes, I am interested in. I'll try to fix issues on this week.

cjihrig · 2017-09-20T00:17:07Z


  ASSERT(r == 0);

+  r = uv_udp_set_source_membership(&client, "ff02::1", NULL, "2001:db8:a0b:12f0::1", UV_JOIN_GROUP);


Can the rest of the test still be run if UV_ENODEV or UV_EPROTONOSUPPORT is returned? If so, maybe the assertion should just allow them so that the remainder of the test is still executed. If that can't be done, maybe a separate test file should be created.

cjihrig · 2017-09-20T00:17:51Z

  ASSERT(r == 0);

+  /* join the source-specific multicast channel */
+  r = uv_udp_set_source_membership(&client, "239.255.0.1", NULL, "10.50.129.200", UV_JOIN_GROUP);


Same comment here about finishing the rest of the test.

cjihrig · 2017-09-20T00:21:29Z

+  if (err) {
+    err = uv_ip6_addr(multicast_addr, 0, &mcast_addr6);
+    if (err)
+        return err;


Two space indentation please.

Sorry, I have fixed all indentation issues.

cjihrig · 2017-09-20T00:21:38Z

+        return err;
+    err = uv_ip6_addr(source_addr, 0, &src_addr6);
+    if (err)
+        return err;


cjihrig · 2017-09-20T00:28:43Z


  return 0;
+#else
+  return -EPROTONOSUPPORT;


UV_EPROTONOSUPPORT

cjihrig · 2017-09-20T00:31:37Z

+  if (err) {
+    err = uv_ip6_addr(multicast_addr, 0, &mcast_addr6);
+    if (err)
+        return err;


Again, two space indentation. I'll stop commenting on it :-)

cjihrig · 2017-09-20T00:34:00Z

  int err;

-  memset(&mreq, 0, sizeof mreq);
+  memset(&mreq, 0, sizeof(mreq));


Can you revert all of the unrelated style changes.

Fixed. BTW, why the style is inconsistent in file (and project) scope?

why the style is inconsistent in file (and project) scope?

Because we haven't agreed on and enforced automated linting. So, some stuff gets by the human eye test.

cjihrig · 2017-10-16T23:47:54Z

+  mreq.imr_multiaddr.s_addr = multicast_addr->sin_addr.s_addr;
+  mreq.imr_sourceaddr.s_addr = source_addr->sin_addr.s_addr;
+
+  switch (membership) {


I would just write this as an if...else if...else. It would be several lines shorter, and there are only a couple of options. I'd also move it to the top of the function so that -EINVAL can be returned right away if necessary.

I agree with you, but in this case functions uv_udp_set_membership and uv_udp_set_source_membership will be written in different manner. If this is OK, I would replace switch .. case with if .. else construction.

Yea, I'd go with if...else

cjihrig · 2017-10-16T23:49:53Z

+
+  return 0;
+#else
+  return UV_EPROTONOSUPPORT;


I think this should be -EPROTONOSUPPORT.

Come to think of it, I think we return ENOSYS in similar situations, but I could be wrong.

In my opinion, ENOSYS means that function not implemented, so I think that EPROTONOSUPPORT or ENOPROTOOPT is more suitable error in this case.

I'll defer to other @libuv/collaborators. Either way, this should be -EWHATEVER in the Unix code.

cjihrig · 2017-10-16T23:51:32Z

+  memcpy(&mreq.gsr_group, multicast_addr, sizeof(mreq.gsr_group));
+  memcpy(&mreq.gsr_source, source_addr, sizeof(mreq.gsr_source));
+
+  switch (membership) {


Same comment as the previous switch statement.

cjihrig · 2017-10-16T23:57:24Z

 #include "stream-inl.h"
 #include "req-inl.h"

+#if defined(MCAST_JOIN_SOURCE_GROUP) && defined(MCAST_LEAVE_SOURCE_GROUP)


Would it be possible to consolidate with the same code in src/unix/udp.c?

Yes, It's possible. Can I place this defines in uv.h header file?

moved to uv.h

cjihrig · 2017-10-16T23:58:28Z

+  mreq.imr_multiaddr.s_addr = multicast_addr->sin_addr.s_addr;
+  mreq.imr_sourceaddr.s_addr = source_addr->sin_addr.s_addr;
+
+  switch (membership) {


Same comment about the switch. I'll stop repeating it now.

LPardue · 2017-10-17T13:35:47Z

+  memset(&mreq, 0, sizeof(mreq));
+
+  if (interface_addr) {
+    err = uv_ip6_addr(interface_addr, 0, &addr6)


Missing semi-colon, causes a compile failure

LPardue · 2017-10-17T13:47:39Z

+    err = uv_ip6_addr(interface_addr, 0, &addr6)
+    if (err)
+      return err;
+    mreq.ipv6mr_interface = addr6.sin6_scope_id;


This fails to compile for me:

src\win\udp.c(798): error C2039: 'ipv6mr_interface': is not a member of 'group_source_req'

Do you mean mreq.gsr_interface?

Yes, that's right. Fixed

@LPardue please, send me a mention next time. I'll try to write tests as soon as I can - it's the last thing that needs to be done to merge this PR.

@falanger will do, feel free to take inspiration from the SSM node tests I created in nodejs/node#15735

cjihrig · 2017-11-01T17:26:02Z

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


This would now be 1.16.0

cjihrig · 2017-11-01T17:27:13Z

+  mreq.imr_multiaddr.s_addr = multicast_addr->sin_addr.s_addr;
+  mreq.imr_sourceaddr.s_addr = source_addr->sin_addr.s_addr;
+
+  switch (membership) {


Yea, I'd go with if...else

cjihrig · 2017-11-01T17:28:34Z

+
+  memset(&mreq, 0, sizeof(mreq));
+
+  if (interface_addr) {


One thing throughout this PR. Can you try to be more explicit in these ifs by checking for == and != to 0, NULL, etc.

cjihrig · 2017-11-01T17:31:39Z

+
+  return 0;
+#else
+  return UV_EPROTONOSUPPORT;


I'll defer to other @libuv/collaborators. Either way, this should be -EWHATEVER in the Unix code.

BridgeAR · 2017-11-22T18:54:31Z

Seems like this is required to unblock a PR in Node.js. It would be great if we could get this to land some time soon therefore :-)

BridgeAR · 2018-01-19T16:13:42Z

Ping @cjihrig @saghul @bnoordhuis

cjihrig · 2018-01-19T18:23:03Z

Unnecessary ping. It looks like there are a few unresolved review comments.

BridgeAR · 2018-01-19T18:24:52Z

@cjihrig you are right. Sorry, I did not check the comments again. I looked at to many PRs today.

@falanger can you please have a look at the comments?

falanger · 2018-02-06T07:12:52Z

@cjihrig Hi! I've resolved all issues.

BridgeAR · 2018-02-06T09:01:23Z

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


I guess it should be Version 1.20.0 now because the current version is already Version 1.19.1.

santigimeno

Was there any resolution with regards to the tests?

santigimeno · 2018-02-06T13:02:44Z

+  struct sockaddr_in mcast_addr4;
+  struct sockaddr_in src_addr4;
+  struct sockaddr_in6 mcast_addr6;
+  struct sockaddr_in6 src_addr6;


I think you could use 2 sockaddr_storage that you could later on cast to sockaddr_in or sockaddr_in6. See: uv_udp_set_multicast_interface implementation.

I don't think so because the uv_udp_set_source_membership function should reproduce uv_udp_set_membership functionality with additional parameter source_addr. These functions have the same behavior and they should be written in the same manner IMHO.

OK, I see that uv_udp_set_membership is not using sockaddr_storage but it seems to me that it could and you could avoid having 2 extra sockaddr structs in the stack.

struct sockaddr_storage mcast_addr; struct sockaddr_in* mcast_addr4; struct sockaddr_in6* mcast_addr6; struct sockaddr_storage src_addr; struct sockaddr_in* src_addr4; struct sockaddr_in6* src_addr6; mcast_addr4 = (struct sockaddr_in*)&mcast_addr; mcast_addr6 = (struct sockaddr_in6*)&mcast_addr; src_addr4 = (struct sockaddr_in*)&src_addr; src_addr6 = (struct sockaddr_in6*)&src_addr;

Though it seems too verbose. Maybe using an union looks nicer. Something like this:

int uv_udp_set_source_membership(uv_udp_t* handle, const char* multicast_addr, const char* interface_addr, const char* source_addr, uv_membership membership) { int err; union address { struct sockaddr_in sa_in; struct sockaddr_in6 sa_in6; }; union address mcast_addr; union address src_addr; err = uv_ip4_addr(multicast_addr, 0, &mcast_addr.sa_in); if (err) { err = uv_ip6_addr(multicast_addr, 0, &mcast_addr.sa_in6); if (err) return err; err = uv_ip6_addr(source_addr, 0, &src_addr.sa_in6); if (err) return err; return uv__udp_set_source_membership6(handle, &mcast_addr.sa_in6, interface_addr, &src_addr.sa_in6, membership); } err = uv_ip4_addr(source_addr, 0, &src_addr.sa_in); if (err) return err; return uv__udp_set_source_membership4(handle, &mcast_addr.sa_in, interface_addr, &src_addr.sa_in, membership); }

Ok, I'll take a look!

santigimeno · 2018-02-06T13:03:06Z

+  struct sockaddr_in6 mcast_addr6;
+  struct sockaddr_in6 src_addr6;
+
+  int err = uv_ip4_addr(multicast_addr, 0, &mcast_addr4);


style: we separate variable declaration from assignment.

santigimeno · 2018-02-06T13:04:11Z

+  struct sockaddr_in mcast_addr4;
+  struct sockaddr_in src_addr4;
+  struct sockaddr_in6 mcast_addr6;
+  struct sockaddr_in6 src_addr6;


Same comments as on unix.

santigimeno · 2018-02-07T09:50:01Z

+  err = uv_ip4_addr(source_addr, 0, &src_addr4);
+  if (err)
+    return err;
+  return uv__udp_set_source_membership4(handle, &mcast_addr4, interface_addr, &src_addr4, membership);


Style: long lines. Max should be 80 characters.

santigimeno · 2018-02-07T09:50:10Z

+  err = uv_ip4_addr(source_addr, 0, &src_addr4);
+  if (err)
+    return err;
+  return uv__udp_set_source_membership4(handle, &mcast_addr4, interface_addr, &src_addr4, membership);


Style: long lines

santigimeno · 2018-02-07T09:54:13Z

Are you planning on adding tests?

falanger · 2018-02-07T19:50:51Z

Sure, I plan to write tests for join/leave SSM group. Unfortunately, it not possible to write tests that receive/send a multicast packet (just like a "usual" multicast) because they require a "real" multicast network.

BridgeAR · 2018-03-02T03:44:04Z

Ping @falanger

BridgeAR · 2018-04-10T00:47:57Z

I see there was a push to the branch after my last comment but I am not sure what the status of this is @falanger

BridgeAR · 2018-04-22T15:30:46Z

Ping @falanger

BridgeAR · 2018-05-15T11:50:37Z

@santigimeno would you be willing to add some tests for this? It would really be great to unblock the Node.js PR by getting this ready and to land it.

LPardue · 2018-06-01T10:35:21Z

A pragmatic decision would be to just have a simple test that shows data flow between sender and receiver in a similar vein to the existing any source multicast. Full testing of source-specific multicast needs an environment more complex than what the framework provides (e.g. multiple network subnets and a router).

It appears this ticket is stalled on that issue and there is lack of input from the project on what is an appropriate solution.

OlofBailey · 2018-12-20T09:49:11Z

What's the status of this lack of SSM support in node.js is turning into a major pain for a lot of projects.

santigimeno · 2019-02-22T05:39:49Z

Superseded by #2202.

falanger changed the title ~~Source-specific multicast support~~ udp4: add source-specific multicast support Aug 1, 2016

saghul reviewed Aug 2, 2016
View reviewed changes

Comment thread .gitignore Outdated

*.xcodeproj

*.xcworkspace

CMakeLists.txt

Copy link
Copy Markdown

Member

saghul Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded change, please drop it

santigimeno reviewed Aug 16, 2016
View reviewed changes

falanger changed the title ~~udp4: add source-specific multicast support~~ udp: add source-specific multicast support Sep 5, 2016

santigimeno reviewed Sep 12, 2016
View reviewed changes

cjihrig reviewed Sep 18, 2016

View reviewed changes

cjihrig added the stalled label Sep 5, 2017

cjihrig reviewed Sep 18, 2017

View reviewed changes

cjihrig reviewed Sep 20, 2017

View reviewed changes

LPardue mentioned this pull request Oct 2, 2017

dgram: add source-specific multicast support nodejs/node#15735

Closed

4 tasks

cjihrig reviewed Oct 16, 2017

View reviewed changes

LPardue reviewed Oct 17, 2017

View reviewed changes

cjihrig reviewed Nov 1, 2017

View reviewed changes

BridgeAR reviewed Feb 6, 2018

View reviewed changes

santigimeno requested changes Feb 6, 2018

View reviewed changes

santigimeno reviewed Feb 7, 2018

View reviewed changes

udp: add source-specific multicast support

af73c67

cjihrig force-pushed the v1.x branch from c92be35 to abe9e01 Compare September 18, 2018 15:50

santigimeno mentioned this pull request Feb 14, 2019

src: fill sockaddr_in6.sin6_len when it's defined #2189

Closed

santigimeno mentioned this pull request Feb 22, 2019

udp: add source-specific multicast support #2202

Merged

santigimeno closed this Feb 22, 2019


		.. c:function:: int uv_udp_set_source_membership(uv_udp_t* handle, const char* multicast_addr, const char* interface_addr, const char* source_addr, uv_membership membership)

		Set membership for a source-specific multicast group


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

		.. versionadded:: 1.10.0


		ASSERT(r == 0);

		r = uv_udp_set_source_membership(&client, "ff02::1", NULL, "2001:db8:a0b:12f0::1", UV_JOIN_GROUP);


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

		.. versionadded:: 1.15.0


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

		.. versionadded:: 1.16.0

Conversation

falanger commented Aug 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Aug 2, 2016

Uh oh!

falanger commented Aug 7, 2016

Uh oh!

saghul commented Aug 8, 2016

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!

falanger commented Sep 5, 2016

Uh oh!

falanger commented Sep 10, 2016

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!

LPardue commented Sep 18, 2017

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!

cjihrig commented Sep 18, 2017

Uh oh!

falanger commented Sep 19, 2017

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!

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!

falanger Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

falanger commented Aug 1, 2016 •

edited

Loading

falanger Oct 20, 2017 •

edited

Loading

falanger Oct 20, 2017 •

edited

Loading