Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531763006:
Enable IPv6 temporary address filtering on iOS (Closed)
|
Created:
5 years ago by guoweis_webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionEnable IPv6 temporary address filtering on iOS.
We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes.
BUG=webrtc:4343
Committed: https://crrev.com/29488c23644721c10a45eba74c67602b0262e582
Cr-Commit-Position: refs/heads/master@{#11114}
Patch Set 1 #
Total comments: 41
Patch Set 2 : address comments. #
Total comments: 3
Patch Set 3 : address comments #Patch Set 4 : rebase #Patch Set 5 : renaming a function. #Patch Set 6 : fix test case. #Patch Set 7 : fix mac test case. #Patch Set 8 : fix the build break in linux_rel #Patch Set 9 : fix android build #
Messages
Total messages: 34 (20 generated)
Description was changed from ========== -mfix -mfix -mfix -mfix -minitial checkin BUG= patch from issue 1510173005 at patchset 310001 (http://crrev.com/1510173005#ps310001) ========== to ========== Enable temporary IPv6 filtering on iOS BUG=webrtc:4343 ==========
guoweis@webrtc.org changed reviewers: + pthatcher@webrtc.org
PTAL
Description was changed from ========== Enable temporary IPv6 filtering on iOS BUG=webrtc:4343 ========== to ========== Enable IPv6 temporary address filtering on iOS BUG=webrtc:4343 ==========
Description was changed from ========== Enable IPv6 temporary address filtering on iOS BUG=webrtc:4343 ========== to ========== Enable IPv6 temporary address filtering on iOS. We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes. BUG=webrtc:4343 ==========
guoweis@webrtc.org changed reviewers: + juberti@webrtc.org
https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/base.gyp#newcode640 webrtc/base/base.gyp:640: 'ifaddrs_converter.cc', Why shouldn't windows have this file? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter.cc File webrtc/base/ifaddrs_converter.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:42: LOG(LS_ERROR) << "IPv6 " << ip->ToString(); Was this just a debugging statement that you should remove? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:49: bool IfAddrsConverter::ConvertNativeToIPAttributes( What is "native" in this context? Should this be ConvertInterfaceToAttributes? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:56: #if !defined(WEBRTC_MAC) Can you leave a comment about what happens for mac? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter.h File webrtc/base/ifaddrs_converter.h (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.h:24: class IfAddrsConverter { Can you please leave a comment explaining the purpose of the class? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.h:30: IPAddress* mask); I like the name pattern ConvertXToY. Can you use that here also? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... File webrtc/base/mac_ifaddrs_converter.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. The pattern for other mac-specific files is macfoo_bar.cc, not mac_foo_bar.cc. So, macifaddrs_converter. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:189: : ioctl_socket_(socket(AF_INET6, SOCK_DGRAM, 0)) { Should the class be called Ipv6AttributesGetter, since it's IPv6 specific? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:189: : ioctl_socket_(socket(AF_INET6, SOCK_DGRAM, 0)) { Can you leave a comment explaining what the 0 means here, and why we're creating a socket? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:200: } Maybe: if (!initialized()) { return; } close(ioctl_socket_); https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:205: int* native_attributes) { Should we check that it's initialized? if (!initialized()) { return false; } https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:211: *native_attributes = ifr.ifr_ifru.ifru_flags; Why not just do the conversion here? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:219: // application layer. This would be more clear as "Converts native IPv6 address attributes to net IPv6 address attributes. If it returns false, the IP address isn't suitable for one-to-one communications applications and should be ignored." https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:221: int* net_attributes) { I don't think you need the "try" in the name here. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:226: // by the application layer. The last part might be more clear if shorter: "not suitable for one-to-one communication applications" https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:229: return false; Would it make sense to make a flag set called "unsuitable"? Perhaps: int unsuitable = IN6_IFF_ANYCAST | IN6_IFF_DUPLICATED | IN6_IFF_TENTATIVE | IN6_IFF_DETACHED; if (unsuitable & native_attributes) { return false; } https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:243: class IfAddrsConverterMac : public IfAddrsConverter { The current pattern for mac-specific classes is currently MacFooBar, not FooBarMac. So, MacIfAddrsConverter https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:248: bool ConvertNativeToIPAttributes(const struct ifaddrs* interface, It seems like we could make this structure more simple by limiting the scope of the IfAddrsConverter more. The only mac-specific part is the ConvertNativeToIPAttributes part, not the full Convert part. So why not make the IfAddrsConveret interface/class based around ConvertNativeToIPAttributes instead of Convert? In other words, move the shared logic out of a base class and into network.cc. Then all you have in the base-class/interface is the part that's actually different and not the shared part also. I think that would be more simple. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:254: } Seeing that this is really about *getting* attributes from an interface and not *converting* them, it seems like this method should be called GetIPAttributes, and the class name IPAttributesGetter. In fact, you already have one of those, which is great. Now you could just have a normal IPAttributesGetter and a MacAttributesGetter which does the conversion internally. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc#newcod... webrtc/base/network.cc:142: return true; This (DEPRECATED) is new functionality, right? So should we have unit tests for it? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc#newcod... webrtc/base/network.cc:399: } This (IFF_RUNNING) is new functionality, right? So should we have unit tests for it? Also, can you make the style consistent (either "variable & CONSTANT" or "CONSTANT & variable")?
PTAL. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/base.gyp#newcode640 webrtc/base/base.gyp:640: 'ifaddrs_converter.cc', On 2015/12/18 19:51:20, pthatcher1 wrote: > Why shouldn't windows have this file? Only POSIX systems use ifaddrs structure. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter.cc File webrtc/base/ifaddrs_converter.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:42: LOG(LS_ERROR) << "IPv6 " << ip->ToString(); On 2015/12/18 19:51:20, pthatcher1 wrote: > Was this just a debugging statement that you should remove? Sorry, missed this one. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:49: bool IfAddrsConverter::ConvertNativeToIPAttributes( On 2015/12/18 19:51:20, pthatcher1 wrote: > What is "native" in this context? Should this be ConvertInterfaceToAttributes? > I meant by native attributes. They have their own native definitions for mac and linux, and I'm converting them to what we have in IPAddress.h. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:56: #if !defined(WEBRTC_MAC) On 2015/12/18 19:51:20, pthatcher1 wrote: > Can you leave a comment about what happens for mac? Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter.h File webrtc/base/ifaddrs_converter.h (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.h:24: class IfAddrsConverter { On 2015/12/18 19:51:20, pthatcher1 wrote: > Can you please leave a comment explaining the purpose of the class? Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.h:30: IPAddress* mask); On 2015/12/18 19:51:20, pthatcher1 wrote: > I like the name pattern ConvertXToY. Can you use that here also? Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... File webrtc/base/mac_ifaddrs_converter.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. On 2015/12/18 19:51:21, pthatcher1 wrote: > The pattern for other mac-specific files is macfoo_bar.cc, not mac_foo_bar.cc. > So, macifaddrs_converter. Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:189: : ioctl_socket_(socket(AF_INET6, SOCK_DGRAM, 0)) { On 2015/12/18 19:51:21, pthatcher1 wrote: > Should the class be called Ipv6AttributesGetter, since it's IPv6 specific? Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:200: } On 2015/12/18 19:51:21, pthatcher1 wrote: > Maybe: > > if (!initialized()) { > return; > } > close(ioctl_socket_); Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:205: int* native_attributes) { On 2015/12/18 19:51:20, pthatcher1 wrote: > Should we check that it's initialized? > > if (!initialized()) { > return false; > } Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:211: *native_attributes = ifr.ifr_ifru.ifru_flags; On 2015/12/18 19:51:20, pthatcher1 wrote: > Why not just do the conversion here? I copied the code from chrome. In chrome, each platform has its own TryConvertNativeToIPAttributes. I feel it's easier to just look at this function to know what flags we ignore/accept. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:221: int* net_attributes) { On 2015/12/18 19:51:21, pthatcher1 wrote: > I don't think you need the "try" in the name here. Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:226: // by the application layer. On 2015/12/18 19:51:21, pthatcher1 wrote: > The last part might be more clear if shorter: "not suitable for one-to-one > communication applications" Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:229: return false; On 2015/12/18 19:51:21, pthatcher1 wrote: > Would it make sense to make a flag set called "unsuitable"? > > Perhaps: > int unsuitable = IN6_IFF_ANYCAST | IN6_IFF_DUPLICATED | IN6_IFF_TENTATIVE | > IN6_IFF_DETACHED; > if (unsuitable & native_attributes) { > return false; > } From the readability point of view, I think they are about the same. Let me know if you feel we should have another variable strongly. If so, it probably should be a const def? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:243: class IfAddrsConverterMac : public IfAddrsConverter { On 2015/12/18 19:51:21, pthatcher1 wrote: > The current pattern for mac-specific classes is currently MacFooBar, not > FooBarMac. So, MacIfAddrsConverter Done. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:248: bool ConvertNativeToIPAttributes(const struct ifaddrs* interface, On 2015/12/18 19:51:21, pthatcher1 wrote: > It seems like we could make this structure more simple by limiting the scope of > the IfAddrsConverter more. The only mac-specific part is the > ConvertNativeToIPAttributes part, not the full Convert part. So why not make > the IfAddrsConveret interface/class based around ConvertNativeToIPAttributes > instead of Convert? > > In other words, move the shared logic out of a base class and into network.cc. > Then all you have in the base-class/interface is the part that's actually > different and not the shared part also. I think that would be more simple. This might not always be true. For example, the vmware interface might have different names on different platforms and instead of having them all in one places with ifdef in network.cc, the current way gives us flexibility if we want to filter things other than attributes. https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/mac_ifaddrs_conve... webrtc/base/mac_ifaddrs_converter.cc:254: } On 2015/12/18 19:51:21, pthatcher1 wrote: > Seeing that this is really about *getting* attributes from an interface and not > *converting* them, it seems like this method should be called GetIPAttributes, > and the class name IPAttributesGetter. > > In fact, you already have one of those, which is great. Now you could just have > a normal IPAttributesGetter and a MacAttributesGetter which does the conversion > internally. I guess this comment is related to the one above? https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc#newcod... webrtc/base/network.cc:142: return true; On 2015/12/18 19:51:21, pthatcher1 wrote: > This (DEPRECATED) is new functionality, right? So should we have unit tests for > it? > This mimics chrome's logic. Yes, we had already. NetworkTest, TestIPv6Selection https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/network.cc#newcod... webrtc/base/network.cc:399: } On 2015/12/18 19:51:21, pthatcher1 wrote: > This (IFF_RUNNING) is new functionality, right? So should we have unit tests > for it? > > Also, can you make the style consistent (either "variable & CONSTANT" or > "CONSTANT & variable")? Done.
lgtm, with some comment updates and maybe a method rename https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter.cc File webrtc/base/ifaddrs_converter.cc (right): https://codereview.webrtc.org/1531763006/diff/1/webrtc/base/ifaddrs_converter... webrtc/base/ifaddrs_converter.cc:49: bool IfAddrsConverter::ConvertNativeToIPAttributes( On 2015/12/21 20:26:33, guoweis wrote: > On 2015/12/18 19:51:20, pthatcher1 wrote: > > What is "native" in this context? Should this be > ConvertInterfaceToAttributes? > > > > I meant by native attributes. They have their own native definitions for mac and > linux, and I'm converting them to what we have in IPAddress.h. Then should it be named ConvertNativeAttributesToIPAttributes? https://codereview.webrtc.org/1531763006/diff/20001/webrtc/base/ifaddrs_conve... File webrtc/base/ifaddrs_converter.h (right): https://codereview.webrtc.org/1531763006/diff/20001/webrtc/base/ifaddrs_conve... webrtc/base/ifaddrs_converter.h:24: // Subclass should implement ConvertNativeToIPAttributes to wrap the different Subclass => Subclasses https://codereview.webrtc.org/1531763006/diff/20001/webrtc/base/ifaddrs_conve... webrtc/base/ifaddrs_converter.h:24: // Subclass should implement ConvertNativeToIPAttributes to wrap the different wrap => implement https://codereview.webrtc.org/1531763006/diff/20001/webrtc/base/ifaddrs_conve... webrtc/base/ifaddrs_converter.h:25: // ways of retrieving IPv6 attributes for various POSIX platforms. This comment still doesn't say what the class does. Perhaps something like "Converts native interface addresses to our internal IPAddress class"?
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1531763006/#ps40001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531763006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531763006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1531763006/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531763006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531763006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1531763006/#ps180001 (title: "fix the build break in linux_rel")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531763006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531763006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/10006)
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1531763006/#ps200001 (title: "fix android build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531763006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531763006/200001
Message was sent while issue was closed.
Description was changed from ========== Enable IPv6 temporary address filtering on iOS. We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes. BUG=webrtc:4343 ========== to ========== Enable IPv6 temporary address filtering on iOS. We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes. BUG=webrtc:4343 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Enable IPv6 temporary address filtering on iOS. We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes. BUG=webrtc:4343 ========== to ========== Enable IPv6 temporary address filtering on iOS. We'll only use temporary address for IPv6. However, due to a bug in iOS sdk, the necessary headers are not included. This change copies the minimum necessary definitions such that we could retrieve the ip attributes. BUG=webrtc:4343 Committed: https://crrev.com/29488c23644721c10a45eba74c67602b0262e582 Cr-Commit-Position: refs/heads/master@{#11114} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/29488c23644721c10a45eba74c67602b0262e582 Cr-Commit-Position: refs/heads/master@{#11114} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
