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

net: move isLegalPort to internal/net #4882

Merged
merged 1 commit into from
Jan 30, 2016
Merged

net: move isLegalPort to internal/net #4882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions lib/internal/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Is a new module file necessary? This seems small enough that it could possibly just go into internal/util

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell While it is small enough now for that, I have been working on cleaning up some of the logic in net and moving some more into internal. Either way works for me though, so if this is something you feel strongly about, I'll be happy to change.

Copy link
Member

Choose a reason for hiding this comment

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

@evanlucas ... :-) then by all means, continue as you are!


module.exports = { isLegalPort };

// Check that the port number is not NaN when coerced to a number,
// is an integer and that it falls within the legal range of port numbers.
function isLegalPort(port) {
if (typeof port === 'string' && port.trim() === '')
return false;
return +port === (port >>> 0) && port >= 0 && port <= 0xFFFF;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use Number.isInteger here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with parseInt() is that something like '10c' becomes 10, which is not right. The problem with Number.isInteger() is that '0xff' (or any other string) is false. There are all kinds of weird edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. Should we really allow values like that? Coercion allows people to use a lot of undocumented allowed values like this and I am not sure if we can cover all the possible valid values without a lot of checks. For example this condition would allow zero to one element number arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion - no we shouldn't coerce. But, it would be an unnecessary breaking change. Also, this function is used for range checking, not type checking.

}
11 changes: 2 additions & 9 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const stream = require('stream');
const timers = require('timers');
const util = require('util');
const internalUtil = require('internal/util');
const internalNet = require('internal/net');
const assert = require('assert');
const cares = process.binding('cares_wrap');
const uv = process.binding('uv');
Expand All @@ -22,6 +23,7 @@ const WriteWrap = process.binding('stream_wrap').WriteWrap;
var cluster;
const errnoException = util._errnoException;
const exceptionWithHostPort = util._exceptionWithHostPort;
const isLegalPort = internalNet.isLegalPort;

function noop() {}

Expand Down Expand Up @@ -846,15 +848,6 @@ function connect(self, address, port, addressType, localAddress, localPort) {
}


// Check that the port number is not NaN when coerced to a number,
// is an integer and that it falls within the legal range of port numbers.
function isLegalPort(port) {
if (typeof port === 'string' && port.trim() === '')
return false;
return +port === (port >>> 0) && port >= 0 && port <= 0xFFFF;
}


Socket.prototype.connect = function(options, cb) {
if (this.write !== Socket.prototype.write)
this.write = Socket.prototype.write;
Expand Down
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
'lib/internal/cluster.js',
'lib/internal/freelist.js',
'lib/internal/linkedlist.js',
'lib/internal/net.js',
'lib/internal/module.js',
'lib/internal/readline.js',
'lib/internal/repl.js',
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-net-internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

// Flags: --expose-internals

require('../common');
const assert = require('assert');
const net = require('internal/net');

assert.strictEqual(net.isLegalPort(''), false);
assert.strictEqual(net.isLegalPort('0'), true);
assert.strictEqual(net.isLegalPort(0), true);
assert.strictEqual(net.isLegalPort(65536), false);
assert.strictEqual(net.isLegalPort('65535'), true);
assert.strictEqual(net.isLegalPort(undefined), false);
assert.strictEqual(net.isLegalPort(null), true);
Copy link
Member

Choose a reason for hiding this comment

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

The difference between undefined and null seems a bit incongruous in retrospect.

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 agree. Would it be worth it to move the null/undefined checks to this function? They are currently done in net in both places where it is being used. Seems like we could simplify some logic by doing so

Copy link
Member

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis is that something you would prefer I do here or in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

A separate commit in this PR is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to be a bit more than I expected. Mind if I wait until another PR?