shill: Add Profile and Manager UpdateDevice support.
UpdateDevice persists Devices into profiles. It's similar to UpdateService,
however, only the topmost DefaultProfile handles UpdateDevice. Use UpdateDevice
to persist Device's Enabled and Cellular's AllowRoaming properties. Remove now
unused Manager::SaveActiveProfile method to avoid confusion.
BUG=chrome-os-partner:10178,chromium-os:32230
TEST=unit tests
Change-Id: I1d293fa84e0bce3156943e8e9d313fc98facbde1
Reviewed-on: https://gerrit.chromium.org/gerrit/26405
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Ready: Ben Chan <benchan@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index e7659fe..889caf8 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -575,7 +575,7 @@
return;
}
allow_roaming_ = value;
- manager()->SaveActiveProfile();
+ manager()->UpdateDevice(this);
// Use AllowRoaming() instead of allow_roaming_ in order to
// incorporate provider preferences when evaluating if a disconnect
diff --git a/cellular.h b/cellular.h
index e60e39b..babf933 100644
--- a/cellular.h
+++ b/cellular.h
@@ -223,6 +223,7 @@
FRIEND_TEST(CellularTest, Disconnect);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
FRIEND_TEST(CellularTest, ModemStateChangeEnable);
+ FRIEND_TEST(CellularTest, SetAllowRoaming);
FRIEND_TEST(CellularTest, StartModemCallback);
FRIEND_TEST(CellularTest, StartModemCallbackFail);
FRIEND_TEST(CellularTest, StopModemCallback);
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index 156873c..c276362 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -772,4 +772,13 @@
dispatcher_.DispatchPendingEvents();
}
+TEST_F(CellularTest, SetAllowRoaming) {
+ EXPECT_FALSE(device_->allow_roaming_);
+ EXPECT_CALL(manager_, UpdateDevice(_));
+ Error error;
+ device_->SetAllowRoaming(true, &error);
+ EXPECT_TRUE(error.IsSuccess());
+ EXPECT_TRUE(device_->allow_roaming_);
+}
+
} // namespace shill
diff --git a/default_profile.cc b/default_profile.cc
index 1f7ce71..91ea6e9 100644
--- a/default_profile.cc
+++ b/default_profile.cc
@@ -132,6 +132,10 @@
return Profile::Save();
}
+bool DefaultProfile::UpdateDevice(const DeviceRefPtr &device) {
+ return device->Save(storage()) && storage()->Flush();
+}
+
bool DefaultProfile::GetStoragePath(FilePath *path) {
*path = storage_path_.Append(base::StringPrintf("%s.profile",
profile_id_.c_str()));
diff --git a/default_profile.h b/default_profile.h
index d87450a..4c3091f 100644
--- a/default_profile.h
+++ b/default_profile.h
@@ -45,6 +45,9 @@
// Returns true on success, false on failure.
virtual bool Save();
+ // Inherited from Profile.
+ virtual bool UpdateDevice(const DeviceRefPtr &device);
+
protected:
// Sets |path| to the persistent store file path for the default, global
// profile. Returns true on success, and false if unable to determine an
diff --git a/default_profile_unittest.cc b/default_profile_unittest.cc
index d3319f1..9fb583e 100644
--- a/default_profile_unittest.cc
+++ b/default_profile_unittest.cc
@@ -265,4 +265,15 @@
EXPECT_TRUE(profile_->ConfigureService(ethernet_service));
}
+TEST_F(DefaultProfileTest, UpdateDevice) {
+ scoped_ptr<MockStore> storage(new MockStore());
+ EXPECT_CALL(*storage, Flush()).WillOnce(Return(true));
+ EXPECT_CALL(*device_, Save(storage.get()))
+ .WillOnce(Return(true))
+ .WillOnce(Return(false));
+ profile_->set_storage(storage.release());
+ EXPECT_TRUE(profile_->UpdateDevice(device_));
+ EXPECT_FALSE(profile_->UpdateDevice(device_));
+}
+
} // namespace shill
diff --git a/device.cc b/device.cc
index a53e775..9eda4df 100644
--- a/device.cc
+++ b/device.cc
@@ -754,7 +754,7 @@
if (persist) {
enabled_persistent_ = enable;
- manager_->SaveActiveProfile();
+ manager_->UpdateDevice(this);
}
enabled_pending_ = enable;
diff --git a/device.h b/device.h
index 37ca7a7..7b95d73 100644
--- a/device.h
+++ b/device.h
@@ -164,6 +164,7 @@
FRIEND_TEST(DeviceTest, GetProperties);
FRIEND_TEST(DeviceTest, Save);
FRIEND_TEST(DeviceTest, SelectedService);
+ FRIEND_TEST(DeviceTest, SetEnabledPersistent);
FRIEND_TEST(DeviceTest, SetServiceConnectedState);
FRIEND_TEST(DeviceTest, Start);
FRIEND_TEST(DeviceTest, Stop);
diff --git a/device_unittest.cc b/device_unittest.cc
index df1a745..953cf36 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -323,6 +323,22 @@
OnIPConfigUpdated(ipconfig.get(), true);
}
+TEST_F(DeviceTest, SetEnabledPersistent) {
+ EXPECT_FALSE(device_->enabled_);
+ EXPECT_FALSE(device_->enabled_pending_);
+ device_->enabled_persistent_ = false;
+ StrictMock<MockManager> manager(control_interface(),
+ dispatcher(),
+ metrics(),
+ glib());
+ EXPECT_CALL(manager, UpdateDevice(_));
+ device_->manager_ = &manager;
+ Error error;
+ device_->SetEnabledPersistent(true, &error, ResultCallback());
+ EXPECT_TRUE(device_->enabled_persistent_);
+ EXPECT_TRUE(device_->enabled_pending_);
+}
+
TEST_F(DeviceTest, Start) {
EXPECT_FALSE(device_->running_);
EXPECT_FALSE(device_->enabled_);
diff --git a/manager.cc b/manager.cc
index 0d6ebaa..d5b75e2 100644
--- a/manager.cc
+++ b/manager.cc
@@ -138,9 +138,7 @@
SLOG(Manager, 2) << "Manager initialized.";
}
-Manager::~Manager() {
- profiles_.clear();
-}
+Manager::~Manager() {}
void Manager::AddDeviceToBlackList(const string &device_name) {
device_info_.AddDeviceToBlackList(device_name);
@@ -534,12 +532,6 @@
ActiveProfile().get() == profile.get());
}
-void Manager::SaveActiveProfile() {
- if (!profiles_.empty()) {
- ActiveProfile()->Save();
- }
-}
-
bool Manager::MoveServiceToProfile(const ServiceRefPtr &to_move,
const ProfileRefPtr &destination) {
const ProfileRefPtr from = to_move->profile();
@@ -658,29 +650,25 @@
}
void Manager::RegisterDevice(const DeviceRefPtr &to_manage) {
- SLOG(Manager, 2) << __func__ << "(" << to_manage->FriendlyName() << ")";
- vector<DeviceRefPtr>::iterator it;
- for (it = devices_.begin(); it != devices_.end(); ++it) {
+ LOG(INFO) << "Device " << to_manage->FriendlyName() << " registered.";
+ for (vector<DeviceRefPtr>::iterator it = devices_.begin();
+ it != devices_.end(); ++it) {
if (to_manage.get() == it->get())
return;
}
devices_.push_back(to_manage);
- // We are applying device properties from the DefaultProfile, and adding
- // the union of hidden services in all loaded profiles to the device.
+ // We are applying device properties from the DefaultProfile, and adding the
+ // union of hidden services in all loaded profiles to the device.
for (vector<ProfileRefPtr>::iterator it = profiles_.begin();
- it != profiles_.end();
- ++it) {
+ it != profiles_.end(); ++it) {
// Load device configuration, if any exists, as well as hidden services.
(*it)->ConfigureDevice(to_manage);
-
- // Currently the only profile for which "Save" is implemented is the
- // DefaultProfile. It iterates over all Devices and stores their state.
- // We perform the Save now in case the device we have just registered
- // is new and needs to be added to the stored DefaultProfile.
- (*it)->Save();
}
+ // If |to_manage| is new, it needs to be persisted.
+ UpdateDevice(to_manage);
+
// In normal usage, running_ will always be true when we are here, however
// unit tests sometimes do things in otherwise invalid states.
if (running_ && (to_manage->enabled_persistent() ||
@@ -787,6 +775,20 @@
SortServices();
}
+void Manager::UpdateDevice(const DeviceRefPtr &to_update) {
+ LOG(INFO) << "Device " << to_update->link_name() << " updated: "
+ << (to_update->enabled_persistent() ? "enabled" : "disabled");
+ // Saves the device to the topmost profile that accepts it. Normally, this
+ // would be the only DefaultProfile at the bottom of the stack except in
+ // autotests that push a second test-only DefaultProfile.
+ for (vector<ProfileRefPtr>::reverse_iterator rit = profiles_.rbegin();
+ rit != profiles_.rend(); ++rit) {
+ if ((*rit)->UpdateDevice(to_update)) {
+ return;
+ }
+ }
+}
+
void Manager::SaveServiceToProfile(const ServiceRefPtr &to_update) {
if (IsServiceEphemeral(to_update)) {
if (profiles_.empty()) {
diff --git a/manager.h b/manager.h
index d7d94d4..4e275d6 100644
--- a/manager.h
+++ b/manager.h
@@ -72,7 +72,6 @@
const ProfileRefPtr &ActiveProfile() const;
bool IsActiveProfile(const ProfileRefPtr &profile) const;
- virtual void SaveActiveProfile();
bool MoveServiceToProfile(const ServiceRefPtr &to_move,
const ProfileRefPtr &destination);
ProfileRefPtr LookupProfileByRpcIdentifier(const std::string &profile_rpcid);
@@ -94,6 +93,9 @@
virtual void DeregisterService(const ServiceRefPtr &to_forget);
virtual void UpdateService(const ServiceRefPtr &to_update);
+ // Persists |to_update| into an appropriate profile.
+ virtual void UpdateDevice(const DeviceRefPtr &to_update);
+
void FilterByTechnology(Technology::Identifier tech,
std::vector<DeviceRefPtr> *found);
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 2933976..0d7a1b3 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -186,8 +186,8 @@
void AddMockProfileToManager(Manager *manager) {
scoped_refptr<MockProfile> profile(
new MockProfile(control_interface(), manager, ""));
- EXPECT_CALL(*profile, GetRpcIdentifier())
- .WillRepeatedly(Return("/"));
+ EXPECT_CALL(*profile, GetRpcIdentifier()).WillRepeatedly(Return("/"));
+ EXPECT_CALL(*profile, UpdateDevice(_)).WillRepeatedly(Return(false));
AdoptProfile(manager, profile);
}
@@ -265,7 +265,7 @@
DeviceRefPtr device_ref(mock_devices_[0].get());
AdoptProfile(manager(), profile); // Passes ownership.
EXPECT_CALL(*profile, ConfigureDevice(device_ref));
- EXPECT_CALL(*profile, Save());
+ EXPECT_CALL(*profile, UpdateDevice(device_ref));
manager()->RegisterDevice(mock_devices_[0]);
}
@@ -1913,6 +1913,20 @@
manager()->UpdateService(service);
}
+TEST_F(ManagerTest, UpdateDevice) {
+ MockProfile *profile0 = new MockProfile(control_interface(), manager(), "");
+ MockProfile *profile1 = new MockProfile(control_interface(), manager(), "");
+ MockProfile *profile2 = new MockProfile(control_interface(), manager(), "");
+ AdoptProfile(manager(), profile0); // Passes ownership.
+ AdoptProfile(manager(), profile1); // Passes ownership.
+ AdoptProfile(manager(), profile2); // Passes ownership.
+ DeviceRefPtr device_ref(mock_devices_[0].get());
+ EXPECT_CALL(*profile0, UpdateDevice(device_ref)).Times(0);
+ EXPECT_CALL(*profile1, UpdateDevice(device_ref)).WillOnce(Return(true));
+ EXPECT_CALL(*profile2, UpdateDevice(device_ref)).WillOnce(Return(false));
+ manager()->UpdateDevice(mock_devices_[0]);
+}
+
TEST_F(ManagerTest, EnumerateProfiles) {
vector<string> profile_paths;
for (size_t i = 0; i < 10; i++) {
diff --git a/mock_manager.h b/mock_manager.h
index 7b6252c..1c27513 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -34,6 +34,7 @@
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_METHOD1(UpdateDevice, void(const DeviceRefPtr &to_update));
MOCK_METHOD1(RecheckPortalOnService, void(const ServiceRefPtr &service));
MOCK_METHOD2(HandleProfileEntryDeletion,
bool (const ProfileRefPtr &profile,
diff --git a/mock_profile.h b/mock_profile.h
index 436eeb7..bbeb2b1 100644
--- a/mock_profile.h
+++ b/mock_profile.h
@@ -28,6 +28,7 @@
MOCK_METHOD0(GetRpcIdentifier, std::string());
MOCK_METHOD1(GetStoragePath, bool(FilePath *filepath));
MOCK_METHOD1(UpdateService, bool(const ServiceRefPtr &service));
+ MOCK_METHOD1(UpdateDevice, bool(const DeviceRefPtr &device));
MOCK_METHOD0(Save, bool());
MOCK_CONST_METHOD0(GetConstStorage, const StoreInterface *());
diff --git a/profile.cc b/profile.cc
index 46d589d..6fbb21f 100644
--- a/profile.cc
+++ b/profile.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -280,6 +280,10 @@
return service_groups;
}
+bool Profile::UpdateDevice(const DeviceRefPtr &device) {
+ return false;
+}
+
void Profile::HelpRegisterDerivedStrings(
const string &name,
Strings(Profile::*get)(Error *),
diff --git a/profile.h b/profile.h
index daa171d..8096aba 100644
--- a/profile.h
+++ b/profile.h
@@ -112,6 +112,12 @@
std::vector<std::string> EnumerateAvailableServices(Error *error);
std::vector<std::string> EnumerateEntries(Error *error);
+ // Clobbers persisted notion of |device| with data from |device|. Returns true
+ // if |device| was found and updated, false otherwise. The base implementation
+ // always returns false -- currently devices are persisted only in
+ // DefaultProfile.
+ virtual bool UpdateDevice(const DeviceRefPtr &device);
+
// Write all in-memory state to disk via |storage_|.
virtual bool Save();
diff --git a/profile_unittest.cc b/profile_unittest.cc
index 4b45fdd..2522fbb 100644
--- a/profile_unittest.cc
+++ b/profile_unittest.cc
@@ -356,4 +356,8 @@
Error::kSuccess));
}
+TEST_F(ProfileTest, UpdateDevice) {
+ EXPECT_FALSE(profile_->UpdateDevice(NULL));
+}
+
} // namespace shill