From 4bbd3c83a80e99c86ece75847c67970de46ff138 Mon Sep 17 00:00:00 2001 From: "guoweis@webrtc.org" Date: Tue, 9 Sep 2014 13:54:45 +0000 Subject: [PATCH] fix a bug in the logic when new Networks are merged. This happens when we have 2 networks with the same key BUG=410554 in chromium http://code.google.com/p/chromium/issues/detail?id=410554 Corresponding change in chromium is https://codereview.chromium.org/536133003/ R=jiayl@webrtc.org Review URL: https://webrtc-codereview.appspot.com/19249005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7117 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/base/network.cc | 36 +++++++++++++++++++++++---------- webrtc/base/network_unittest.cc | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/webrtc/base/network.cc b/webrtc/base/network.cc index d94c69eae..686de974d 100644 --- a/webrtc/base/network.cc +++ b/webrtc/base/network.cc @@ -65,6 +65,11 @@ const int kNetworksUpdateIntervalMs = 2000; const int kHighestNetworkPreference = 127; +typedef struct { + Network* net; + std::vector ips; +} AddressList; + bool CompareNetworks(const Network* a, const Network* b) { if (a->prefix_length() == b->prefix_length()) { if (a->name() == b->name()) { @@ -144,10 +149,12 @@ void NetworkManagerBase::GetNetworks(NetworkList* result) const { void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, bool* changed) { - // Sort the list so that we can detect when it changes. - typedef std::pair > address_list; - std::map address_map; + // AddressList in this map will track IP addresses for all Networks + // with the same key. + std::map consolidated_address_list; NetworkList list(new_networks); + + // Result of Network merge. Element in this list should have unique key. NetworkList merged_list; std::sort(list.begin(), list.end(), CompareNetworks); @@ -162,16 +169,19 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, std::string key = MakeNetworkKey(list[i]->name(), list[i]->prefix(), list[i]->prefix_length()); - if (address_map.find(key) == address_map.end()) { - address_map[key] = address_list(list[i], std::vector()); + if (consolidated_address_list.find(key) == + consolidated_address_list.end()) { + AddressList addrlist; + addrlist.net = list[i]; + consolidated_address_list[key] = addrlist; might_add_to_merged_list = true; } const std::vector& addresses = list[i]->GetIPs(); - address_list& current_list = address_map[key]; + AddressList& current_list = consolidated_address_list[key]; for (std::vector::const_iterator it = addresses.begin(); it != addresses.end(); ++it) { - current_list.second.push_back(*it); + current_list.ips.push_back(*it); } if (!might_add_to_merged_list) { delete list[i]; @@ -179,20 +189,24 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, } // Next, look for existing network objects to re-use. - for (std::map::iterator it = address_map.begin(); - it != address_map.end(); + for (std::map::iterator it = + consolidated_address_list.begin(); + it != consolidated_address_list.end(); ++it) { const std::string& key = it->first; - Network* net = it->second.first; + Network* net = it->second.net; NetworkMap::iterator existing = networks_map_.find(key); if (existing == networks_map_.end()) { // This network is new. Place it in the network map. merged_list.push_back(net); networks_map_[key] = net; + // Also, we might have accumulated IPAddresses from the first + // step, set it here. + net->SetIPs(it->second.ips, true); *changed = true; } else { // This network exists in the map already. Reset its IP addresses. - *changed = existing->second->SetIPs(it->second.second, *changed); + *changed = existing->second->SetIPs(it->second.ips, *changed); merged_list.push_back(existing->second); if (existing->second != net) { delete net; diff --git a/webrtc/base/network_unittest.cc b/webrtc/base/network_unittest.cc index 431f8b4ea..8123f8bb6 100644 --- a/webrtc/base/network_unittest.cc +++ b/webrtc/base/network_unittest.cc @@ -614,4 +614,40 @@ TEST_F(NetworkTest, TestIgnoreNonDefaultRoutes) { } #endif +// Test MergeNetworkList successfully combines all IPs for the same +// prefix/length into a single Network. +TEST_F(NetworkTest, TestMergeNetworkList) { + BasicNetworkManager manager; + NetworkManager::NetworkList list; + + // Create 2 IPAddress classes with only last digit different. + IPAddress ip1, ip2; + EXPECT_TRUE(IPFromString("2400:4030:1:2c00:be30:0:0:1", &ip1)); + EXPECT_TRUE(IPFromString("2400:4030:1:2c00:be30:0:0:2", &ip2)); + + // Create 2 networks with the same prefix and length. + Network* net1 = new Network("em1", "em1", TruncateIP(ip1, 64), 64); + Network* net2 = new Network("em1", "em1", TruncateIP(ip1, 64), 64); + + // Add different IP into each. + net1->AddIP(ip1); + net2->AddIP(ip2); + + list.push_back(net1); + list.push_back(net2); + bool changed; + MergeNetworkList(manager, list, &changed); + EXPECT_TRUE(changed); + + NetworkManager::NetworkList list2; + manager.GetNetworks(&list2); + + // Make sure the resulted networklist has only 1 element and 2 + // IPAddresses. + EXPECT_EQ(list2.size(), 1uL); + EXPECT_EQ(list2[0]->GetIPs().size(), 2uL); + EXPECT_EQ(list2[0]->GetIPs()[0], ip1); + EXPECT_EQ(list2[0]->GetIPs()[1], ip2); +} + } // namespace rtc