Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dgram: setMulticastInterface() outgoing interface selection #7855

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions doc/api/dgram.md
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,84 @@ added: v0.6.9
Sets or clears the `SO_BROADCAST` socket option. When set to `true`, UDP
packets may be sent to a local interface's broadcast address.

### socket.setMulticastInterface(multicastInterface)
<!-- YAML
added: REPLACEME
-->

* `multicastInterface` {String}

*Note: All references to scope in this section are refering to
[IPv6 Zone Indices][], which are defined by [RFC 4007][]. In string form, an IP
with a scope index is written as `'IP%scope'` where scope is an interface name or
interface number.*

Sets the default outgoing multicast interface of the socket to a chosen
interface or back to system interface selection. The `multicastInterface` must
be a valid string representation of an IP from the socket's family.

For IPv4 sockets, this should be the IP configured for the desired physical
interface. All packets sent to multicast on the socket will be sent on the
interface determined by the most recent successful use of this call.

For IPv6 sockets, `multicastInterface` should include a scope to indicate the
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that "should" is the right term. here and in the previous paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've no idea in terms of informal English. In RFC terminology, Line 315 has the only actual MUST, leaving some strong recommendations that I would call SHOULD:

In the IPv4 case, I mean SHOULD in the sense that any OS MAY be choosing other behaviors for other IPs that one should not rely on for maximum portability.
In the IPv6 case, I mean SHOULD as in it is technically optional to add the default scope, but I would recommend including it specifically to avoid bugs through miscommunication.

interface as in the examples that follow. In IPv6, individual `send` calls can
also use explicit scope in addresses, so only packets sent to a multicast
address without specifying an explicit scope are affected by the most recent
successful use of this call.

#### Examples: IPv6 Outgoing Multicast Interface

On most systems, where scope format uses the interface name:

```js
const socket = dgram.createSocket('udp6');

socket.bind(1234, () => {
socket.setMulticastInterface('::%eth1');
});
```

On Windows, where scope format uses an interface number:

```js
const socket = dgram.createSocket('udp6');

socket.bind(1234, () => {
socket.setMulticastInterface('::%2');
});
```

#### Example: IPv4 Outgoing Multicast Interface
All systems use an IP of the host on the desired physical interface:
```js
const socket = dgram.createSocket('udp4');

socket.bind(1234, () => {
socket.setMulticastInterface('10.0.0.2');
});
```

#### Call Results

A call on a socket that is not ready to send or no longer open may throw a *Not
running* [`Error`][].

If `multicastInterface` can not be parsed into an IP then an *EINVAL*
[`System Error`][] is thrown.

On IPv4, if `multicastInterface` is a valid address but does not match any
interface, or if the address does not match the family then
a [`System Error`][] such as `EADDRNOTAVAIL` or `EPROTONOSUP` is thrown.

On IPv6, most errors with specifying or omiting scope will result in the socket
continuing to use (or returning to) the system's default interface selection.

A socket's address family's ANY address (IPv4 `'0.0.0.0'` or IPv6 `'::'`) can be
used to return control of the sockets default outgoing interface to the system
for future multicast packets.
Copy link
Member

Choose a reason for hiding this comment

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

This whole section is not in harmony with the rest of the document. I would try to sum it up a bit.
Also, I would move the content before the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this level of detail in response to @jasnell 's request and then some requests for clarification of terms. Given that combinations like windows, IPv6 and multicast are extreme edge cases but need the most detail to implement correctly, I don't think it is possible to create a harmony where more common cases get the most (or at least equal) attention. I also don't see a clear way to restructure multicast related methods to a separate doc?



### socket.setMulticastLoopback(flag)
<!-- YAML
added: v0.3.8
Expand Down Expand Up @@ -512,4 +590,7 @@ and `udp6` sockets). The bound address and port can be retrieved using
[`socket.address().address`]: #dgram_socket_address
[`socket.address().port`]: #dgram_socket_address
[`socket.bind()`]: #dgram_socket_bind_port_address_callback
[`System Error`]: errors.html#errors_class_system_error
[byte length]: buffer.html#buffer_class_method_buffer_bytelength_string_encoding
[IPv6 Zone Indices]: https://en.wikipedia.org/wiki/IPv6_address#Link-local_addresses_and_zone_indices
[RFC 4007]: https://tools.ietf.org/html/rfc4007
15 changes: 15 additions & 0 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,21 @@ Socket.prototype.setMulticastLoopback = function(arg) {
};


