Skip to content

client IPv6 support [BEP-7] #41

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

Closed
astro wants to merge 1 commit into from
Closed

client IPv6 support [BEP-7] #41

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
21 changes: 21 additions & 0 deletions client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,27 @@ Tracker.prototype._handleResponse = function (requestUrl, data) {
self.client.emit('peer', addr)
})
}

if (Buffer.isBuffer(data.peers6)) {
// tracker returned compact response
var addrs
try {
addrs = compact2string.multi6(data.peers6)
} catch (err) {
return self.client.emit('warning', err)
}
addrs.forEach(function (addr) {
self.client.emit('peer', addr)
})
} else if (Array.isArray(data.peers6)) {
// tracker returned normal response
data.peers.forEach(function (peer) {
Copy link
Member

@feross feross Nov 27, 2014

Choose a reason for hiding this comment

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

Whoa! Discovered a bigger bug. This should be data.peers6.forEach! Will fix. ;)

Copy link
Contributor Author

@astro astro Nov 27, 2014

Choose a reason for hiding this comment

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

Oopsie. Ideally, we'd cover this with tests...

Copy link
Member

@feross feross Nov 27, 2014

Choose a reason for hiding this comment

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

Once we support ipv6 on the server (#43) we can easily add a test that tests both sides of this. :)

var ip = /:/.test(peer.ip) ?
Copy link
Member

@feross feross Nov 26, 2014

Choose a reason for hiding this comment

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

Is there ever a time when an IPv6 address won't have a : character? What is this check for?

Copy link
Member

@feross feross Nov 26, 2014

Choose a reason for hiding this comment

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

From my reading of the spec, the peers6 key should only contain ipv6 addresses.

peers6 contains address-port pairs where the addresses are all IPv6

And IPv6 addresses seem to always have : because that's the delimiter. So I can't figure out what this line is for.

I'll remove it for now, but let me know if it's important and we can re-add it.

Copy link
Contributor Author

@astro astro Nov 27, 2014

Choose a reason for hiding this comment

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

I was adding some tolerance in #40, and because the non-compact format essentially allows free-form hostnames, why not check if this is an IPv6 address at all?

It's not important to me and might lead interoperating people to assume a false level of flexibility in the HTTP tracker protocol.

Copy link
Member

@feross feross Nov 27, 2014

Choose a reason for hiding this comment

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

I see what you're saying. It's not hard to check if it's a domain, so let's do it.

'[' + peer.ip + ']' :
peer.ip
self.client.emit('peer', ip + ':' + peer.port)
})
}
} else if (requestUrl === self._scrapeUrl) {
// NOTE: the unofficial spec says to use the 'files' key but i've seen 'host' in practice
data = data.files || data.host || {}
Expand Down