shill: Implement DeleteEntry Profile method
Implement the DeleteEntry DBus method call, which removes
a Profile entry as well as detaching any connected services
from the profile data. As a bonus change, modify Manager to
consolidate finding a Profile that is suitable for a Service
which does not have one.
BUG=chromium-os:25542
TEST=New unit tests
Change-Id: I6a954a41ab2d1b49f6432858e2263a63b5af21f1
Reviewed-on: https://gerrit.chromium.org/gerrit/14944
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 e743d8b..de2de61 100644
--- a/manager.cc
+++ b/manager.cc
@@ -264,19 +264,11 @@
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 @@
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 @@
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 @@
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
diff --git a/manager.h b/manager.h
index f61d4ce..a6ff900 100644
--- a/manager.h
+++ b/manager.h
@@ -102,6 +102,13 @@
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 @@
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 @@
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 004ee62..63a3d8e 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -116,6 +116,10 @@
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 @@
// 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 @@
// 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 @@
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 @@
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 d59097e..557c988 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -27,6 +27,9 @@
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 ce3bb89..a56ab43 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -43,6 +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_METHOD1(Save, bool(StoreInterface *store_interface));
MOCK_METHOD1(SetConnection, void(ConnectionRefPtr connection));
MOCK_CONST_METHOD0(technology, Technology::Identifier());
diff --git a/profile.cc b/profile.cc
index 8ff5ff0..fba86df 100644
--- a/profile.cc
+++ b/profile.cc
@@ -149,6 +149,21 @@
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;
diff --git a/profile.h b/profile.h
index cf0b199..2df6cf6 100644
--- a/profile.h
+++ b/profile.h
@@ -91,6 +91,10 @@
// 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 @@
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 13b8f59..42f7d1c 100644
--- a/profile_dbus_adaptor.cc
+++ b/profile_dbus_adaptor.cc
@@ -76,8 +76,11 @@
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 9a1fa7c..3514a97 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::vector;
using testing::_;
using testing::Invoke;
+using testing::Mock;
using testing::Return;
using testing::SetArgumentPointee;
using testing::StrictMock;
@@ -75,6 +77,53 @@
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(" "));