shill: vpn: Destroy Unload()ed VPN services
BUG=chromium-os:28481
TEST=New unit test.
Change-Id: I222441d3ff5cbc7d97d97f7fdd2917eee5921721
Reviewed-on: https://gerrit.chromium.org/gerrit/19143
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Ready: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/manager.cc b/manager.cc
index 11677f1..d9fc96c 100644
--- a/manager.cc
+++ b/manager.cc
@@ -294,10 +294,12 @@
ProfileRefPtr active_profile = profiles_.back();
profiles_.pop_back();
vector<ServiceRefPtr>::iterator it;
- for (it = services_.begin(); it != services_.end(); ++it) {
- if ((*it)->profile().get() == active_profile.get() &&
- !MatchProfileWithService(*it)) {
- (*it)->Unload();
+ for (it = services_.begin(); it != services_.end();) {
+ if ((*it)->profile().get() != active_profile.get() ||
+ MatchProfileWithService(*it) ||
+ !UnloadService(&it)) {
+ LOG(ERROR) << "Skipping unload of service";
+ ++it;
}
}
SortServices();
@@ -336,14 +338,17 @@
const std::string &entry_name) {
bool moved_services = false;
for (vector<ServiceRefPtr>::iterator it = services_.begin();
- it != services_.end(); ++it) {
+ it != services_.end();) {
if ((*it)->profile().get() == profile.get() &&
(*it)->GetStorageIdentifier() == entry_name) {
profile->AbandonService(*it);
- if (!MatchProfileWithService(*it)) {
- (*it)->Unload();
+ if (MatchProfileWithService(*it) ||
+ !UnloadService(&it)) {
+ ++it;
}
moved_services = true;
+ } else {
+ ++it;
}
}
return moved_services;
@@ -598,6 +603,17 @@
}
}
+bool Manager::UnloadService(vector<ServiceRefPtr>::iterator *service_iterator) {
+ if (!(**service_iterator)->Unload()) {
+ return false;
+ }
+
+ DCHECK(!(**service_iterator)->connection());
+ *service_iterator = services_.erase(*service_iterator);
+
+ return true;
+}
+
void Manager::UpdateService(const ServiceRefPtr &to_update) {
CHECK(to_update);
LOG(INFO) << "Service " << to_update->UniqueName() << " updated;"
diff --git a/manager.h b/manager.h
index ac2f4d2..6f632ee 100644
--- a/manager.h
+++ b/manager.h
@@ -182,6 +182,8 @@
FRIEND_TEST(ManagerTest, DefaultTechnology);
FRIEND_TEST(ManagerTest, DeviceRegistrationAndStart);
FRIEND_TEST(ManagerTest, EnumerateProfiles);
+ FRIEND_TEST(ManagerTest, HandleProfileEntryDeletionWithUnload);
+ FRIEND_TEST(ManagerTest, PopProfileWithUnload);
FRIEND_TEST(ManagerTest, SortServices);
FRIEND_TEST(ManagerTest, SortServicesWithConnection);
@@ -204,6 +206,12 @@
std::vector<std::string> EnumerateWatchedServices(Error *error);
std::string GetActiveProfileRpcIdentifier(Error *error);
+ // Unload a service while iterating through |services_|. Returns true if
+ // service was erased (which means the caller loop should not increment
+ // |service_iterator|), false otherwise (meaning the caller should
+ // increment |service_iterator|).
+ bool UnloadService(std::vector<ServiceRefPtr>::iterator *service_iterator);
+
void HelpRegisterConstDerivedRpcIdentifiers(
const std::string &name,
RpcIdentifiers(Manager::*get)(Error *));
diff --git a/manager_unittest.cc b/manager_unittest.cc
index c7b05f0..afc2fc8 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -802,6 +802,161 @@
EXPECT_EQ(profile1, s_configure_succeed->profile());
}
+TEST_F(ManagerTest, HandleProfileEntryDeletionWithUnload) {
+ MockServiceRefPtr s_will_remove0(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_remove1(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_not_remove0(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_not_remove1(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+
+ EXPECT_CALL(*metrics(), NotifyDefaultServiceChanged(NULL))
+ .Times(4); // Once for each registration.
+
+ string entry_name("entry_name");
+ EXPECT_CALL(*s_will_remove0.get(), GetStorageIdentifier())
+ .WillRepeatedly(Return(entry_name));
+ EXPECT_CALL(*s_will_remove1.get(), GetStorageIdentifier())
+ .WillRepeatedly(Return(entry_name));
+ EXPECT_CALL(*s_will_not_remove0.get(), GetStorageIdentifier())
+ .WillRepeatedly(Return(entry_name));
+ EXPECT_CALL(*s_will_not_remove1.get(), GetStorageIdentifier())
+ .WillRepeatedly(Return(entry_name));
+
+ manager()->RegisterService(s_will_remove0);
+ manager()->RegisterService(s_will_not_remove0);
+ manager()->RegisterService(s_will_remove1);
+ manager()->RegisterService(s_will_not_remove1);
+
+ // One for each service added above.
+ ASSERT_EQ(4, manager()->services_.size());
+
+ scoped_refptr<MockProfile> profile(
+ new StrictMock<MockProfile>(control_interface(), manager(), ""));
+
+ s_will_remove0->set_profile(profile);
+ s_will_remove1->set_profile(profile);
+ s_will_not_remove0->set_profile(profile);
+ s_will_not_remove1->set_profile(profile);
+
+ AdoptProfile(manager(), profile);
+
+ // Deny any of the services re-entry to the profile.
+ EXPECT_CALL(*profile, ConfigureService(_))
+ .WillRepeatedly(Return(false));
+
+ EXPECT_CALL(*profile, AbandonService(ServiceRefPtr(s_will_remove0)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*profile, AbandonService(ServiceRefPtr(s_will_remove1)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*profile, AbandonService(ServiceRefPtr(s_will_not_remove0)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*profile, AbandonService(ServiceRefPtr(s_will_not_remove1)))
+ .WillOnce(Return(true));
+
+ EXPECT_CALL(*s_will_remove0, Unload())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*s_will_remove1, Unload())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*s_will_not_remove0, Unload())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*s_will_not_remove1, Unload())
+ .WillOnce(Return(false));
+
+
+ // This will cause all the profiles to be unloaded.
+ EXPECT_TRUE(manager()->HandleProfileEntryDeletion(profile, entry_name));
+
+ // 2 of the 4 services added above should have been unregistered and
+ // removed, leaving 2.
+ EXPECT_EQ(2, manager()->services_.size());
+ EXPECT_EQ(s_will_not_remove0.get(), manager()->services_[0].get());
+ EXPECT_EQ(s_will_not_remove1.get(), manager()->services_[1].get());
+}
+
+TEST_F(ManagerTest, PopProfileWithUnload) {
+ MockServiceRefPtr s_will_remove0(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_remove1(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_not_remove0(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+ MockServiceRefPtr s_will_not_remove1(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+
+ EXPECT_CALL(*metrics(), NotifyDefaultServiceChanged(NULL))
+ .Times(5); // Once for each registration, and one after profile pop.
+
+ manager()->RegisterService(s_will_remove0);
+ manager()->RegisterService(s_will_not_remove0);
+ manager()->RegisterService(s_will_remove1);
+ manager()->RegisterService(s_will_not_remove1);
+
+ // One for each service added above.
+ ASSERT_EQ(4, manager()->services_.size());
+
+ scoped_refptr<MockProfile> profile0(
+ new StrictMock<MockProfile>(control_interface(), manager(), ""));
+ scoped_refptr<MockProfile> profile1(
+ new StrictMock<MockProfile>(control_interface(), manager(), ""));
+
+ s_will_remove0->set_profile(profile1);
+ s_will_remove1->set_profile(profile1);
+ s_will_not_remove0->set_profile(profile1);
+ s_will_not_remove1->set_profile(profile1);
+
+ AdoptProfile(manager(), profile0);
+ AdoptProfile(manager(), profile1);
+
+ // Deny any of the services entry to profile0, so they will all be unloaded.
+ EXPECT_CALL(*profile0, ConfigureService(_))
+ .WillRepeatedly(Return(false));
+
+ EXPECT_CALL(*s_will_remove0, Unload())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*s_will_remove1, Unload())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*s_will_not_remove0, Unload())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*s_will_not_remove1, Unload())
+ .WillOnce(Return(false));
+
+ // This will pop profile1, which should cause all our profiles to unload.
+ manager()->PopProfileInternal();
+
+ // 2 of the 4 services added above should have been unregistered and
+ // removed, leaving 2.
+ EXPECT_EQ(2, manager()->services_.size());
+ EXPECT_EQ(s_will_not_remove0.get(), manager()->services_[0].get());
+ EXPECT_EQ(s_will_not_remove1.get(), manager()->services_[1].get());
+}
+
TEST_F(ManagerTest, SetProperty) {
{
::DBus::Error error;
diff --git a/mock_service.h b/mock_service.h
index 760b3eb..bfcc01a 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -43,7 +43,7 @@
MOCK_CONST_METHOD0(GetRpcIdentifier, std::string());
MOCK_CONST_METHOD0(GetStorageIdentifier, std::string());
MOCK_METHOD1(Load, bool(StoreInterface *store_interface));
- MOCK_METHOD0(Unload, void());
+ MOCK_METHOD0(Unload, bool());
MOCK_METHOD1(Save, bool(StoreInterface *store_interface));
MOCK_METHOD0(SaveToCurrentProfile, bool());
MOCK_METHOD2(Configure, void(const KeyValueStore &args, Error *error));
diff --git a/mock_vpn_service.h b/mock_vpn_service.h
index 4e6b2b1..41d8fe0 100644
--- a/mock_vpn_service.h
+++ b/mock_vpn_service.h
@@ -22,6 +22,7 @@
MOCK_METHOD1(SetState, void(ConnectState state));
MOCK_METHOD0(InitDriverPropertyStore, void());
+ MOCK_CONST_METHOD0(unloaded, bool());
private:
DISALLOW_COPY_AND_ASSIGN(MockVPNService);
diff --git a/service.cc b/service.cc
index b5e8ad2..0f9bb70 100644
--- a/service.cc
+++ b/service.cc
@@ -315,7 +315,7 @@
return true;
}
-void Service::Unload() {
+bool Service::Unload() {
auto_connect_ = false;
check_portal_ = kCheckPortalAuto;
favorite_ = false;
@@ -325,6 +325,7 @@
ui_data_ = "";
UnloadEapCredentials();
+ return false;
}
bool Service::Save(StoreInterface *storage) {
diff --git a/service.h b/service.h
index 80d3e9c..dea45a1 100644
--- a/service.h
+++ b/service.h
@@ -206,8 +206,10 @@
virtual bool Load(StoreInterface *storage);
// Indicate to service that it is no longer persisted to storage. It
- // should purge any stored profile state (e.g., credentials).
- virtual void Unload();
+ // should purge any stored profile state (e.g., credentials). Returns
+ // true to indicate that this service should also be unregistered from
+ // the manager, false otherwise.
+ virtual bool Unload();
// Saves the service to persistent |storage|. Returns true on success.
virtual bool Save(StoreInterface *storage);
@@ -387,6 +389,7 @@
void SetEAPKeyManagement(const std::string &key_management);
void SetEAPPassword(const std::string &password, Error *error);
void SetEAPPrivateKeyPassword(const std::string &password, Error *error);
+ Manager *manager() const { return manager_; }
Metrics *metrics() const { return metrics_; }
private:
diff --git a/vpn_provider.cc b/vpn_provider.cc
index b9daf1a..13c4464 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -4,6 +4,8 @@
#include "shill/vpn_provider.h"
+#include <algorithm>
+
#include <base/logging.h>
#include <chromeos/dbus/service_constants.h>
@@ -84,4 +86,12 @@
return false;
}
+void VPNProvider::RemoveService(VPNServiceRefPtr service) {
+ vector<VPNServiceRefPtr>::iterator it;
+ it = std::find(services_.begin(), services_.end(), service);
+ if (it != services_.end()) {
+ services_.erase(it);
+ }
+}
+
} // namespace shill
diff --git a/vpn_provider.h b/vpn_provider.h
index f4b532c..c968b1e 100644
--- a/vpn_provider.h
+++ b/vpn_provider.h
@@ -39,8 +39,14 @@
// device has been accepted by a service.
bool OnDeviceInfoAvailable(const std::string &link_name, int interface_index);
+ // Clean up a VPN services that has been unloaded and will be deregistered.
+ // This removes the VPN provider's reference to this service in its
+ // services_ vector.
+ void RemoveService(VPNServiceRefPtr service);
+
private:
FRIEND_TEST(VPNProviderTest, OnDeviceInfoAvailable);
+ FRIEND_TEST(VPNProviderTest, RemoveService);
ControlInterface *control_interface_;
EventDispatcher *dispatcher_;
diff --git a/vpn_provider_unittest.cc b/vpn_provider_unittest.cc
index e65bff8..d2604db 100644
--- a/vpn_provider_unittest.cc
+++ b/vpn_provider_unittest.cc
@@ -14,7 +14,7 @@
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
#include "shill/mock_vpn_driver.h"
-#include "shill/vpn_service.h"
+#include "shill/mock_vpn_service.h"
using std::string;
using testing::_;
@@ -112,4 +112,33 @@
provider_.services_.clear();
}
+TEST_F(VPNProviderTest, RemoveService) {
+ scoped_refptr<MockVPNService> service0(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+ scoped_refptr<MockVPNService> service1(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+ scoped_refptr<MockVPNService> service2(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+
+ provider_.services_.push_back(service0.get());
+ provider_.services_.push_back(service1.get());
+ provider_.services_.push_back(service2.get());
+
+ ASSERT_EQ(3, provider_.services_.size());
+
+ provider_.RemoveService(service1);
+
+ EXPECT_EQ(2, provider_.services_.size());
+ EXPECT_EQ(service0, provider_.services_[0]);
+ EXPECT_EQ(service2, provider_.services_[1]);
+
+ provider_.RemoveService(service2);
+
+ EXPECT_EQ(1, provider_.services_.size());
+ EXPECT_EQ(service0, provider_.services_[0]);
+
+ provider_.RemoveService(service0);
+ EXPECT_EQ(0, provider_.services_.size());
+}
+
} // namespace shill
diff --git a/vpn_service.cc b/vpn_service.cc
index 7e99962..2154808 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -11,6 +11,7 @@
#include <chromeos/dbus/service_constants.h>
#include "shill/key_value_store.h"
+#include "shill/manager.h"
#include "shill/technology.h"
#include "shill/vpn_driver.h"
@@ -80,6 +81,19 @@
driver_->Save(storage, GetStorageIdentifier());
}
+bool VPNService::Unload() {
+ Service::Unload();
+
+ // VPN services which have been removed from the profile should be
+ // disconnected.
+ driver_->Disconnect();
+
+ // Ask the VPN provider to remove us from its list.
+ manager()->vpn_provider()->RemoveService(this);
+
+ return true;
+}
+
void VPNService::InitDriverPropertyStore() {
driver_->InitPropertyStore(mutable_store());
}
diff --git a/vpn_service.h b/vpn_service.h
index 2b45b61..6466a57 100644
--- a/vpn_service.h
+++ b/vpn_service.h
@@ -30,6 +30,7 @@
virtual std::string GetStorageIdentifier() const;
virtual bool Load(StoreInterface *storage);
virtual bool Save(StoreInterface *storage);
+ virtual bool Unload();
virtual void InitDriverPropertyStore();
diff --git a/wifi_service.cc b/wifi_service.cc
index 2faa452..aa59bf6 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -307,7 +307,7 @@
return true;
}
-void WiFiService::Unload() {
+bool WiFiService::Unload() {
Service::Unload();
hidden_ssid_ = false;
passphrase_ = "";
@@ -323,6 +323,7 @@
// for a service changes. crosbug.com/25670
wifi_->ClearCachedCredentials();
}
+ return !IsVisible();
}
bool WiFiService::IsSecurityMatch(const string &security) const {
diff --git a/wifi_service.h b/wifi_service.h
index 33c78c6..a98ed9d 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -81,7 +81,7 @@
virtual bool IsLoadableFrom(StoreInterface *storage) const;
virtual bool Load(StoreInterface *storage);
virtual bool Save(StoreInterface *storage);
- virtual void Unload();
+ virtual bool Unload();
virtual bool HasEndpoints() const { return !endpoints_.empty(); }
virtual bool IsVisible() const;