shill: Clean up Service::Remove()
This method used to leave the service in an inconsistent state
(unloading the service while still linking it with a profile that
contained its configuration). This CL adds a real implementation
which disassociates the service from its current profile and
unloads the service if no other profile supports it. It also
supports removing the service from the Manager's list if the
service should no longer exist.
BUG=chromium:281655
TEST=Unit tests + manual:
Call RemoveService on a visible, remembered service and note that
the service is unloaded and no longer associated with a profile.
Call RemoveService on a remembered but non-visible service and
note that the service is unloaded, the configuration no longer
exists and the service is removed from the Manager ServiceCompleteList.
Change-Id: I985c4498b5acf55b8f3d78ee570d30597a2c5384
Reviewed-on: https://chromium-review.googlesource.com/167530
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 9cfb5c4..8fe7bec 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -1117,6 +1117,57 @@
}
}
+TEST_F(ManagerTest, RemoveService) {
+ MockServiceRefPtr mock_service(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager()));
+
+ // Used in expectations which cannot accept a mock refptr.
+ const ServiceRefPtr &service = mock_service;
+
+ manager()->RegisterService(service);
+ EXPECT_EQ(GetEphemeralProfile(manager()), service->profile().get());
+
+ scoped_refptr<MockProfile> profile(
+ new StrictMock<MockProfile>(
+ control_interface(), metrics(), manager(), ""));
+ AdoptProfile(manager(), profile);
+
+ // If service is ephemeral, it should be unloaded and left ephemeral.
+ EXPECT_CALL(*profile, AbandonService(service)).Times(0);
+ EXPECT_CALL(*profile, ConfigureService(service)).Times(0);
+ EXPECT_CALL(*mock_service, Unload()).WillOnce(Return(false));
+ manager()->RemoveService(service);
+ Mock::VerifyAndClearExpectations(mock_service);
+ Mock::VerifyAndClearExpectations(profile);
+ EXPECT_EQ(GetEphemeralProfile(manager()), service->profile().get());
+ EXPECT_TRUE(manager()->HasService(service)); // Since Unload() was false.
+
+ // If service is not ephemeral and the Manager finds a profile to assign
+ // the service to, the service should be re-parented. Note that since we
+ // are using a MockProfile, ConfigureService() never actually changes the
+ // Service's profile.
+ service->set_profile(profile);
+ EXPECT_CALL(*profile, AbandonService(service));
+ EXPECT_CALL(*profile, ConfigureService(service)).WillOnce(Return(true));
+ EXPECT_CALL(*mock_service, Unload()).Times(0);
+ manager()->RemoveService(service);
+ Mock::VerifyAndClearExpectations(mock_service);
+ Mock::VerifyAndClearExpectations(profile);
+ EXPECT_TRUE(manager()->HasService(service));
+ EXPECT_EQ(profile.get(), service->profile().get());
+
+ // If service becomes ephemeral since there is no profile to support it,
+ // it should be unloaded.
+ EXPECT_CALL(*profile, AbandonService(service));
+ EXPECT_CALL(*profile, ConfigureService(service)).WillOnce(Return(false));
+ EXPECT_CALL(*mock_service, Unload()).WillOnce(Return(true));
+ manager()->RemoveService(service);
+ EXPECT_FALSE(manager()->HasService(service));
+}
+
TEST_F(ManagerTest, CreateDuplicateProfileWithMissingKeyfile) {
// It's much easier to use real Glib in creating a Manager for this
// test here since we want the storage side-effects.