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.

Sign up for GitHub

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

A few fixes #13

Closed
wants to merge 7 commits into from
Closed

A few fixes #13

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
12ffcb2
a little linting
Feb 10, 2015
e5f5373
remove netroute and extra ip checks
Feb 10, 2015
56d8fb4
jshinting
Feb 10, 2015
46abc9e
fix logic error
Feb 10, 2015
1c49dae
add .jscsrc... whatever that is
Feb 11, 2015
fafcb5c
add the correct .jscsrc
Feb 11, 2015
d02f5dc
back out curly braces
Feb 11, 2015
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
42 changes: 21 additions & 21 deletions lib/nat-upnp/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ var client = exports;
function Client() {
this.ssdp = nat.ssdp.create();
this.timeout = 1800;
};
}

client.create = function create() {
return new Client();
};

function normalizeOptions(options) {
function toObject(addr) {
if (typeof addr === 'number') return { port: addr };
if (typeof addr === 'object') return addr;
if (typeof addr === 'number') { return { port: addr }; }
Copy link
Owner

Choose a reason for hiding this comment

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

Wow, please don't do this here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean add curly braces?

I've come across so many errors that result from not using braces when working with others. I'm a very very strict linter. If in doubt, spell it out.

That said, it's your code. I tried to make lint commits separate from code commits.

Copy link
Owner

Choose a reason for hiding this comment

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

This is quite consistent with node.js code style. Though, we usually put statement on a next line.

Anyway, it would be really great if this project will be using jscs for code style checking. I have suggested a configuration file in another comment, do you think it might be interesting for you to implement?

if (typeof addr === 'object') { return addr; }

return {};
}
Expand All @@ -27,10 +27,10 @@ function normalizeOptions(options) {
}

Client.prototype.portMapping = function portMapping(options, callback) {
if (!callback) callback = function() {};
if (!callback) { callback = function() {}; }

this.findGateway(function(err, gateway, address) {
if (err) return callback(err);
if (err) { return callback(err); }

var ports = normalizeOptions(options);

Expand All @@ -50,10 +50,10 @@ Client.prototype.portMapping = function portMapping(options, callback) {
};

Client.prototype.portUnmapping = function portMapping(options, callback) {
if (!callback) callback = function() {};
if (!callback) { callback = function() {}; }

this.findGateway(function(err, gateway, address) {
if (err) return callback(err);
this.findGateway(function(err, gateway/*, address*/) {
if (err) { return callback(err); }

var ports = normalizeOptions(options);

Expand All @@ -72,10 +72,10 @@ Client.prototype.getMappings = function getMappings(options, callback) {
options = null;
}

if (!options) options = {};
if (!options) { options = {}; }

this.findGateway(function(err, gateway, address) {
if (err) return callback(err);
if (err) { return callback(err); }
var i = 0,
end = false,
results = [];
Expand All @@ -93,7 +93,7 @@ Client.prototype.getMappings = function getMappings(options, callback) {

var key;
Object.keys(data).some(function(k) {
if (!/:GetGenericPortMappingEntryResponse/.test(k)) return false;
if (!/:GetGenericPortMappingEntryResponse/.test(k)) { return false; }

key = k;
return true;
Expand All @@ -111,7 +111,7 @@ Client.prototype.getMappings = function getMappings(options, callback) {
port: parseInt(data.NewInternalPort, 10)
},
protocol: data.NewProtocol.toLowerCase(),
enabled: data.NewEnabled == 1,
enabled: data.NewEnabled === 1,
description: data.NewPortMappingDescription,
ttl: parseInt(data.NewLeaseDuration, 10)
};
Expand All @@ -122,7 +122,7 @@ Client.prototype.getMappings = function getMappings(options, callback) {
callback(null);
});
}, function(err) {
if (err) return callback(err);
if (err) { return callback(err); }

if (options.local) {
results = results.filter(function(item) {
Expand All @@ -141,25 +141,25 @@ Client.prototype.getMappings = function getMappings(options, callback) {
}

callback(null, results);
})
});
});
};

Client.prototype.externalIp = function externalIp(callback) {
this.findGateway(function(err, gateway, address) {
if (err) return callback(err);
this.findGateway(function(err, gateway/*, address*/) {
if (err) { return callback(err); }
gateway.run('GetExternalIPAddress', [], function(err, data) {
if (err) return callback(err);
if (err) { return callback(err); }
var key;

Object.keys(data).some(function(k) {
if (!/:GetExternalIPAddressResponse$/.test(k)) return false;
if (!/:GetExternalIPAddressResponse$/.test(k)) { return false; }

key = k;
return true;
});

if (!key) return callback(Error('Incorrect response'));
if (!key) { return callback(Error('Incorrect response')); }
callback(null, data[key].NewExternalIPAddress);
});
});
Expand All @@ -178,8 +178,8 @@ Client.prototype.findGateway = function findGateway(callback) {
callback(new Error('timeout'));
}, this.timeout);

p.on('device', function(info, address) {
if (timeouted) return;
p.on('device', function (info, address) {
if (timeouted) { return; }
p.emit('end');
clearTimeout(timeout);

Expand Down
106 changes: 31 additions & 75 deletions lib/nat-upnp/ssdp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
var dgram = require('dgram'),
util = require('util'),
os = require('os'),
net = require('net'),
netroute = require('netroute'),
ip = require('ip'),
EventEmitter = require('events').EventEmitter;

var ssdp = exports;

function Ssdp() {
function Ssdp(opts) {
EventEmitter.call(this);

this._opts = opts || {};
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just this.options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever.

_ prefix as convention for "private, not part of the API"

this._sourcePort = this._opts.sourcePort || 0;
Copy link
Owner

Choose a reason for hiding this comment

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

localPort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, whatever. I think sourcePort is more clear but maybe that's just me.

Copy link
Owner

Choose a reason for hiding this comment

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

I could live with _sourcePort, it is nothing important. Thanks for doing all of this!

this.multicast = '239.255.255.250';
this.port = 1900;
this._bound = false;
Expand All @@ -20,13 +19,25 @@ function Ssdp() {

// Create sockets on all external interfaces
this.createSockets();
};
}
util.inherits(Ssdp, EventEmitter);

ssdp.create = function create() {
return new Ssdp();
};

Ssdp.parseMimeHeader = function (headerStr) {
var lines = headerStr.split(/\r\n/g);

// Parse headers from lines to hashmap
return lines.reduce(function(headers, line) {
line.replace(/^([^:]*)\s*:\s*(.*)$/, function (a, key, value) {
headers[key.toLowerCase()] = value;
});
return headers;
}, {});
};

Ssdp.prototype.createSockets = function createSockets() {
var self = this,
interfaces = os.networkInterfaces();
Expand Down Expand Up @@ -55,7 +66,7 @@ Ssdp.prototype.search = function search(device, promise) {
}

// If promise was ended before binding - do not send queries
if (promise._ended) return;
if (promise._ended) { return; }

var self = this,
query = new Buffer('M-SEARCH * HTTP/1.1\r\n' +
Expand All @@ -71,8 +82,8 @@ Ssdp.prototype.search = function search(device, promise) {
}, this);

function ondevice(info, address) {
if (promise._ended) return;
if (info.st !== device) return;
if (promise._ended) { return; }
if (info.st !== device) { return; }

promise.emit('device', info, address);
}
Expand All @@ -93,7 +104,7 @@ Ssdp.prototype.createSocket = function createSocket(interface) {

socket.on('message', function (message, info) {
// Ignore messages after closing sockets
if (self._closed) return;
if (self._closed) { return; }

// Parse response
self.parseResponse(message.toString(), socket.address, info);
Expand All @@ -103,18 +114,16 @@ Ssdp.prototype.createSocket = function createSocket(interface) {
process.nextTick(function() {
// Unqueue this._queue once all sockets are ready
function onready() {
if (self._boundCount === self.sockets.length) return;
self._bound = true;
if (self._boundCount < self.sockets.length) { return; }

self._bound = true;
self._queue.forEach(function(item) {
if (item.action === 'search') {
return self.search(item.device, item.promise);
}
return self[item.action](item.device, item.promise);
});
}

socket.on('listening', function() {
self._boundCount++;
self._boundCount += 1;
onready();
});

Expand All @@ -125,76 +134,23 @@ Ssdp.prototype.createSocket = function createSocket(interface) {
});

socket.address = interface.address;
socket.bind(self.port, interface.address);
socket.bind(self._sourcePort, interface.address);
});

return socket;
};

// TODO create separate logic for parsing unsolicited upnp broadcasts, if and when that need arises
Ssdp.prototype.parseResponse = function parseResponse(response, addr, remote) {
response = response.split(/\r\n/g);

var status = response[0],
headers = response.slice(1);

// Ignore incorrect packets
if (!/^(HTTP|NOTIFY)/.test(status)) return;

// Parse headers from lines to hashmap
headers = headers.reduce(function(headers, line) {
line.replace(/^([^:]*)\s*:\s*(.*)$/, function(a, key, value) {
headers[key.toLowerCase()] = value;
});
return headers;
}, {});
if (!/^(HTTP|NOTIFY)/m.test(response)) { return; }

// We do not have interest in headers without location
if (!headers.location) return;
var headers = Ssdp.parseMimeHeader(response);

var interfaces = os.networkInterfaces(),
routes = netroute.getInfo(),
local;

local = routes.IPv4.concat(routes.IPv6).filter(function(route) {
if (route.gateway) {
if (ip.isEqual(remote.address, route.gateway)) return true;
}

// Remove /24 part from address
if (route.destination) {
route.destination = route.destination.replace(/\/\d+$/, '');
}

if (net.isIP(route.destination) && route.netmask) {
return ip.isEqual(ip.mask(remote.address, route.netmask),
ip.mask(route.destination, route.netmask));
}
if (net.isIP(route.destination)) {
return ip.isEqual(remote.address, route.destination) ||
ip.isEqual(route.destination, '::');
}
}).map(function(route) {
return interfaces[route.interface].filter(function(addr) {
return net.isIP(addr.address) === net.isIP(route.gateway) ||
net.isIP(addr.address) === net.isIP(route.destination) ||
net.isIP(addr.address) && net.isIP(route.netmask) &&
net.isIP(route.gateway) &&
ip.isEqual(ip.mask(addr.address, route.netmask),
ip.mask(route.gateway, route.netmask));
}).map(function(addr) {
return addr.address;
});
}).reduce(function(prev, next) {
return prev.concat(next);
}, []);

if (local.length === 0) {
local = addr;
} else {
local = local[0];
}
// We are only interested in messages that can be matched against the original search target
if (!headers.st) { return; }

this.emit('_device', headers, local);
this.emit('_device', headers, remote.address);
};

Ssdp.prototype.close = function close() {
Expand Down