diff options
| -rw-r--r-- | manager.cc | 60 | ||||
| -rw-r--r-- | manager.h | 9 | ||||
| -rw-r--r-- | manager_unittest.cc | 117 | ||||
| -rw-r--r-- | mock_manager.h | 3 | ||||
| -rw-r--r-- | mock_service.h | 1 | ||||
| -rw-r--r-- | profile.cc | 15 | ||||
| -rw-r--r-- | profile.h | 5 | ||||
| -rw-r--r-- | profile_dbus_adaptor.cc | 7 | ||||
| -rw-r--r-- | profile_unittest.cc | 49 |
9 files changed, 237 insertions, 29 deletions
@@ -264,19 +264,11 @@ void Manager::PopProfileInternal() { CHECK(!profiles_.empty()); ProfileRefPtr active_profile = profiles_.back(); profiles_.pop_back(); - vector<ServiceRefPtr>::iterator s_it; - for (s_it = services_.begin(); s_it != services_.end(); ++s_it) { - if ((*s_it)->profile().get() == active_profile.get()) { - vector<ProfileRefPtr>::reverse_iterator p_it; - for (p_it = profiles_.rbegin(); p_it != profiles_.rend(); ++p_it) { - if ((*p_it)->ConfigureService(*s_it)) { - break; - } - } - if (p_it == profiles_.rend()) { - ephemeral_profile_->AdoptService(*s_it); - (*s_it)->Unload(); - } + vector<ServiceRefPtr>::iterator it; + for (it = services_.begin(); it != services_.end(); ++it) { + if ((*it)->profile().get() == active_profile.get() && + !MatchProfileWithService(*it)) { + (*it)->Unload(); } } SortServices(); @@ -311,6 +303,23 @@ void Manager::PopAnyProfile(Error *error) { PopProfileInternal(); } +bool Manager::HandleProfileEntryDeletion(const ProfileRefPtr &profile, + const std::string &entry_name) { + bool moved_services = false; + for (vector<ServiceRefPtr>::iterator it = services_.begin(); + it != services_.end(); ++it) { + if ((*it)->profile().get() == profile.get() && + (*it)->GetStorageIdentifier() == entry_name) { + profile->AbandonService(*it); + if (!MatchProfileWithService(*it)) { + (*it)->Unload(); + } + moved_services = true; + } + } + return moved_services; +} + const ProfileRefPtr &Manager::ActiveProfile() const { DCHECK_NE(profiles_.size(), 0); return profiles_.back(); @@ -421,16 +430,7 @@ void Manager::RegisterService(const ServiceRefPtr &to_manage) { VLOG(2) << "In " << __func__ << "(): Registering service " << to_manage->UniqueName(); - bool configured = false; - for (vector<ProfileRefPtr>::reverse_iterator it = profiles_.rbegin(); - !configured && it != profiles_.rend(); - ++it) { - configured = (*it)->ConfigureService(to_manage); - } - - // If not found, add it to the ephemeral profile - if (!configured) - ephemeral_profile_->AdoptService(to_manage); + MatchProfileWithService(to_manage); // Now add to OUR list. vector<ServiceRefPtr>::iterator it; @@ -549,6 +549,20 @@ void Manager::SortServices() { AutoConnect(); } +bool Manager::MatchProfileWithService(const ServiceRefPtr &service) { + vector<ProfileRefPtr>::reverse_iterator it; + for (it = profiles_.rbegin(); it != profiles_.rend(); ++it) { + if ((*it)->ConfigureService(service)) { + break; + } + } + if (it == profiles_.rend()) { + ephemeral_profile_->AdoptService(service); + return false; + } + return true; +} + void Manager::AutoConnect() { // We might be called in the middle of another request (e.g., as a // consequence of Service::SetState calling UpdateService). To avoid @@ -102,6 +102,13 @@ class Manager { void PopProfile(const std::string &name, Error *error); // Remove the active profile. void PopAnyProfile(Error *error); + // Handle the event where a profile is about to remove a profile entry. + // Any Services that are dependent on this storage identifier will need + // to find new profiles. Return true if any service has been moved to a new + // profile. Any such services will have had the profile group removed from + // the profile. + virtual bool HandleProfileEntryDeletion(const ProfileRefPtr &profile, + const std::string &entry_name); virtual DeviceInfo *device_info() { return &device_info_; } ModemInfo *modem_info() { return &modem_info_; } @@ -125,7 +132,6 @@ class Manager { FRIEND_TEST(ManagerTest, DefaultTechnology); FRIEND_TEST(ManagerTest, DeviceRegistrationAndStart); FRIEND_TEST(ManagerTest, EnumerateProfiles); - FRIEND_TEST(ManagerTest, PushPopProfile); FRIEND_TEST(ManagerTest, SortServices); FRIEND_TEST(ManagerTest, SortServicesWithConnection); @@ -156,6 +162,7 @@ class Manager { void PopProfileInternal(); bool OrderServices(ServiceRefPtr a, ServiceRefPtr b); void SortServices(); + bool MatchProfileWithService(const ServiceRefPtr &service); EventDispatcher *dispatcher_; ScopedRunnableMethodFactory<Manager> task_factory_; diff --git a/manager_unittest.cc b/manager_unittest.cc index 004ee624..63a3d8e3 100644 --- a/manager_unittest.cc +++ b/manager_unittest.cc @@ -116,6 +116,10 @@ class ManagerTest : public PropertyStoreTest { manager->profiles_.push_back(profile); } + ProfileRefPtr GetEphemeralProfile(Manager *manager) { + return manager->ephemeral_profile_; + } + Profile *CreateProfileForManager(Manager *manager, GLib *glib) { Profile::Identifier id("rather", "irrelevant"); scoped_ptr<Profile> profile(new Profile(control_interface(), @@ -613,7 +617,7 @@ TEST_F(ManagerTest, PushPopProfile) { // Add this service to the manager -- it should end up in the ephemeral // profile. manager.RegisterService(service); - ASSERT_EQ(manager.ephemeral_profile_, service->profile()); + ASSERT_EQ(GetEphemeralProfile(&manager), service->profile()); // Create storage for a profile that contains the service storage name. ASSERT_TRUE(CreateBackingStoreForService(&temp_dir, kProfile2Id, @@ -623,7 +627,7 @@ TEST_F(ManagerTest, PushPopProfile) { // ephemeral profile to this new profile since it has an entry for // this service. EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile2)); - EXPECT_NE(manager.ephemeral_profile_, service->profile()); + EXPECT_NE(GetEphemeralProfile(&manager), service->profile()); EXPECT_EQ(kProfile2, "~" + service->profile()->GetFriendlyName()); // Insert another profile that should supersede ownership of the service. @@ -650,7 +654,7 @@ TEST_F(ManagerTest, PushPopProfile) { EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager)); // The service should now revert to the ephemeral profile. - EXPECT_EQ(manager.ephemeral_profile_, service->profile()); + EXPECT_EQ(GetEphemeralProfile(&manager), service->profile()); // Pop the remaining two services off the stack. EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager)); @@ -660,6 +664,113 @@ TEST_F(ManagerTest, PushPopProfile) { EXPECT_EQ(Error::kNotFound, TestPopAnyProfile(&manager)); } +// Use this matcher instead of passing RefPtrs directly into the arguments +// of EXPECT_CALL() because otherwise we may create un-cleaned-up references at +// system teardown. +MATCHER_P(IsRefPtrTo, ref_address, "") { + return arg.get() == ref_address; +} + +TEST_F(ManagerTest, HandleProfileEntryDeletion) { + MockServiceRefPtr s_not_in_profile( + new NiceMock<MockService>(control_interface(), + dispatcher(), + metrics(), + manager())); + MockServiceRefPtr s_not_in_group( + new NiceMock<MockService>(control_interface(), + dispatcher(), + metrics(), + manager())); + MockServiceRefPtr s_configure_fail( + new NiceMock<MockService>(control_interface(), + dispatcher(), + metrics(), + manager())); + MockServiceRefPtr s_configure_succeed( + new NiceMock<MockService>(control_interface(), + dispatcher(), + metrics(), + manager())); + + string entry_name("entry_name"); + EXPECT_CALL(*s_not_in_profile.get(), GetStorageIdentifier()).Times(0); + EXPECT_CALL(*s_not_in_group.get(), GetStorageIdentifier()) + .WillRepeatedly(Return("not_entry_name")); + EXPECT_CALL(*s_configure_fail.get(), GetStorageIdentifier()) + .WillRepeatedly(Return(entry_name)); + EXPECT_CALL(*s_configure_succeed.get(), GetStorageIdentifier()) + .WillRepeatedly(Return(entry_name)); + + manager()->RegisterService(s_not_in_profile); + manager()->RegisterService(s_not_in_group); + manager()->RegisterService(s_configure_fail); + manager()->RegisterService(s_configure_succeed); + + scoped_refptr<MockProfile> profile0( + new StrictMock<MockProfile>(control_interface(), manager(), "")); + scoped_refptr<MockProfile> profile1( + new StrictMock<MockProfile>(control_interface(), manager(), "")); + + s_not_in_group->set_profile(profile1); + s_configure_fail->set_profile(profile1); + s_configure_succeed->set_profile(profile1); + + AdoptProfile(manager(), profile0); + AdoptProfile(manager(), profile1); + + // No services are a member of this profile. + EXPECT_FALSE(manager()->HandleProfileEntryDeletion(profile0, entry_name)); + + // No services that are members of this profile have this entry name. + EXPECT_FALSE(manager()->HandleProfileEntryDeletion(profile1, "")); + + // Only services that are members of the profile and group will be abandoned. + EXPECT_CALL(*profile1.get(), + AbandonService(IsRefPtrTo(s_not_in_profile.get()))).Times(0); + EXPECT_CALL(*profile1.get(), + AbandonService(IsRefPtrTo(s_not_in_group.get()))).Times(0); + EXPECT_CALL(*profile1.get(), + AbandonService(IsRefPtrTo(s_configure_fail.get()))) + .WillOnce(Return(true)); + EXPECT_CALL(*profile1.get(), + AbandonService(IsRefPtrTo(s_configure_succeed.get()))) + .WillOnce(Return(true)); + + // Never allow services to re-join profile1. + EXPECT_CALL(*profile1.get(), ConfigureService(_)) + .WillRepeatedly(Return(false)); + + // Only allow one of the members of the profile and group to successfully + // join profile0. + EXPECT_CALL(*profile0.get(), + ConfigureService(IsRefPtrTo(s_not_in_profile.get()))).Times(0); + EXPECT_CALL(*profile0.get(), + ConfigureService(IsRefPtrTo(s_not_in_group.get()))).Times(0); + EXPECT_CALL(*profile0.get(), + ConfigureService(IsRefPtrTo(s_configure_fail.get()))) + .WillOnce(Return(false)); + EXPECT_CALL(*profile0.get(), + ConfigureService(IsRefPtrTo(s_configure_succeed.get()))) + .WillOnce(Return(true)); + + // Expect the failed-to-configure service to have Unload() called on it. + EXPECT_CALL(*s_not_in_profile.get(), Unload()).Times(0); + EXPECT_CALL(*s_not_in_group.get(), Unload()).Times(0); + EXPECT_CALL(*s_configure_fail.get(), Unload()).Times(1); + EXPECT_CALL(*s_configure_succeed.get(), Unload()).Times(0); + + EXPECT_TRUE(manager()->HandleProfileEntryDeletion(profile1, entry_name)); + + EXPECT_EQ(GetEphemeralProfile(manager()), s_not_in_profile->profile().get()); + EXPECT_EQ(profile1, s_not_in_group->profile()); + EXPECT_EQ(GetEphemeralProfile(manager()), s_configure_fail->profile()); + + // Since we are using a MockProfile, the profile does not actually change, + // since ConfigureService was not actually called on the service. + EXPECT_EQ(profile1, s_configure_succeed->profile()); +} + TEST_F(ManagerTest, Dispatch) { { ::DBus::Error error; diff --git a/mock_manager.h b/mock_manager.h index d59097e6..557c988e 100644 --- a/mock_manager.h +++ b/mock_manager.h @@ -27,6 +27,9 @@ class MockManager : public Manager { MOCK_METHOD1(RegisterService, void(const ServiceRefPtr &to_manage)); MOCK_METHOD1(UpdateService, void(const ServiceRefPtr &to_update)); MOCK_METHOD1(DeregisterService, void(const ServiceRefPtr &to_forget)); + MOCK_METHOD2(HandleProfileEntryDeletion, + bool (const ProfileRefPtr &profile, + const std::string &entry_name)); private: DISALLOW_COPY_AND_ASSIGN(MockManager); diff --git a/mock_service.h b/mock_service.h index ce3bb89a..a56ab432 100644 --- a/mock_service.h +++ b/mock_service.h @@ -43,6 +43,7 @@ class MockService : public Service { MOCK_CONST_METHOD0(GetRpcIdentifier, std::string()); MOCK_CONST_METHOD0(GetStorageIdentifier, std::string()); MOCK_METHOD1(Load, bool(StoreInterface *store_interface)); + MOCK_METHOD0(Unload, void()); MOCK_METHOD1(Save, bool(StoreInterface *store_interface)); MOCK_METHOD1(SetConnection, void(ConnectionRefPtr connection)); MOCK_CONST_METHOD0(technology, Technology::Identifier()); @@ -149,6 +149,21 @@ bool Profile::ContainsService(const ServiceConstRefPtr &service) { return service->IsLoadableFrom(storage_.get()); } +void Profile::DeleteEntry(const std::string &entry_name, Error *error) { + if (!storage_->ContainsGroup(entry_name)) { + Error::PopulateAndLog(error, Error::kNotFound, + base::StringPrintf("Entry %s does not exist in profile", + entry_name.c_str())); + return; + } + if (!manager_->HandleProfileEntryDeletion(this, entry_name)) { + // If HandleProfileEntryDeletion() returns succeeds, DeleteGroup() + // has already been called when AbandonService was called. + // Otherwise, we need to delete the group ourselves. + storage_->DeleteGroup(entry_name); + } +} + bool Profile::IsValidIdentifierToken(const string &token) { if (token.empty()) { return false; @@ -91,6 +91,10 @@ class Profile : public base::RefCounted<Profile> { // return false. virtual bool ConfigureDevice(const DeviceRefPtr &device); + // Remove a named entry from the profile. This includes detaching + // any service that uses this profile entry. + virtual void DeleteEntry(const std::string &entry_name, Error *error); + // Return whether |service| can configure itself from the profile. bool ContainsService(const ServiceConstRefPtr &service); @@ -125,6 +129,7 @@ class Profile : public base::RefCounted<Profile> { private: friend class ProfileAdaptorInterface; + FRIEND_TEST(ProfileTest, DeleteEntry); FRIEND_TEST(ProfileTest, GetStoragePath); FRIEND_TEST(ProfileTest, IsValidIdentifierToken); diff --git a/profile_dbus_adaptor.cc b/profile_dbus_adaptor.cc index 13b8f598..42f7d1cf 100644 --- a/profile_dbus_adaptor.cc +++ b/profile_dbus_adaptor.cc @@ -76,8 +76,11 @@ map<string, ::DBus::Variant> ProfileDBusAdaptor::GetEntry( return map<string, ::DBus::Variant>(); } -void ProfileDBusAdaptor::DeleteEntry(const std::string& /*name*/, - ::DBus::Error &/*error*/) { +void ProfileDBusAdaptor::DeleteEntry(const std::string &name, + ::DBus::Error &error) { + Error e; + profile_->DeleteEntry(name, &e); + e.ToDBusError(&error); } } // namespace shill diff --git a/profile_unittest.cc b/profile_unittest.cc index 9a1fa7c4..3514a97c 100644 --- a/profile_unittest.cc +++ b/profile_unittest.cc @@ -14,6 +14,7 @@ #include "shill/glib.h" #include "shill/key_file_store.h" +#include "shill/mock_manager.h" #include "shill/mock_profile.h" #include "shill/mock_service.h" #include "shill/mock_store.h" @@ -25,6 +26,7 @@ using std::string; using std::vector; using testing::_; using testing::Invoke; +using testing::Mock; using testing::Return; using testing::SetArgumentPointee; using testing::StrictMock; @@ -75,6 +77,53 @@ class ProfileTest : public PropertyStoreTest { ProfileRefPtr profile_; }; +TEST_F(ProfileTest, DeleteEntry) { + scoped_ptr<MockManager> manager(new StrictMock<MockManager>( + control_interface(), dispatcher(), metrics(), glib())); + profile_->manager_ = manager.get(); + + MockStore *storage(new StrictMock<MockStore>()); + profile_->storage_.reset(storage); // Passes ownership + const string kEntryName("entry_name"); + + // If entry does not appear in storage, DeleteEntry() should return an error. + EXPECT_CALL(*storage, ContainsGroup(kEntryName)) + .WillOnce(Return(false)); + { + Error error; + profile_->DeleteEntry(kEntryName, &error); + EXPECT_EQ(Error::kNotFound, error.type()); + } + + Mock::VerifyAndClearExpectations(storage); + EXPECT_CALL(*storage, ContainsGroup(kEntryName)) + .WillRepeatedly(Return(true)); + + // If HandleProfileEntryDeletion() returns false, Profile should call + // DeleteGroup() itself. + EXPECT_CALL(*manager.get(), HandleProfileEntryDeletion(_, kEntryName)) + .WillOnce(Return(false)); + EXPECT_CALL(*storage, DeleteGroup(kEntryName)) + .WillOnce(Return(true)); + { + Error error; + profile_->DeleteEntry(kEntryName, &error); + EXPECT_TRUE(error.IsSuccess()); + } + + // If HandleProfileEntryDeletion() returns true, Profile should not call + // DeleteGroup() itself. + EXPECT_CALL(*manager.get(), HandleProfileEntryDeletion(_, kEntryName)) + .WillOnce(Return(true)); + EXPECT_CALL(*storage, DeleteGroup(kEntryName)) + .Times(0); + { + Error error; + profile_->DeleteEntry(kEntryName, &error); + EXPECT_TRUE(error.IsSuccess()); + } +} + TEST_F(ProfileTest, IsValidIdentifierToken) { EXPECT_FALSE(Profile::IsValidIdentifierToken("")); EXPECT_FALSE(Profile::IsValidIdentifierToken(" ")); |