Socket.prototype.setMulticastInterface = function(interfaceAddress) {
this._healthCheck();

if (typeof interfaceAddress !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we be able to get away with checking for IP addresses only here (net.isIP())?

Copy link
Contributor Author

@lostnet lostnet Aug 5, 2016

Choose a reason for hiding this comment

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

net.isIP() makes it look like no checks are necessary either in JS or the wrapper:

Its existing tests don't cover Scope, so I added some:
assert.equal(net.isIP('::%eth1'), 6);
assert.equal(net.isIP('::1%1'), 6);
These passed leading me to verify it is using a C implementation to get to libuv:

static void IsIP(const FunctionCallbackInfo<Value>& args) {

yet, this doesn't need any wrapper:

node/lib/net.js

Line 1581 in 863952e

exports.isIP = cares.isIP;

and still passes:
assert.equal(net.isIP(), 0);

If net.isIP() is correct then no check on interfaceAddress in JS and removing the argument checks should be fine?

**I've run the tests with gdb and determined that net.isIP() passes since *ip ends up as the string "undefined" when uv_inet_pton is called. So the wrapper is more conservative than net.isIP() but in a way we already discussed and throwing separate missing/non string typeerrors independently EINVALs provides better debugging while calling net.isIP() would duplicate code just to rename EINVAL something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lostnet would you please cover the missing parts for net.isIP() as you described in another patch?

throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'interfaceAddress',
'string');
}

const err = this._handle.setMulticastInterface(interfaceAddress);
if (err) {
throw errnoException(err, 'setMulticastInterface');
Copy link
Member

Choose a reason for hiding this comment

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

It should use the new error system

}
};

Socket.prototype.addMembership = function(multicastAddress,
interfaceAddress) {
this._healthCheck();
Expand Down
17 changes: 17 additions & 0 deletions src/udp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ void UDPWrap::Initialize(Local<Object> target,
GetSockOrPeerName<UDPWrap, uv_udp_getsockname>);
env->SetProtoMethod(t, "addMembership", AddMembership);
env->SetProtoMethod(t, "dropMembership", DropMembership);
env->SetProtoMethod(t, "setMulticastInterface", SetMulticastInterface);
env->SetProtoMethod(t, "setMulticastTTL", SetMulticastTTL);
env->SetProtoMethod(t, "setMulticastLoopback", SetMulticastLoopback);
env->SetProtoMethod(t, "setBroadcast", SetBroadcast);
Expand Down Expand Up @@ -238,6 +239,22 @@ X(SetMulticastLoopback, uv_udp_set_multicast_loop)

#undef X

void UDPWrap::SetMulticastInterface(const FunctionCallbackInfo<Value>& args) {
UDPWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap,
args.Holder(),
args.GetReturnValue().Set(UV_EBADF));

CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsString());

Utf8Value iface(args.GetIsolate(), args[0]);

const char* iface_cstr = *iface;

int err = uv_udp_set_multicast_interface(&wrap->handle_, iface_cstr);
args.GetReturnValue().Set(err);
}

void UDPWrap::SetMembership(const FunctionCallbackInfo<Value>& args,
uv_membership membership) {
Expand Down
2 changes: 2 additions & 0 deletions src/udp_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class UDPWrap: public HandleWrap {
static void RecvStop(const v8::FunctionCallbackInfo<v8::Value>& args);
static void AddMembership(const v8::FunctionCallbackInfo<v8::Value>& args);
static void DropMembership(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetMulticastInterface(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetMulticastTTL(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetMulticastLoopback(
const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down
4 changes: 3 additions & 1 deletion test/internet/test-dgram-multicast-multi-process.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const assert = require('assert');
const dgram = require('dgram');
const fork = require('child_process').fork;
const LOCAL_BROADCAST_HOST = '224.0.0.114';
const LOCAL_HOST_IFADDR = '0.0.0.0';
const TIMEOUT = common.platformTimeout(5000);
const messages = [
Buffer.from('First message to send'),
Expand Down Expand Up @@ -159,6 +160,7 @@ if (process.argv[2] !== 'child') {
sendSocket.setBroadcast(true);
sendSocket.setMulticastTTL(1);
sendSocket.setMulticastLoopback(true);
sendSocket.setMulticastInterface(LOCAL_HOST_IFADDR);
});

sendSocket.on('close', function() {
Expand Down Expand Up @@ -198,7 +200,7 @@ if (process.argv[2] === 'child') {
});

listenSocket.on('listening', function() {
listenSocket.addMembership(LOCAL_BROADCAST_HOST);
listenSocket.addMembership(LOCAL_BROADCAST_HOST, LOCAL_HOST_IFADDR);

listenSocket.on('message', function(buf, rinfo) {
console.error('[CHILD] %s received "%s" from %j', process.pid,
Expand Down
Loading