Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083013008:
Return both IPv6 and IPv4 address from the lookup. (Closed)
DescriptionReturn both IPv6 and IPv4 address from the lookup.
We currently only return IPv4 address, which may cause issues in IPv6 networks
if we provide host name as the turn servers.
BUG=webrtc:5871
R=juberti@google.com, pthatcher@webrtc.org
Committed: https://crrev.com/56c0b204901082376b84b2836227bfebbea97aeb
Cr-Commit-Position: refs/heads/master@{#13291}
Patch Set 1 #
Total comments: 3
Patch Set 2 : . #Messages
Total messages: 19 (9 generated)
https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc#new... webrtc/base/nethelpers.cc:47: hints.ai_family = family; This will be essentially using AF_UNSPEC because the family coming from an IP that is unresolved.
Description was changed from ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 ========== to ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 ==========
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
juberti@google.com changed reviewers: + juberti@google.com
lgtm, as discussed in email. Can we test this with an injected fake resolver that returns both A and AAAA records?
On 2016/06/24 16:45:40, juberti wrote: > lgtm, as discussed in email. > > Can we test this with an injected fake resolver that returns both A and AAAA > records? What would be testing? That we pass in AF_UNSPEC? To do so we'd have to replace getaddrinfo temporarily in a unit test. Or we could factor out a function that's GetAddrInfoHints and then test that.
https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc#new... webrtc/base/nethelpers.cc:47: hints.ai_family = family; On 2016/06/24 01:19:08, honghaiz3 wrote: > This will be essentially using AF_UNSPEC because the family coming from an IP > that is unresolved. I think we should have a really big comment right here. Perhaps something like: |family| here will almost always be AF_UNSPEC, because |family| comes from AsyncResolver::addr_.family(), which is a SocketAddress, and a SocketAddress, when constructed with a hostname is given a family of AF_UNSPEC. However, if someday in the future a way to construct a SocketAddress with both a hostname and a family other than AF_UNSPEC, then it would be possible to get a specific family value here. The behavior of AF_UNSPEC is roughly "get both ipv4 and ipv6", as documented by the various operating systems: Linux: http://man7.org/linux/man-pages/man3/getaddrinfo.3.html Windows: https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx Mac: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... Android (source code, not documentation): https://android.googlesource.com/platform/bionic/+/7e0bfb511e85834d7c6cb96312...
Add comments as Peter suggested. https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083013008/diff/1/webrtc/base/nethelpers.cc#new... webrtc/base/nethelpers.cc:47: hints.ai_family = family; On 2016/06/24 18:36:05, pthatcher1 wrote: > On 2016/06/24 01:19:08, honghaiz3 wrote: > > This will be essentially using AF_UNSPEC because the family coming from an IP > > that is unresolved. > > I think we should have a really big comment right here. > > Perhaps something like: > > > |family| here will almost always be AF_UNSPEC, because |family| comes from > AsyncResolver::addr_.family(), which is a SocketAddress, and a SocketAddress, > when constructed with a hostname is given a family of AF_UNSPEC. However, if > someday in the future a way to construct a SocketAddress with both a hostname > and a family other than AF_UNSPEC, then it would be possible to get a specific > family value here. > > The behavior of AF_UNSPEC is roughly "get both ipv4 and ipv6", as documented by > the various operating systems: > > Linux: > http://man7.org/linux/man-pages/man3/getaddrinfo.3.html > Windows: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx > Mac: > https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... > Android (source code, not documentation): > https://android.googlesource.com/platform/bionic/+/7e0bfb511e85834d7c6cb96312... > > Done. Thanks!
lgtm
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from juberti@google.com Link to the patchset: https://codereview.webrtc.org/2083013008/#ps20001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/06/27 04:22:32, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... I think Justin probably want to test that if the resolver returns both IPv6 and IPv4 addresses, the TURN and STUN ports will pick only the address matching its own address family. I will land this first and make a new CL to get separate reviews.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Description was changed from ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 ========== to ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 R=juberti@google.com, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/56c0b204901082376b84b2836... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 56c0b204901082376b84b2836227bfebbea97aeb (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 R=juberti@google.com, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/56c0b204901082376b84b2836... ========== to ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 R=juberti@google.com, pthatcher@webrtc.org Committed: https://crrev.com/56c0b204901082376b84b2836227bfebbea97aeb Cr-Commit-Position: refs/heads/master@{#13291} ==========
Message was sent while issue was closed.
Description was changed from ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrt:5871 R=juberti@google.com, pthatcher@webrtc.org Committed: https://crrev.com/56c0b204901082376b84b2836227bfebbea97aeb Cr-Commit-Position: refs/heads/master@{#13291} ========== to ========== Return both IPv6 and IPv4 address from the lookup. We currently only return IPv4 address, which may cause issues in IPv6 networks if we provide host name as the turn servers. BUG=webrtc:5871 R=juberti@google.com, pthatcher@webrtc.org Committed: https://crrev.com/56c0b204901082376b84b2836227bfebbea97aeb Cr-Commit-Position: refs/heads/master@{#13291} ========== |
