Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
739983005:
Determine connection type in NetworkChangeNotifierLinux. (Closed)
|
Created:
6 years, 1 month ago by derekjchow1 Modified:
5 years, 10 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetermine connection type in NetworkChangeNotifierLinux.
Check interface for wireless extensions to determine if it is a wifi connection. Check SIOCETHTOOL for ethernet. GetCurrentConnectionType will return the CONNECTION_UNKNOWN unless all connections are the same type.
R=pauljensen@chromium.org
BUG=160537
Committed: https://crrev.com/5482d5e52c66f72082ee243464d0e109b66419a9
Cr-Commit-Position: refs/heads/master@{#314042}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address comments from patch set 1 #Patch Set 3 : Return CONNECTION_WIFI if all connection types are wifi #
Total comments: 30
Patch Set 4 : Address comments from patch set 3 #
Total comments: 24
Patch Set 5 : Address comments from patch set 4 #
Total comments: 4
Patch Set 6 : Address comments from patch set 5 #
Total comments: 10
Patch Set 7 : Remove unnessecary headers #
Total comments: 4
Patch Set 8 : Test ethtool for CONNECTION_ETHERNET #
Total comments: 10
Patch Set 9 : Suppress tunnel interfaces. Use ScopedFD. #
Total comments: 16
Patch Set 10 : Address comments in patchset 9 #
Total comments: 2
Patch Set 11 : Re-add write to ifnames #
Total comments: 3
Patch Set 12 : Fix nit, don't use designated initializers #Patch Set 13 : Android build fixes. #
Messages
Total messages: 40 (3 generated)
not lgtm https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:43: } extra line break https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:45: type = NetworkChangeNotifier::CONNECTION_ETHERNET; We should be defaulting to UNKNOWN not ETHERNET. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:294: return GetInterfaceConnectionType(primary_if); You've added lots of code executed while the lock is held. The lock should only be held while is_offline_ is read (a simple operation) so that we avoid having multiple threads blocking here. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:482: // IPv4 address means interface is configured. I don't think an IPv4 address means an interface is configured. Some interfaces are IPv6-only (e.g. TMobile's rmnet0). https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:483: // IP address is cleared by DHCP when Ethernet cable is unplugged. It may I don't think DHCP clears IP addresses on local interfaces when an Ethernet cable is unplugged. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.h:72: bool GetNetworkList(NetworkInterfaceList* networks, int policy) const; This API is exposed in net_util.h and should not be exposed here. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.h:73: std::string GetPrimaryNetif() const; This cannot be accurate as there may be multiple interfaces used by the default network. TMobile for example uses multiple interfaces (rmnet0 and clatd). https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... File net/base/network_change_notifier_linux.h (right): https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:20: static NetworkChangeNotifierLinux* GetInstance(); Accessing static instances is discouraged. Please remove. https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:24: bool RemoveNetifToIgnore(const std::string& netif_name); I see no motivation for adding these APIs. Please remove. https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:27: scoped_refptr<base::MessageLoopProxy> GetMessageLoopProxy() const; I don't think we should be exposing this.
Thanks for the feedback Paul. PTAL. I would also appreciate if you could clarify how GetConnectionType behaves when multiple interfaces are used (such as the T-Mobile case). https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:43: } On 2014/11/21 18:55:36, pauljensen wrote: > extra line break Done. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:45: type = NetworkChangeNotifier::CONNECTION_ETHERNET; On 2014/11/21 18:55:36, pauljensen wrote: > We should be defaulting to UNKNOWN not ETHERNET. I'm looking to use NetworkChangeNotifierLinux in a situation where NetworkChangeNotifier can distinguish between Ethernet and Wifi connections so it would be nice to have another check before defaulting to UNKNOWN. I've added another check that sets the connection type to ETHERNET if ifname starts with "eth" or "em". Is that ok? https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:294: return GetInterfaceConnectionType(primary_if); On 2014/11/21 18:55:36, pauljensen wrote: > You've added lots of code executed while the lock is held. The lock should only > be held while is_offline_ is read (a simple operation) so that we avoid having > multiple threads blocking here. Done. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:482: // IPv4 address means interface is configured. On 2014/11/21 18:55:36, pauljensen wrote: > I don't think an IPv4 address means an interface is configured. Some interfaces > are IPv6-only (e.g. TMobile's rmnet0). Done. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:483: // IP address is cleared by DHCP when Ethernet cable is unplugged. It may On 2014/11/21 18:55:36, pauljensen wrote: > I don't think DHCP clears IP addresses on local interfaces when an Ethernet > cable is unplugged. Done. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.h:72: bool GetNetworkList(NetworkInterfaceList* networks, int policy) const; On 2014/11/21 18:55:36, pauljensen wrote: > This API is exposed in net_util.h and should not be exposed here. Done. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.h:73: std::string GetPrimaryNetif() const; On 2014/11/21 18:55:36, pauljensen wrote: > This cannot be accurate as there may be multiple interfaces used by the default > network. TMobile for example uses multiple interfaces (rmnet0 and clatd). What should the correct behavior of NetworkChangeNotifier::GetConnectionType be in the case of multiple interfaces? This past reverted CL (https://codereview.chromium.org/298023005/patch/130001/140001) suggests that we should return the connection type if every network interface has the same connection type or otherwise return CONNECTION_UNKNOWN. Should address_tracker_linux preform something similar? https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... File net/base/network_change_notifier_linux.h (right): https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:20: static NetworkChangeNotifierLinux* GetInstance(); On 2014/11/21 18:55:36, pauljensen wrote: > Accessing static instances is discouraged. Please remove. Done. https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:24: bool RemoveNetifToIgnore(const std::string& netif_name); On 2014/11/21 18:55:36, pauljensen wrote: > I see no motivation for adding these APIs. Please remove. Done. https://codereview.chromium.org/739983005/diff/1/net/base/network_change_noti... net/base/network_change_notifier_linux.h:27: scoped_refptr<base::MessageLoopProxy> GetMessageLoopProxy() const; On 2014/11/21 18:55:36, pauljensen wrote: > I don't think we should be exposing this. Done.
https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.cc:45: type = NetworkChangeNotifier::CONNECTION_ETHERNET; On 2014/11/21 23:13:58, derekjchow1 wrote: > On 2014/11/21 18:55:36, pauljensen wrote: > > We should be defaulting to UNKNOWN not ETHERNET. > > I'm looking to use NetworkChangeNotifierLinux in a situation where > NetworkChangeNotifier can distinguish between Ethernet and Wifi connections so > it would be nice to have another check before defaulting to UNKNOWN. I've added > another check that sets the connection type to ETHERNET if ifname starts with > "eth" or "em". Is that ok? I don't think this works. I believe on Chromebook Pixels the LTE interface is named eth0. https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... net/base/address_tracker_linux.h:73: std::string GetPrimaryNetif() const; On 2014/11/21 23:13:58, derekjchow1 wrote: > On 2014/11/21 18:55:36, pauljensen wrote: > > This cannot be accurate as there may be multiple interfaces used by the > default > > network. TMobile for example uses multiple interfaces (rmnet0 and clatd). > > What should the correct behavior of NetworkChangeNotifier::GetConnectionType be > in the case of multiple interfaces? The correct behavior of NetworkChangeNotifier::GetConnectionType is to return the type of the first hop of the default internet connection. In the case of TMobile cellular with two interfaces, NetworkChangeNotifier::GetConnectionType should return one of the cellular types. > > This past reverted CL > (https://codereview.chromium.org/298023005/patch/130001/140001) suggests that we > should return the connection type if every network interface has the same > connection type or otherwise return CONNECTION_UNKNOWN. Should > address_tracker_linux preform something similar? GetNetworkList() (which uses AddressTrackerLinux on several hosts) should accurately determine the type of each interface if possible. NetworkChangeNotifier should should compute the type of the first hop of the default internet connection. If all results from GetNetworkList() are of the same type I think it is safe to assume the default internet connection is of this type too.
Hi Paul, how do the following changes sound? 1) Modify AddressTrackerLinux to monitor changes to the default route. GetConnectionType will return the connection type associated with the default route. This should return the default interface used to connect to the internet. 2) Add some functions to create a map of interface names to connection types (ie. "em0" is an ethernet interface or "eth0" is a 4G connection). GetInterfaceConnectionType will then look up this map to determine connection type if it does not find wireless extensions. For most cases, the map will be empty and the connection type will be unknown. However, for situations where the properties of network interfaces of the system are well known (such as Chromebook case given) we can return the connection type from the map. On 2014/11/24 12:33:18, pauljensen wrote: > https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... > File net/base/address_tracker_linux.cc (right): > > https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... > net/base/address_tracker_linux.cc:45: type = > NetworkChangeNotifier::CONNECTION_ETHERNET; > On 2014/11/21 23:13:58, derekjchow1 wrote: > > On 2014/11/21 18:55:36, pauljensen wrote: > > > We should be defaulting to UNKNOWN not ETHERNET. > > > > I'm looking to use NetworkChangeNotifierLinux in a situation where > > NetworkChangeNotifier can distinguish between Ethernet and Wifi connections so > > it would be nice to have another check before defaulting to UNKNOWN. I've > added > > another check that sets the connection type to ETHERNET if ifname starts with > > "eth" or "em". Is that ok? > > I don't think this works. I believe on Chromebook Pixels the LTE interface is > named eth0. > > https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... > File net/base/address_tracker_linux.h (right): > > https://codereview.chromium.org/739983005/diff/1/net/base/address_tracker_lin... > net/base/address_tracker_linux.h:73: std::string GetPrimaryNetif() const; > On 2014/11/21 23:13:58, derekjchow1 wrote: > > On 2014/11/21 18:55:36, pauljensen wrote: > > > This cannot be accurate as there may be multiple interfaces used by the > > default > > > network. TMobile for example uses multiple interfaces (rmnet0 and clatd). > > > > What should the correct behavior of NetworkChangeNotifier::GetConnectionType > be > > in the case of multiple interfaces? > > The correct behavior of NetworkChangeNotifier::GetConnectionType is to return > the type of the first hop of the default internet connection. In the case of > TMobile cellular with two interfaces, NetworkChangeNotifier::GetConnectionType > should return one of the cellular types. > > > > > This past reverted CL > > (https://codereview.chromium.org/298023005/patch/130001/140001) suggests that > we > > should return the connection type if every network interface has the same > > connection type or otherwise return CONNECTION_UNKNOWN. Should > > address_tracker_linux preform something similar? > > GetNetworkList() (which uses AddressTrackerLinux on several hosts) should > accurately determine the type of each interface if possible. > NetworkChangeNotifier should should compute the type of the first hop of the > default internet connection. If all results from GetNetworkList() are of the > same type I think it is safe to assume the default internet connection is of > this type too.
On 2014/12/01 18:58:34, derekjchow1 wrote: > Hi Paul, how do the following changes sound? > > 1) Modify AddressTrackerLinux to monitor changes to the default route. > GetConnectionType will return the connection type associated with the default > route. This should return the default interface used to connect to the internet. There may be multiple default routes and default interfaces. For example TMobile uses two interfaces and two default routes (one IPv4 and one IPv6). > > 2) Add some functions to create a map of interface names to connection types > (ie. "em0" is an ethernet interface or "eth0" is a 4G connection). > GetInterfaceConnectionType will then look up this map to determine connection > type if it does not find wireless extensions. For most cases, the map will be > empty and the connection type will be unknown. However, for situations where the > properties of network interfaces of the system are well known (such as > Chromebook case given) we can return the connection type from the map. The Chromebook case I gave is by no means well known, it's more of a corner case I happen to know about, though I don't understand the motivation or methodology for why em0 is 4G. What is your motivation for trying to fix this issue? Is there a particular variety of linux you want this fixed for (e.g. cast)? Maybe we could start with a simpler subset of this issue so we have a smaller problem space to attack; for example if all interfaces that are UP have the SIOCGIWNAME extension than we might be able to assume it's WiFi.
> What is your motivation for trying to fix this issue? Is there a particular > variety of linux you want this fixed for (e.g. cast)? The particular issue I'm trying to address is a situation where an ethernet and wifi connection are both present. When the ethernet cable is plugged in, GetCurrentConnectionType should return ETHERNET since it is the interface used to connect to the internet. Otherwise, the wifi connection is being used and it should return WIFI. I'm trying to fix this for cast, but it's also applicable to other situations such as my home laptop where it uses ethernet if the cable is plugged in and wifi if it isn't. > Maybe we could start with > a simpler subset of this issue so we have a smaller problem space to attack; for > example if all interfaces that are UP have the SIOCGIWNAME extension than we > might be able to assume it's WiFi. I don't think this works. A connection is still UP even if it is unconnected or unassociated. Just as a quick example, my current workstation has two ethernet ports eth1 and em1, but only eth1 has a cable plugged into it. Running "ifconfig" lists both interfaces as UP. For cast there are ethernet and wifi interfaces UP at the same time. The above strategy will always return UNKNOWN in this case as there is an ethernet and wifi connection in the connection list, even if only one of them is used to connect to the internet. > There may be multiple default routes and default interfaces. For example > TMobile uses two interfaces and two default routes (one IPv4 and one IPv6). Is two the maximum number of default routes? If so why don't we check the connection types of both routes and return the connection type if they are the same. If they are different we can return UNKNOWN.
On 2014/12/01 22:24:37, derekjchow1 wrote: > > What is your motivation for trying to fix this issue? Is there a particular > > variety of linux you want this fixed for (e.g. cast)? > > The particular issue I'm trying to address is a situation where an ethernet and > wifi connection are both present. When the ethernet cable is plugged in, > GetCurrentConnectionType should return ETHERNET since it is the interface used > to connect to the internet. Otherwise, the wifi connection is being used and it > should return WIFI. I'm trying to fix this for cast, but it's also applicable to > other situations such as my home laptop where it uses ethernet if the cable is > plugged in and wifi if it isn't. I think determining the correct answer for all linux distributions is going to be an extremely hard task. You'd have to write code to analyse all the routing rules, routing tables, individual routes, and iptables routing. I think limiting this work to a smaller problem would make things feasible. Some possibilities: 1. If all UP interfaces with IP addresses are of the same type, we can infer the connection type from the interface types. 2. The O/S is likely the one controlling all the routing rules and tables. The O/S likely provides a simple interface for getting the default network type. This is what we did for Android and ChromeOS. > > > Maybe we could start with > > a simpler subset of this issue so we have a smaller problem space to attack; > for > > example if all interfaces that are UP have the SIOCGIWNAME extension than we > > might be able to assume it's WiFi. > > I don't think this works. A connection is still UP even if it is unconnected or > unassociated. Just as a quick example, my current workstation has two ethernet > ports eth1 and em1, but only eth1 has a cable plugged into it. Running > "ifconfig" lists both interfaces as UP. For cast there are ethernet and wifi > interfaces UP at the same time. The above strategy will always return UNKNOWN in > this case as there is an ethernet and wifi connection in the connection list, > even if only one of them is used to connect to the internet. What about limiting it to interfaces with IP addresses? Can you post the "ip addr" output? > > > There may be multiple default routes and default interfaces. For example > > TMobile uses two interfaces and two default routes (one IPv4 and one IPv6). > > Is two the maximum number of default routes? If so why don't we check the > connection types of both routes and return the connection type if they are the > same. If they are different we can return UNKNOWN. As I tried to explain in answering your other question, I think this would be an extremely tricky endeavor. In Android Lollipop we now use many routing rules with many routing tables and we mark (SO_MARK) all sockets so without knowledge of how the system sets up marks there is virtually no chance of determining the default connection type by looking at routing rules or tables.
> What about limiting it to interfaces with IP addresses? Can you post the "ip > addr" output? mlan0 UP 192.168.0.106/24 0x00001043 b0:ee:45:49:af:6c lo UP 127.0.0.1/8 0x00000049 00:00:00:00:00:00 uap0 DOWN 0.0.0.0/0 0x00001002 00:1a:11:ff:af:6c wfd0 DOWN 0.0.0.0/0 0x00001002 b2:ee:45:49:af:6c eth1 UP 172.17.21.134/24 0x00001043 00:14:d1:da:c4:66 eth0 UP 0.0.0.0/0 0x00001003 00:5a:50:c4:a2:ee Both Wifi and Ethernet have IP addresses assigned. > 2. The O/S is likely the one controlling all the routing rules and tables. The > O/S likely provides a simple interface for getting the default network type. > This is what we did for Android and ChromeOS. I'll try digging into this a little further. All initial results suggest using the interface associated with the default route. Let me know if you have other ideas and I'd be happy to try them out.
Sorry for the long delay Paul, I have yet to find generic linux solutions for determining the default network interface or for determining if an interface is a ethernet connection (Android has android.net.ConnectivityManager and ChromeOS uses Shill). Consequently, I don't think there is a way to rework AddressTrackerLinux/NetworkChangeNotifierLinux to eliminate the need for a cast specific NetworkChangeNotifier. In the meantime I've implemented your suggestion to check all interfaces with a valid assigned IP address and return CONNECTION_WIFI if they are all wifi connections. This may be of use for the Network Information API on Chrome Linux (http://w3c.github.io/netinfo/#dfn-connection-types). PTAL.
https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:18: #include <arpa/inet.h> Shouldn't system headers go above Chromium header includes? https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:396: networks.empty()) I feel like this needs curly braces as the condition is longer than one line. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:399: for (unsigned int i = 1; i < networks.size(); i++) { Can this code be moved to a host-independent location? For example: https://codereview.chromium.org/298023005/diff/130001/net/base/network_change... https://codereview.chromium.org/298023005/diff/130001/net/base/network_change... https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:86: // in |interface_index|. Please explain |ifname| argument. e.g. what size should it be and how it relates to the return value. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:122: NetworkChangeNotifier::ConnectionType GetNewConnectionType(); I think "New" in the name is confusing, can we replace that with "Current"? https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:144: bool is_initialized_; Can you rename this to be more precise? i.e. is_current_connection_type_initialized_ https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:145: base::ConditionVariable is_initialized_cv_; Ditto. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:9: #include <sys/ioctl.h> I think this should be moved below <set> for alphabetical reasons https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:76: strncpy(buf, "", IFNAMSIZ); Can we remove this strncpy and add a memset(buf, 0, IFNAMSIZ) at the beginning of the function? https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:85: strncpy(buf, "", IFNAMSIZ); Ditto. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:88: strncpy(buf, ifr.ifr_name, IFNAMSIZ); This could result in an unterminated string causing other parts of the code to read off the end of the arrays. If we memset() the string to 0's at the beginning of the function I think we should make this only copy IFNAMSIZ-1 bytes. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:21: // Returns the name for the interface with interface index |interface_index|. This should mention how |buf| relates to the return value. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:30: NetworkChangeNotifier::ConnectionType GetInterfaceConnectionType( This header should include "net/base/network_change_notifier.h" https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:31: std::string ifname); This header should include <string> https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:31: std::string ifname); I think this should be a const reference, not passed by value.
https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:18: #include <arpa/inet.h> On 2015/01/13 13:14:29, pauljensen wrote: > Shouldn't system headers go above Chromium header includes? Done. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:396: networks.empty()) On 2015/01/13 13:14:29, pauljensen wrote: > I feel like this needs curly braces as the condition is longer than one line. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.cc:399: for (unsigned int i = 1; i < networks.size(); i++) { On 2015/01/13 13:14:29, pauljensen wrote: > Can this code be moved to a host-independent location? For example: > https://codereview.chromium.org/298023005/diff/130001/net/base/network_change... > https://codereview.chromium.org/298023005/diff/130001/net/base/network_change... Added "ConnectionTypeFromInterfaceList" to net_util.h. I also added a new unit test (based on this https://codereview.chromium.org/298023005/patch/130001/140003). https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:86: // in |interface_index|. On 2015/01/13 13:14:29, pauljensen wrote: > Please explain |ifname| argument. e.g. what size should it be and how it relates > to the return value. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:122: NetworkChangeNotifier::ConnectionType GetNewConnectionType(); On 2015/01/13 13:14:30, pauljensen wrote: > I think "New" in the name is confusing, can we replace that with "Current"? I changed the function to UpdateCurrentConnectionType (GetCurrentConnectionType is already defined above). https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:144: bool is_initialized_; On 2015/01/13 13:14:30, pauljensen wrote: > Can you rename this to be more precise? i.e. > is_current_connection_type_initialized_ Done. https://codereview.chromium.org/739983005/diff/40001/net/base/address_tracker... net/base/address_tracker_linux.h:145: base::ConditionVariable is_initialized_cv_; On 2015/01/13 13:14:29, pauljensen wrote: > Ditto. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:9: #include <sys/ioctl.h> On 2015/01/13 13:14:30, pauljensen wrote: > I think this should be moved below <set> for alphabetical reasons Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:76: strncpy(buf, "", IFNAMSIZ); On 2015/01/13 13:14:30, pauljensen wrote: > Can we remove this strncpy and add a memset(buf, 0, IFNAMSIZ) at the beginning > of the function? Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:85: strncpy(buf, "", IFNAMSIZ); On 2015/01/13 13:14:30, pauljensen wrote: > Ditto. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.cc:88: strncpy(buf, ifr.ifr_name, IFNAMSIZ); On 2015/01/13 13:14:30, pauljensen wrote: > This could result in an unterminated string causing other parts of the code to > read off the end of the arrays. If we memset() the string to 0's at the > beginning of the function I think we should make this only copy IFNAMSIZ-1 > bytes. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:21: // Returns the name for the interface with interface index |interface_index|. On 2015/01/13 13:14:31, pauljensen wrote: > This should mention how |buf| relates to the return value. Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:30: NetworkChangeNotifier::ConnectionType GetInterfaceConnectionType( On 2015/01/13 13:14:31, pauljensen wrote: > This header should include "net/base/network_change_notifier.h" Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:31: std::string ifname); On 2015/01/13 13:14:31, pauljensen wrote: > This header should include <string> Done. https://codereview.chromium.org/739983005/diff/40001/net/base/net_util_linux.... net/base/net_util_linux.h:31: std::string ifname); On 2015/01/13 13:14:30, pauljensen wrote: > I think this should be a const reference, not passed by value. Done.
Please run some trybots on the change. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:16: #include "net/base/net_util_linux.h" net_util_linux.h#8 says it's not for general purpose use. Can you move GetInterfaceName back into this file and remove the GetInterfaceConnectionType declaration (as I don't see it used in multiple files)? https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:264: if (*link_changed || *address_changed) { doesn't need curly braces anymore https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:395: } if GetNetworkListImpl fails we should fall back to the old behavior of setting the type based on online_links.empty() https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.h:88: typedef char* (*GetInterfaceNameFunction)(unsigned int interface_index, Let's put this back to "int" as ifi_index and ifr_ifindex are both int's. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux_unittest.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux_unittest.cc:27: strncpy(buf, "tun0", IFNAMSIZ); I think this needs curly braces, but instead of doing that, why not put it back to code more like the original: if (...) return strncpy(...); return strncpy(...); https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1040: NetworkChangeNotifier::ConnectionType ConnectionTypeFromInterfaceList( Please move to network_change_notifier.cc https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1044: Please rework this to ignore the terredo interface on windows as the copy I linked to did. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:504: ConnectionTypeFromInterfaceList(const NetworkInterfaceList& interfaces); Please move this to network_change_notifier.h as it's really only appropriate for NetworkChangeNotifier implementations. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:880: TEST(NetUtilTest, ConnectionTypeFromInterfaceList) { please move to network_change_notifier_unittest.cc https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:887: NetworkInterface interface; please move inside the loop https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:889: i < NetworkChangeNotifier::CONNECTION_LAST; i++) { < should be <= https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:897: j < NetworkChangeNotifier::CONNECTION_LAST; j++) { < should be <=
I'm a fairly new Chromium developer, so I don't have try-bot access yet (just sent in the trybot form today). https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:16: #include "net/base/net_util_linux.h" On 2015/01/15 15:05:10, pauljensen wrote: > net_util_linux.h#8 says it's not for general purpose use. Can you move > GetInterfaceName back into this file and remove the GetInterfaceConnectionType > declaration (as I don't see it used in multiple files)? Done. We use GetInterfaceName as an argument to GetNetworkListImpl, so I've redeclared it as a public static function in AddressTrackerLinux. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:264: if (*link_changed || *address_changed) { On 2015/01/15 15:05:10, pauljensen wrote: > doesn't need curly braces anymore Done. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.cc:395: } On 2015/01/15 15:05:10, pauljensen wrote: > if GetNetworkListImpl fails we should fall back to the old behavior of setting > the type based on online_links.empty() Done. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux.h:88: typedef char* (*GetInterfaceNameFunction)(unsigned int interface_index, On 2015/01/15 15:05:10, pauljensen wrote: > Let's put this back to "int" as ifi_index and ifr_ifindex are both int's. Done. Also changed GetInterfaceNameFunction in net_util_linux to int as well. https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... File net/base/address_tracker_linux_unittest.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/address_tracker... net/base/address_tracker_linux_unittest.cc:27: strncpy(buf, "tun0", IFNAMSIZ); On 2015/01/15 15:05:10, pauljensen wrote: > I think this needs curly braces, but instead of doing that, why not put it back > to code more like the original: > if (...) > return strncpy(...); > return strncpy(...); Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1040: NetworkChangeNotifier::ConnectionType ConnectionTypeFromInterfaceList( On 2015/01/15 15:05:10, pauljensen wrote: > Please move to network_change_notifier.cc Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1044: On 2015/01/15 15:05:10, pauljensen wrote: > Please rework this to ignore the terredo interface on windows as the copy I > linked to did. Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:504: ConnectionTypeFromInterfaceList(const NetworkInterfaceList& interfaces); On 2015/01/15 15:05:10, pauljensen wrote: > Please move this to network_change_notifier.h as it's really only appropriate > for NetworkChangeNotifier implementations. Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:880: TEST(NetUtilTest, ConnectionTypeFromInterfaceList) { On 2015/01/15 15:05:10, pauljensen wrote: > please move to network_change_notifier_unittest.cc Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:887: NetworkInterface interface; On 2015/01/15 15:05:10, pauljensen wrote: > please move inside the loop Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:889: i < NetworkChangeNotifier::CONNECTION_LAST; i++) { On 2015/01/15 15:05:10, pauljensen wrote: > < should be <= Done. https://codereview.chromium.org/739983005/diff/60001/net/base/net_util_unitte... net/base/net_util_unittest.cc:897: j < NetworkChangeNotifier::CONNECTION_LAST; j++) { On 2015/01/15 15:05:10, pauljensen wrote: > < should be <= Done.
Ping? I ran a few trybots all but one seem successful (linux_chromium_trusty32_rel is failing with some flaking tests).
Sorry, I got pulled onto other things and have been sheriffing the last two days. I'll try and take a more thorough look later this week when I'm not sheriffing. https://codereview.chromium.org/739983005/diff/80001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/80001/net/base/address_tracker... net/base/address_tracker_linux.h:70: // pointer why such a short line? https://codereview.chromium.org/739983005/diff/80001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/80001/net/base/net_util_linux.... net/base/net_util_linux.h:16: #include "net/base/network_change_notifier.h" This isn't needed anymore.
https://codereview.chromium.org/739983005/diff/80001/net/base/address_tracker... File net/base/address_tracker_linux.h (right): https://codereview.chromium.org/739983005/diff/80001/net/base/address_tracker... net/base/address_tracker_linux.h:70: // pointer On 2015/01/26 18:09:35, pauljensen wrote: > why such a short line? Done. https://codereview.chromium.org/739983005/diff/80001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/80001/net/base/net_util_linux.... net/base/net_util_linux.h:16: #include "net/base/network_change_notifier.h" On 2015/01/26 18:09:35, pauljensen wrote: > This isn't needed anymore. Done.
Can you adjust this piece of the description: "Assume ethernet otherwise." What does "Also adds option to ignore netif." mean? Can you remove the Change-Id from the description too? https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:16: #include "net/base/net_util_linux.h" unused? https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:17: #include "net/base/net_util_posix.h" unused? https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:85: char* AddressTrackerLinux::GetInterfaceName(int interface_index, char* buf) { put "// static" on the previous line before static function definitions. https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux... net/base/net_util_linux.cc:91: type = NetworkChangeNotifier::CONNECTION_UNKNOWN; I think this else clause can be deleted https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux... net/base/net_util_linux.h:11: #include <string> unused?
Description updated. https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:16: #include "net/base/net_util_linux.h" On 2015/01/27 20:25:17, pauljensen wrote: > unused? It's used to call GetNetworkListImpl. https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:17: #include "net/base/net_util_posix.h" On 2015/01/27 20:25:17, pauljensen wrote: > unused? Done. https://codereview.chromium.org/739983005/diff/100001/net/base/address_tracke... net/base/address_tracker_linux.cc:85: char* AddressTrackerLinux::GetInterfaceName(int interface_index, char* buf) { On 2015/01/27 20:25:17, pauljensen wrote: > put "// static" on the previous line before static function definitions. Done. https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux... net/base/net_util_linux.cc:91: type = NetworkChangeNotifier::CONNECTION_UNKNOWN; On 2015/01/27 20:25:17, pauljensen wrote: > I think this else clause can be deleted Done. https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/100001/net/base/net_util_linux... net/base/net_util_linux.h:11: #include <string> On 2015/01/27 20:25:17, pauljensen wrote: > unused? Done.
https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux... net/base/net_util_linux.cc:90: Could we use SIOCETHTOOL to identify ethernet devices? "ethtool" seems to be able to differentiate eth0 and wlan0 appropriately. "strace ethtool" shows it using SIOCETHTOOL.
https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux... net/base/net_util_linux.h:9: // of net_util_linux.cc to tests. This comment needs to be updated now that this is also included into address_tracker_linux.cc
https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux... net/base/net_util_linux.cc:90: On 2015/01/28 00:27:53, pauljensen wrote: > Could we use SIOCETHTOOL to identify ethernet devices? "ethtool" seems to be > able to differentiate eth0 and wlan0 appropriately. "strace ethtool" shows it > using SIOCETHTOOL. Nice. Done. I tested SIOCETHTOOL last night on my laptop and a small computer with USB wifi. Seems to work to distinguish ethernet. See this demo I used to test: https://gist.github.com/anonymous/7bb9dbb089924f3800f7 https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux.h File net/base/net_util_linux.h (right): https://codereview.chromium.org/739983005/diff/120001/net/base/net_util_linux... net/base/net_util_linux.h:9: // of net_util_linux.cc to tests. On 2015/01/28 15:14:46, pauljensen wrote: > This comment needs to be updated now that this is also included into > address_tracker_linux.cc Done.
https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... net/base/address_tracker_linux.cc:85: int ioctl_socket = socket(AF_INET, SOCK_DGRAM, 0); Can we use a ScopedFD here to remove the need for the close(ioctl_socket) line (and so we don't forget it). https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... net/base/address_tracker_linux.cc:404: base::hash_set<int> online_links = GetOnlineLinks(); I tried your tool out on three different linux machines and it seemed to work nicely. It did think a tun0 interface was ethernet on both a computer with only ethernet and a computer with only wifi. This is incorrect. Technically a tun interface could be using some non-networking method of sending/receiving data (e.g. smoke signals or slightly more feasibly: a camera (to receive data) and a display (to send data)), but I think we should only concern ourselves with network interface based communications, so I think we can ignore tun interfaces here. I think can be done easily enough by: 1. reworking IsTunnelInterface() to take an interface index rather than an ifinfomsg. 2. iterating through online_links and removing anything that satisfies IsTunnelInterface https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:93: struct ethtool_cmd ecmd; This struct needs initialization; you can simply declare with " = {}", please do the same for |pwrq| and |ifr| for consistency; " = {}" is a less code than the memset(). https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:97: ifr.ifr_data = (void*)&ecmd; I don't think a cast is necessary when going to void*. https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:104: close(s); Please use a ScopedFD so we can remove all the close(s) calls (and so we don't forget one).
https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... net/base/address_tracker_linux.cc:85: int ioctl_socket = socket(AF_INET, SOCK_DGRAM, 0); On 2015/01/28 19:05:04, pauljensen wrote: > Can we use a ScopedFD here to remove the need for the close(ioctl_socket) line > (and so we don't forget it). Done. https://codereview.chromium.org/739983005/diff/140001/net/base/address_tracke... net/base/address_tracker_linux.cc:404: base::hash_set<int> online_links = GetOnlineLinks(); On 2015/01/28 19:05:04, pauljensen wrote: > I tried your tool out on three different linux machines and it seemed to work > nicely. > It did think a tun0 interface was ethernet on both a computer with only ethernet > and a computer with only wifi. This is incorrect. > Technically a tun interface could be using some non-networking method of > sending/receiving data (e.g. smoke signals or slightly more feasibly: a camera > (to receive data) and a display (to send data)), but I think we should only > concern ourselves with network interface based communications, so I think we can > ignore tun interfaces here. > I think can be done easily enough by: > 1. reworking IsTunnelInterface() to take an interface index rather than an > ifinfomsg. > 2. iterating through online_links and removing anything that satisfies > IsTunnelInterface Done. https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:93: struct ethtool_cmd ecmd; On 2015/01/28 19:05:04, pauljensen wrote: > This struct needs initialization; you can simply declare with " = {}", please do > the same for |pwrq| and |ifr| for consistency; " = {}" is a less code than the > memset(). Done. https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:97: ifr.ifr_data = (void*)&ecmd; On 2015/01/28 19:05:04, pauljensen wrote: > I don't think a cast is necessary when going to void*. Done. https://codereview.chromium.org/739983005/diff/140001/net/base/net_util_linux... net/base/net_util_linux.cc:104: close(s); On 2015/01/28 19:05:04, pauljensen wrote: > Please use a ScopedFD so we can remove all the close(s) calls (and so we don't > forget one). Done.
https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:90: static struct ifreq ifr; um this shouldn't be static...that would make this function thread-unsafe https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:91: memset(&ifr, 0, sizeof(ifr)); could we combine this line into the above line using " = {}" https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:92: ifr.ifr_ifindex = interface_index; could we combine this line into the above declaration using " = { .ifr_ifindex = interface_index }; https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:98: return buf; could we combine the last 6 lines of this function down to: if (ioctl(...) == 0) strncpy(buf, ...) return buf; https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:411: online_links.erase(it); this won't work: |it| becomes invalid after erase() call. I think we can use the new C++11 return value for erase() like so: for (base::hash_set<int>::const_iterator it = online_links.begin(); it != online_links.end();) { if (IsTunnelInterface(*it)) { it = online_links.erase(it); } else { ++it; } https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:86: memset(&pwrq, 0, sizeof(pwrq)); this line is now unnecessary https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:93: ecmd.cmd = ETHTOOL_GSET; how about combining this line into the previous line like so: struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET }; https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:95: ifr.ifr_data = &ecmd; ditto like so: struct ifreq ifr = { .ifr_data = &emd };
https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:90: static struct ifreq ifr; On 2015/01/28 21:15:55, pauljensen wrote: > um this shouldn't be static...that would make this function thread-unsafe Done. https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:91: memset(&ifr, 0, sizeof(ifr)); On 2015/01/28 21:15:55, pauljensen wrote: > could we combine this line into the above line using " = {}" Done. https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:92: ifr.ifr_ifindex = interface_index; On 2015/01/28 21:15:55, pauljensen wrote: > could we combine this line into the above declaration using " = { .ifr_ifindex = > interface_index }; Done. https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:98: return buf; On 2015/01/28 21:15:55, pauljensen wrote: > could we combine the last 6 lines of this function down to: > if (ioctl(...) == 0) > strncpy(buf, ...) > return buf; Done. https://codereview.chromium.org/739983005/diff/160001/net/base/address_tracke... net/base/address_tracker_linux.cc:411: online_links.erase(it); On 2015/01/28 21:15:55, pauljensen wrote: > this won't work: |it| becomes invalid after erase() call. > I think we can use the new C++11 return value for erase() like so: > for (base::hash_set<int>::const_iterator it = online_links.begin(); > it != online_links.end();) { > if (IsTunnelInterface(*it)) { > it = online_links.erase(it); > } else { > ++it; > } https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ha... Only MSVC uses a C++11 implementation of base::hash_set, so this won't work on linux. To work around this, I used a temporary iterator instead. PTAL. If you don't think this works I will create a new hash_set with the non-tunnel interfaces instead. https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:86: memset(&pwrq, 0, sizeof(pwrq)); On 2015/01/28 21:15:55, pauljensen wrote: > this line is now unnecessary Done. https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:93: ecmd.cmd = ETHTOOL_GSET; On 2015/01/28 21:15:55, pauljensen wrote: > how about combining this line into the previous line like so: > struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET }; Done. https://codereview.chromium.org/739983005/diff/160001/net/base/net_util_linux... net/base/net_util_linux.cc:95: ifr.ifr_data = &ecmd; On 2015/01/28 21:15:55, pauljensen wrote: > ditto like so: > struct ifreq ifr = { .ifr_data = &emd }; Done.
https://codereview.chromium.org/739983005/diff/180001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/180001/net/base/net_util_linux... net/base/net_util_linux.cc:144: ifname.assign(get_interface_name(it->second.ifa_index, buffer)); You removed the write to ifnames; please restore.
https://codereview.chromium.org/739983005/diff/180001/net/base/net_util_linux.cc File net/base/net_util_linux.cc (right): https://codereview.chromium.org/739983005/diff/180001/net/base/net_util_linux... net/base/net_util_linux.cc:144: ifname.assign(get_interface_name(it->second.ifa_index, buffer)); On 2015/01/29 14:24:57, pauljensen wrote: > You removed the write to ifnames; please restore. Done.
lgtm. Please this a fair bit before CQing. To test you can build net_watcher and run it. Make sure it works on Ubuntu 14.04 with wifi and ethernet. https://codereview.chromium.org/739983005/diff/200001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/200001/net/base/address_tracke... net/base/address_tracker_linux.cc:398: NetworkChangeNotifier::ConnectionType type = nit: move |type| declaration down next to |networks|
Test with my home PC running 14.04 with both ethernet and PCI wifi card. Works as expected: Ethernet only: CONNECTION_ETHERNET Ethernet with unassociated wifi: CONNECTION_ETHERNET Wifi connected, cable unplugged: CONNECTION_WIFI Both ethernet and wifi connected: CONNECTION_UNKNOWN Cable unplugged, wifi disassociated: CONNECTION_NONE Also tested on Chromecast using ifconfig to bring up/down networks. Results are the same. https://codereview.chromium.org/739983005/diff/200001/net/base/address_tracke... File net/base/address_tracker_linux.cc (right): https://codereview.chromium.org/739983005/diff/200001/net/base/address_tracke... net/base/address_tracker_linux.cc:90: struct ifreq ifr = {.ifr_ifindex = interface_index}; I built net_watcher for Chromecast to test this change and noticed it brought up the following compiler error: ../../net/base/address_tracker_linux.cc: In static member function 'static char* net::internal::AddressTrackerLinux::GetInterfaceName(int, char*)': ../../net/base/address_tracker_linux.cc:90:23: error: expected primary-expression before '.' token struct ifreq ifr = {.ifr_ifindex = interface_index}; Further investigation explained that designated initializers are not supported by GCC for C++11 (we use gcc4.8/4.9). I changed this here and in net_util_linux to keep the CL compatible with both clang and gcc. https://codereview.chromium.org/739983005/diff/200001/net/base/address_tracke... net/base/address_tracker_linux.cc:398: NetworkChangeNotifier::ConnectionType type = On 2015/01/29 19:40:12, pauljensen wrote: > nit: move |type| declaration down next to |networks| Done.
The CQ bit was checked by derekjchow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739983005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Hmm, looks like ethtool.h isn't included until Android NDK version 21 (Lollipop) which I assume we're not yet using to build Chrome. I'd say just remove the ethtool.h stuff with "#if !defined(OS_ANDROID)" The STL error is another story. Perhaps this is invoking the C++11 erase() function because you pass in a const_iterator; you could try changing to a simple iterator, perhaps this won't try to return the value from the inner function.
On 2015/01/30 12:30:55, pauljensen wrote: > Hmm, looks like ethtool.h isn't included until Android NDK version 21 (Lollipop) > which I assume we're not yet using to build Chrome. I'd say just remove the > ethtool.h stuff with "#if !defined(OS_ANDROID)" > Done. > The STL error is another story. Perhaps this is invoking the C++11 erase() > function because you pass in a const_iterator; you could try changing to a > simple iterator, perhaps this won't try to return the value from the inner > function. The non-const iterator didn't build either. Calling erase on *tunnel_it works fine for both platforms though.
The CQ bit was checked by derekjchow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739983005/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/5482d5e52c66f72082ee243464d0e109b66419a9 Cr-Commit-Position: refs/heads/master@{#314042} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
