[shill] Make profiles backed with StoreInterfaces
Rely on persistence of data in StoreInterface to maintain
Service/Device/IPConfig info for entities that are not
currently active, instead of maintaining lists in Profile
objects themselves.
BUG=chromium-os:17253
TEST=unit, run on device
Change-Id: I206f44ddf16c584354f8fcadb57032f047f33d0a
Reviewed-on: http://gerrit.chromium.org/gerrit/10024
Commit-Ready: Chris Masone <cmasone@chromium.org>
Reviewed-by: Chris Masone <cmasone@chromium.org>
Tested-by: Chris Masone <cmasone@chromium.org>
diff --git a/Makefile b/Makefile
index 927e4d0..b20ad66 100644
--- a/Makefile
+++ b/Makefile
@@ -199,6 +199,7 @@
rtnl_handler_unittest.o \
rtnl_listener_unittest.o \
rtnl_message_unittest.o \
+ service_under_test.o \
service_unittest.o \
shill_unittest.o \
testrunner.o \
diff --git a/cellular_service.cc b/cellular_service.cc
index be5af33..8011bc6 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -59,7 +59,7 @@
return cellular_->TechnologyIs(type);
}
-string CellularService::GetStorageIdentifier() {
+string CellularService::GetStorageIdentifier() const {
string id = base::StringPrintf("%s_%s_%s",
kServiceType,
cellular_->address().c_str(),
diff --git a/cellular_service.h b/cellular_service.h
index d9c0cd2..4f1b3b3 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -36,7 +36,7 @@
virtual bool TechnologyIs(const Technology::Identifier type) const;
// cellular_<MAC>_<Service_Operator_Name>
- std::string GetStorageIdentifier();
+ std::string GetStorageIdentifier() const;
const std::string &activation_state() const { return activation_state_; }
void set_activation_state(const std::string &state) {
diff --git a/default_profile_unittest.cc b/default_profile_unittest.cc
index 4e4fdca..0fcebb5 100644
--- a/default_profile_unittest.cc
+++ b/default_profile_unittest.cc
@@ -8,10 +8,13 @@
#include <string>
#include <vector>
+#include <base/file_path.h>
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
+#include "shill/key_file_store.h"
+#include "shill/glib.h"
#include "shill/manager.h"
#include "shill/mock_control.h"
#include "shill/mock_device.h"
@@ -31,7 +34,7 @@
DefaultProfileTest()
: profile_(new DefaultProfile(control_interface(),
manager(),
- FilePath(kTestStoragePath),
+ FilePath(storage_path()),
properties_)),
device_(new MockDevice(control_interface(),
dispatcher(),
@@ -43,9 +46,20 @@
virtual ~DefaultProfileTest() {}
+ virtual void SetUp() {
+ PropertyStoreTest::SetUp();
+ FilePath final_path;
+ ASSERT_TRUE(profile_->GetStoragePath(&final_path));
+ scoped_ptr<KeyFileStore> storage(new KeyFileStore(&real_glib_));
+ storage->set_path(final_path);
+ ASSERT_TRUE(storage->Open());
+ profile_->set_storage(storage.release()); // Passes ownership.
+ }
+
protected:
static const char kTestStoragePath[];
+ GLib real_glib_;
ProfileRefPtr profile_;
scoped_refptr<MockDevice> device_;
Manager::Properties properties_;
@@ -107,7 +121,7 @@
TEST_F(DefaultProfileTest, GetStoragePath) {
FilePath path;
EXPECT_TRUE(profile_->GetStoragePath(&path));
- EXPECT_EQ(string(kTestStoragePath) + "/default.profile", path.value());
+ EXPECT_EQ(storage_path() + "/default.profile", path.value());
}
} // namespace shill
diff --git a/ephemeral_profile.cc b/ephemeral_profile.cc
index 75ccff6..672a098 100644
--- a/ephemeral_profile.cc
+++ b/ephemeral_profile.cc
@@ -25,8 +25,17 @@
EphemeralProfile::~EphemeralProfile() {}
-void EphemeralProfile::Finalize() {
- services()->clear();
+bool EphemeralProfile::AdoptService(const ServiceRefPtr &service) {
+ VLOG(2) << "Adding " << service->UniqueName() << " to ephemeral profile.";
+ service->set_profile(this);
+ return true;
+}
+
+bool EphemeralProfile::AbandonService(const ServiceRefPtr &service) {
+ if (service->profile() == this)
+ service->set_profile(NULL);
+ VLOG(2) << "Removing " << service->UniqueName() << " from ephemeral profile.";
+ return true;
}
bool EphemeralProfile::Save() {
diff --git a/ephemeral_profile.h b/ephemeral_profile.h
index 2b7f0f6..e4326e6 100644
--- a/ephemeral_profile.h
+++ b/ephemeral_profile.h
@@ -28,8 +28,8 @@
EphemeralProfile(ControlInterface *control_interface, Manager *manager);
virtual ~EphemeralProfile();
- // Merely stop managing service persistence; flush nothing to disk.
- virtual void Finalize();
+ virtual bool AdoptService(const ServiceRefPtr &service);
+ virtual bool AbandonService(const ServiceRefPtr &service);
// Should not be called.
virtual bool Save();
diff --git a/ethernet_service.cc b/ethernet_service.cc
index 4570464..7a8f363 100644
--- a/ethernet_service.cc
+++ b/ethernet_service.cc
@@ -53,7 +53,7 @@
return ethernet_->GetRpcIdentifier();
}
-string EthernetService::GetStorageIdentifier() {
+string EthernetService::GetStorageIdentifier() const {
return base::StringPrintf("%s_%s",
kServiceType, ethernet_->address().c_str());
}
diff --git a/ethernet_service.h b/ethernet_service.h
index 3be0a18..4a38958 100644
--- a/ethernet_service.h
+++ b/ethernet_service.h
@@ -31,7 +31,7 @@
virtual bool TechnologyIs(const Technology::Identifier type) const;
// ethernet_<MAC>
- virtual std::string GetStorageIdentifier();
+ virtual std::string GetStorageIdentifier() const;
private:
static const char kServiceType[];
diff --git a/manager.cc b/manager.cc
index 452cda3..5f08ccb 100644
--- a/manager.cc
+++ b/manager.cc
@@ -133,9 +133,8 @@
// Persist profile, device, service information to disk.
vector<ProfileRefPtr>::iterator it;
for (it = profiles_.begin(); it != profiles_.end(); ++it) {
- (*it)->Finalize();
+ (*it)->Save();
}
- ephemeral_profile_->Finalize();
adaptor_->UpdateRunning();
modem_info_.Stop();
@@ -151,17 +150,14 @@
return profiles_.back();
}
-bool Manager::MoveToActiveProfile(const ProfileRefPtr &from,
- const ServiceRefPtr &to_move) {
- return ActiveProfile()->AdoptService(to_move) &&
- from->AbandonService(to_move->UniqueName());
+bool Manager::MoveServiceToProfile(const ServiceRefPtr &to_move,
+ const ProfileRefPtr &destination) {
+ const ProfileRefPtr from = to_move->profile();
+ return destination->AdoptService(to_move) &&
+ from->AbandonService(to_move);
}
void Manager::RegisterDevice(const DeviceRefPtr &to_manage) {
- // TODO(pstew): Should DefaultProfile have a list of devices, analogous to
- // the list of services that it manages? If so, we should do a similar merge
- // thing here.
-
vector<DeviceRefPtr>::iterator it;
for (it = devices_.begin(); it != devices_.end(); ++it) {
if (to_manage.get() == it->get())
@@ -172,6 +168,13 @@
// TODO(pstew): Should check configuration
if (running_)
to_manage->Start();
+
+ // NB: Should we keep a ptr to default profile and only persist that here?
+ for (vector<ProfileRefPtr>::iterator it = profiles_.begin();
+ it != profiles_.end();
+ ++it) {
+ (*it)->Save();
+ }
}
void Manager::DeregisterDevice(const DeviceRefPtr &to_forget) {
@@ -190,15 +193,16 @@
void Manager::RegisterService(const ServiceRefPtr &to_manage) {
VLOG(2) << __func__ << to_manage->UniqueName();
+ bool merged = false;
for (vector<ProfileRefPtr>::iterator it = profiles_.begin();
- it != profiles_.end();
+ !merged && it != profiles_.end();
++it) {
- if ((*it)->MergeService(to_manage)) // this will merge, if possible.
- break;
+ merged = (*it)->MergeService(to_manage); // Will merge, if possible.
}
// If not found, add it to the ephemeral profile
- ephemeral_profile_->AdoptService(to_manage);
+ if (!merged)
+ ephemeral_profile_->AdoptService(to_manage);
// Now add to OUR list.
vector<ServiceRefPtr>::iterator it;
@@ -216,11 +220,7 @@
service_paths);
}
-void Manager::DeregisterService(const ServiceConstRefPtr &to_forget) {
- // If the service is in the ephemeral profile, destroy it.
- if (!ephemeral_profile_->AbandonService(to_forget->UniqueName())) {
- // if it's in one of the real profiles...um...I guess mark it unconnectable?
- }
+void Manager::DeregisterService(const ServiceRefPtr &to_forget) {
vector<ServiceRefPtr>::iterator it;
for (it = services_.begin(); it != services_.end(); ++it) {
if (to_forget->UniqueName() == (*it)->UniqueName()) {
diff --git a/manager.h b/manager.h
index 8a04962..3a9482b 100644
--- a/manager.h
+++ b/manager.h
@@ -59,14 +59,14 @@
void AdoptProfile(const ProfileRefPtr &profile);
const ProfileRefPtr &ActiveProfile();
- bool MoveToActiveProfile(const ProfileRefPtr &from,
- const ServiceRefPtr &to_move);
+ bool MoveServiceToProfile(const ServiceRefPtr &to_move,
+ const ProfileRefPtr &destination);
void RegisterDevice(const DeviceRefPtr &to_manage);
void DeregisterDevice(const DeviceRefPtr &to_forget);
virtual void RegisterService(const ServiceRefPtr &to_manage);
- virtual void DeregisterService(const ServiceConstRefPtr &to_forget);
+ virtual void DeregisterService(const ServiceRefPtr &to_forget);
virtual void UpdateService(const ServiceConstRefPtr &to_update);
void FilterByTechnology(Technology::Identifier tech,
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 76369da..b667dd7 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -15,7 +15,10 @@
#include <gtest/gtest.h>
#include "shill/adaptor_interfaces.h"
+#include "shill/ephemeral_profile.h"
#include "shill/error.h"
+#include "shill/glib.h"
+#include "shill/key_file_store.h"
#include "shill/key_value_store.h"
#include "shill/mock_adaptors.h"
#include "shill/mock_control.h"
@@ -26,6 +29,7 @@
#include "shill/mock_store.h"
#include "shill/mock_wifi.h"
#include "shill/property_store_unittest.h"
+#include "shill/service_under_test.h"
#include "shill/wifi_service.h"
using std::map;
@@ -35,6 +39,7 @@
namespace shill {
using ::testing::_;
+using ::testing::AnyNumber;
using ::testing::Ne;
using ::testing::NiceMock;
using ::testing::Return;
@@ -78,6 +83,23 @@
}
bool ServiceOrderIs(ServiceRefPtr svc1, ServiceRefPtr svc2);
+ Profile *CreateProfileForManager(Manager *manager, GLib *glib) {
+ Profile::Identifier id("rather", "irrelevant");
+ scoped_ptr<Profile> profile(new Profile(control_interface(),
+ manager,
+ id,
+ "",
+ false));
+ FilePath final_path(storage_path());
+ final_path = final_path.Append("test.profile");
+ scoped_ptr<KeyFileStore> storage(new KeyFileStore(glib));
+ storage->set_path(final_path);
+ if (!storage->Open())
+ return NULL;
+ profile->set_storage(storage.release()); // Passes ownership.
+ return profile.release();
+ }
+
protected:
scoped_refptr<MockWiFi> mock_wifi_;
vector<scoped_refptr<MockDevice> > mock_devices_;
@@ -140,6 +162,10 @@
run_path(),
storage_path(),
string());
+ ProfileRefPtr profile(CreateProfileForManager(&manager, &glib));
+ ASSERT_TRUE(profile.get());
+ manager.AdoptProfile(profile);
+
scoped_refptr<MockService> mock_service(
new NiceMock<MockService>(control_interface(),
dispatcher(),
@@ -174,6 +200,66 @@
manager.Stop();
}
+TEST_F(ManagerTest, RegisterKnownService) {
+ // It's much easier and safer to use a real GLib for this test.
+ GLib glib;
+ Manager manager(control_interface(),
+ dispatcher(),
+ &glib,
+ run_path(),
+ storage_path(),
+ string());
+ ProfileRefPtr profile(CreateProfileForManager(&manager, &glib));
+ ASSERT_TRUE(profile.get());
+ manager.AdoptProfile(profile);
+ {
+ ServiceRefPtr service1(new ServiceUnderTest(control_interface(),
+ dispatcher(),
+ &manager));
+ service1->set_favorite(!service1->favorite());
+ ASSERT_TRUE(profile->AdoptService(service1));
+ ASSERT_TRUE(profile->ContainsService(service1));
+ } // Force destruction of service1.
+
+ ServiceRefPtr service2(new ServiceUnderTest(control_interface(),
+ dispatcher(),
+ &manager));
+ manager.RegisterService(service2);
+ EXPECT_EQ(service2->profile().get(), profile.get());
+ manager.Stop();
+}
+
+TEST_F(ManagerTest, RegisterUnknownService) {
+ // It's much easier and safer to use a real GLib for this test.
+ GLib glib;
+ Manager manager(control_interface(),
+ dispatcher(),
+ &glib,
+ run_path(),
+ storage_path(),
+ string());
+ ProfileRefPtr profile(CreateProfileForManager(&manager, &glib));
+ ASSERT_TRUE(profile.get());
+ manager.AdoptProfile(profile);
+ {
+ ServiceRefPtr service1(new ServiceUnderTest(control_interface(),
+ dispatcher(),
+ &manager));
+ service1->set_favorite(!service1->favorite());
+ ASSERT_TRUE(profile->AdoptService(service1));
+ ASSERT_TRUE(profile->ContainsService(service1));
+ } // Force destruction of service1.
+ scoped_refptr<MockService> mock_service2(
+ new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ &manager));
+ EXPECT_CALL(*mock_service2.get(), GetStorageIdentifier())
+ .WillRepeatedly(Return(mock_service2->UniqueName()));
+ manager.RegisterService(mock_service2);
+ EXPECT_NE(mock_service2->profile().get(), profile.get());
+ manager.Stop();
+}
+
TEST_F(ManagerTest, GetProperties) {
ProfileRefPtr profile(new MockProfile(control_interface(), manager(), ""));
manager()->AdoptProfile(profile);
@@ -221,62 +307,43 @@
}
TEST_F(ManagerTest, MoveService) {
- // It's much easier and safer to use a real GLib for this test.
- GLib glib;
Manager manager(control_interface(),
dispatcher(),
- &glib,
+ glib(),
run_path(),
storage_path(),
string());
+ scoped_refptr<MockService> s2(new MockService(control_interface(),
+ dispatcher(),
+ &manager));
+ // Inject an actual profile, backed by a fake StoreInterface
{
- Profile::Identifier id;
- id.identifier = "irrelevant";
+ Profile::Identifier id("irrelevant");
ProfileRefPtr profile(
new Profile(control_interface(), &manager, id, "", false));
MockStore *storage = new MockStore;
- EXPECT_CALL(*storage, Flush()).WillOnce(Return(true));
+ // Say we don't have |s2| the first time asked, then that we do.
+ EXPECT_CALL(*storage, ContainsGroup(s2->GetStorageIdentifier()))
+ .WillOnce(Return(false))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(*storage, Flush())
+ .Times(AnyNumber())
+ .WillRepeatedly(Return(true));
profile->set_storage(storage);
manager.AdoptProfile(profile);
}
- // I want to ensure that the Profiles are managing this Service object
- // lifetime properly, so I can't hold a ref to it here.
- ProfileRefPtr profile(new MockProfile(control_interface(), &manager, ""));
- string service_name;
- {
- scoped_refptr<MockService> s2(
- new MockService(control_interface(),
- dispatcher(),
- &manager));
- EXPECT_CALL(*s2.get(), Save(_)).WillOnce(Return(true));
+ // Create a profile that already has |s2| in it.
+ ProfileRefPtr profile(new EphemeralProfile(control_interface(), &manager));
+ profile->AdoptService(s2);
- profile->AdoptService(s2);
- s2->set_profile(profile);
- service_name = s2->UniqueName();
- }
-
- // Now, move the |service| to another profile.
- ASSERT_TRUE(manager.MoveToActiveProfile(profile,
- profile->FindService(service_name)));
+ // Now, move the Service |s2| to another profile.
+ EXPECT_CALL(*s2.get(), Save(_)).WillOnce(Return(true));
+ ASSERT_TRUE(manager.MoveServiceToProfile(s2, manager.ActiveProfile()));
// Force destruction of the original Profile, to ensure that the Service
// is kept alive and populated with data.
profile = NULL;
- {
- ServiceRefPtr serv(manager.ActiveProfile()->FindService(service_name));
- ASSERT_TRUE(serv.get() != NULL);
- Error error(Error::kInvalidProperty, "");
- ::DBus::Error dbus_error;
- map<string, ::DBus::Variant> props;
- bool expected = true;
- serv->mutable_store()->SetBoolProperty(flimflam::kAutoConnectProperty,
- expected,
- &error);
- DBusAdaptor::GetProperties(serv->store(), &props, &dbus_error);
- ASSERT_TRUE(ContainsKey(props, flimflam::kAutoConnectProperty));
- EXPECT_EQ(props[flimflam::kAutoConnectProperty].reader().get_bool(),
- expected);
- }
+ ASSERT_TRUE(manager.ActiveProfile()->ContainsService(s2));
manager.Stop();
}
diff --git a/mock_manager.h b/mock_manager.h
index cf6c951..022180b 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -24,7 +24,7 @@
MOCK_CONST_METHOD0(store, const PropertyStore&(void));
MOCK_METHOD1(RegisterService, void(const ServiceRefPtr &to_manage));
MOCK_METHOD1(UpdateService, void(const ServiceConstRefPtr &to_update));
- MOCK_METHOD1(DeregisterService, void(const ServiceConstRefPtr &to_forget));
+ MOCK_METHOD1(DeregisterService, void(const ServiceRefPtr &to_forget));
private:
DISALLOW_COPY_AND_ASSIGN(MockManager);
diff --git a/mock_service.cc b/mock_service.cc
index fafb8cd..5b44fda 100644
--- a/mock_service.cc
+++ b/mock_service.cc
@@ -11,6 +11,7 @@
#include <gmock/gmock.h>
#include "shill/refptr_types.h"
+#include "shill/store_interface.h"
using std::string;
using testing::_;
@@ -26,7 +27,9 @@
EventDispatcher *dispatcher,
Manager *manager)
: Service(control_interface, dispatcher, manager, "mock") {
- ON_CALL(*this, GetRpcIdentifier()).WillByDefault(Return(""));
+ const string &id = UniqueName();
+ EXPECT_CALL(*this, GetRpcIdentifier()).WillRepeatedly(Return(id));
+ EXPECT_CALL(*this, GetStorageIdentifier()).WillRepeatedly(Return(id));
ON_CALL(*this, state()).WillByDefault(Return(kStateUnknown));
ON_CALL(*this, failure()).WillByDefault(Return(kFailureUnknown));
ON_CALL(*this, TechnologyIs(_)).WillByDefault(Return(false));
@@ -34,4 +37,8 @@
MockService::~MockService() {}
+bool MockService::FauxSave(StoreInterface *store) {
+ return store->SetString(UniqueName(), "dummy", "dummy");
+}
+
} // namespace shill
diff --git a/mock_service.h b/mock_service.h
index 2f77a6f..7de6948 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -32,10 +32,14 @@
MOCK_CONST_METHOD0(failure, ConnectFailure());
MOCK_METHOD0(GetDeviceRpcId, std::string());
MOCK_CONST_METHOD0(GetRpcIdentifier, std::string());
- MOCK_METHOD0(GetStorageIdentifier, std::string());
+ MOCK_CONST_METHOD0(GetStorageIdentifier, std::string());
MOCK_METHOD1(Load, bool(StoreInterface *store_interface));
MOCK_METHOD1(Save, bool(StoreInterface *store_interface));
+ // Set a string for this Service via |store|. Can be wired to Save() for
+ // test purposes.
+ bool FauxSave(StoreInterface *store);
+
private:
DISALLOW_COPY_AND_ASSIGN(MockService);
};
diff --git a/profile.cc b/profile.cc
index f65aa1b..2025de1 100644
--- a/profile.cc
+++ b/profile.cc
@@ -4,7 +4,7 @@
#include "shill/profile.h"
-#include <map>
+#include <set>
#include <string>
#include <vector>
@@ -23,7 +23,7 @@
#include "shill/service.h"
#include "shill/store_interface.h"
-using std::map;
+using std::set;
using std::string;
using std::vector;
@@ -55,9 +55,7 @@
NULL);
}
-Profile::~Profile() {
- DCHECK_EQ(services_.size(), 0);
-}
+Profile::~Profile() {}
bool Profile::InitStorage(GLib *glib) {
FilePath final_path;
@@ -92,48 +90,32 @@
}
bool Profile::AdoptService(const ServiceRefPtr &service) {
- if (ContainsKey(services_, service->UniqueName()))
+ if (storage_->ContainsGroup(service->GetStorageIdentifier()))
return false;
service->set_profile(this);
- services_[service->UniqueName()] = service;
- return true;
+ return service->Save(storage_.get()) && storage_->Flush();
}
-bool Profile::AbandonService(const string &name) {
- map<string, ServiceRefPtr>::iterator to_abandon = services_.find(name);
- if (to_abandon != services_.end()) {
- services_.erase(to_abandon);
- return true;
- }
- return false;
+bool Profile::AbandonService(const ServiceRefPtr &service) {
+ if (service->profile() == this)
+ service->set_profile(NULL);
+ return storage_->DeleteGroup(service->GetStorageIdentifier()) &&
+ storage_->Flush();
}
-bool Profile::DemoteService(const string &name) {
- map<string, ServiceRefPtr>::iterator to_demote = services_.find(name);
- if (to_demote == services_.end())
- return false;
- return true; // TODO(cmasone): mark |to_demote| as inactive or something.
+bool Profile::UpdateService(const ServiceRefPtr &service) {
+ return service->Save(storage_.get()) && storage_->Flush();
}
bool Profile::MergeService(const ServiceRefPtr &service) {
- // TODO(cmasone): Make |service| just load itself from |store_|.
- map<string, ServiceRefPtr>::iterator it;
- for (it = services_.begin(); it != services_.end(); ++it) {
- if (Mergeable(it->second, service))
- return true;
- }
- return false;
+ if (!storage_->ContainsGroup(service->GetStorageIdentifier()))
+ return false;
+ service->set_profile(this);
+ return service->Load(storage_.get());
}
-ServiceRefPtr Profile::FindService(const std::string& name) {
- if (ContainsKey(services_, name))
- return services_[name];
- return NULL;
-}
-
-void Profile::Finalize() {
- Save();
- services_.clear();
+bool Profile::ContainsService(const ServiceConstRefPtr &service) {
+ return storage_->ContainsGroup(service->GetStorageIdentifier());
}
bool Profile::IsValidIdentifierToken(const std::string &token) {
@@ -178,7 +160,7 @@
}
bool Profile::Save() {
- return SaveServices() && storage_->Flush();
+ return storage_->Flush();
}
bool Profile::GetStoragePath(FilePath *path) {
@@ -198,12 +180,10 @@
}
vector<string> Profile::EnumerateEntries() {
- vector<string> rpc_ids;
- map<string, ServiceRefPtr>::const_iterator it;
- for (it = services_.begin(); it != services_.end(); ++it) {
- rpc_ids.push_back(it->second->GetRpcIdentifier());
- }
- return rpc_ids;
+ // TODO(someone): Determine if we care about this wasteful copying; consider
+ // making GetGroups return a vector.
+ set<string> groups(storage_->GetGroups());
+ return vector<string>(groups.begin(), groups.end());
}
void Profile::HelpRegisterDerivedStrings(
@@ -215,15 +195,4 @@
StringsAccessor(new CustomAccessor<Profile, Strings>(this, get, set)));
}
-bool Profile::SaveServices() {
- for (map<string, ServiceRefPtr>::iterator it = services_.begin();
- it != services_.end();
- ++it) {
- VLOG(1) << "Saving service named " << it->first;
- if (!it->second->Save(storage()))
- return false;
- }
- return true;
-}
-
} // namespace shill
diff --git a/profile.h b/profile.h
index 1ae27f6..0637fdd 100644
--- a/profile.h
+++ b/profile.h
@@ -63,29 +63,26 @@
// Begin managing the persistence of |service|.
// Returns true if |service| is new to this profile and was added,
// false if the |service| already existed.
- bool AdoptService(const ServiceRefPtr &service);
+ virtual bool AdoptService(const ServiceRefPtr &service);
- // Cease managing the persistence of the Service named |name|.
- // Returns true if |name| was found and abandoned, false if not found.
- bool AbandonService(const std::string &name);
+ // Cease managing the persistence of the Service |service|.
+ // Returns true if |service| was found and abandoned, or not found.
+ // Returns false if can't be abandoned.
+ virtual bool AbandonService(const ServiceRefPtr &service);
- // Continue persisting the Service named |name|, but don't consider it
- // usable for connectivity.
- // Returns true if |name| was found and demoted, false if not found.
- bool DemoteService(const std::string &name);
+ // Clobbers persisted notion of |service| with data from |service|.
+ // Returns true if |service| was found and updated, false if not found.
+ bool UpdateService(const ServiceRefPtr &service);
// Determine if |service| represents a service that's already in |services_|.
// If so, merge them smartly and return true. If not, return false.
bool MergeService(const ServiceRefPtr &service);
- ServiceRefPtr FindService(const std::string& name);
+ bool ContainsService(const ServiceConstRefPtr &service);
std::vector<std::string> EnumerateAvailableServices();
std::vector<std::string> EnumerateEntries();
- // Flush any pending entry info to disk and stop managing service persistence.
- virtual void Finalize();
-
// Write all in-memory state to disk via |storage_|.
virtual bool Save();
@@ -100,15 +97,12 @@
protected:
// Protected getters
Manager *manager() const { return manager_; }
- std::map<std::string, ServiceRefPtr> *services() { return &services_; }
StoreInterface *storage() { return storage_.get(); }
private:
friend class ProfileAdaptorInterface;
FRIEND_TEST(ProfileTest, IsValidIdentifierToken);
FRIEND_TEST(ProfileTest, ParseIdentifier);
- // TODO(cmasone): once we can add services organically, take this out.
- FRIEND_TEST(ServiceTest, MoveService);
static bool IsValidIdentifierToken(const std::string &token);
@@ -118,24 +112,14 @@
// on success.
static bool ParseIdentifier(const std::string &raw, Identifier *parsed);
- // Returns true if |candidate| can be merged into |service|.
- bool Mergeable(const ServiceRefPtr &/*service*/,
- const ServiceRefPtr &/*candiate*/) {
- return false;
- }
-
void HelpRegisterDerivedStrings(
const std::string &name,
Strings(Profile::*get)(void),
void(Profile::*set)(const Strings&, Error *));
- // Persists |services_| to disk via |storage_|.
- bool SaveServices();
-
// Data members shared with subclasses via getter/setters above in the
// protected: section
Manager *manager_;
- std::map<std::string, ServiceRefPtr> services_;
// Shared with |adaptor_| via public getter.
PropertyStore store_;
diff --git a/profile_unittest.cc b/profile_unittest.cc
index 71aa5a1..320fbf0 100644
--- a/profile_unittest.cc
+++ b/profile_unittest.cc
@@ -7,19 +7,24 @@
#include <string>
#include <vector>
+#include <base/file_path.h>
#include <base/memory/scoped_ptr.h>
#include <base/string_util.h>
#include <gtest/gtest.h>
+#include "shill/glib.h"
+#include "shill/key_file_store.h"
#include "shill/mock_profile.h"
#include "shill/mock_service.h"
#include "shill/mock_store.h"
#include "shill/property_store_unittest.h"
+#include "shill/service_under_test.h"
using std::set;
using std::string;
using std::vector;
using testing::_;
+using testing::Invoke;
using testing::Return;
using testing::SetArgumentPointee;
using testing::StrictMock;
@@ -28,9 +33,9 @@
class ProfileTest : public PropertyStoreTest {
public:
- ProfileTest()
- : store_(new MockStore),
- profile_(new MockProfile(control_interface(), manager(), "")) {
+ ProfileTest() {
+ Profile::Identifier id("rather", "irrelevant");
+ profile_ = new Profile(control_interface(), manager(), id, "", false);
}
MockService *CreateMockService() {
@@ -39,9 +44,19 @@
manager());
}
+ virtual void SetUp() {
+ PropertyStoreTest::SetUp();
+ FilePath final_path(storage_path());
+ final_path = final_path.Append("test.profile");
+ scoped_ptr<KeyFileStore> storage(new KeyFileStore(&real_glib_));
+ storage->set_path(final_path);
+ ASSERT_TRUE(storage->Open());
+ profile_->set_storage(storage.release()); // Passes ownership.
+ }
+
protected:
- scoped_ptr<MockStore> store_;
- scoped_refptr<MockProfile> profile_;
+ GLib real_glib_;
+ ProfileRefPtr profile_;
};
TEST_F(ProfileTest, IsValidIdentifierToken) {
@@ -89,8 +104,7 @@
TEST_F(ProfileTest, GetFriendlyName) {
static const char kUser[] = "theUser";
static const char kIdentifier[] = "theIdentifier";
- Profile::Identifier id;
- id.identifier = kIdentifier;
+ Profile::Identifier id(kIdentifier);
ProfileRefPtr profile(
new Profile(control_interface(), manager(), id, "", false));
EXPECT_EQ(kIdentifier, profile->GetFriendlyName());
@@ -104,8 +118,7 @@
static const char kIdentifier[] = "someprofile";
static const char kFormat[] = "/a/place/for/%s";
FilePath path;
- Profile::Identifier id;
- id.identifier = kIdentifier;
+ Profile::Identifier id(kIdentifier);
ProfileRefPtr profile(
new Profile(control_interface(), manager(), id, "", false));
EXPECT_FALSE(profile->GetStoragePath(&path));
@@ -118,75 +131,96 @@
}
TEST_F(ProfileTest, ServiceManagement) {
- string service1_name;
- string service2_name;
- {
- ServiceRefPtr service1(CreateMockService());
- ServiceRefPtr service2(CreateMockService());
- service1_name = service1->UniqueName();
- service2_name = service2->UniqueName();
- ASSERT_TRUE(profile_->AdoptService(service1));
- ASSERT_TRUE(profile_->AdoptService(service2));
- ASSERT_FALSE(profile_->AdoptService(service1));
- }
- ASSERT_TRUE(profile_->FindService(service1_name).get() != NULL);
- ASSERT_TRUE(profile_->FindService(service2_name).get() != NULL);
+ scoped_refptr<MockService> service1(CreateMockService());
+ scoped_refptr<MockService> service2(CreateMockService());
- ASSERT_TRUE(profile_->AbandonService(service1_name));
- ASSERT_TRUE(profile_->FindService(service1_name).get() == NULL);
- ASSERT_TRUE(profile_->FindService(service2_name).get() != NULL);
+ EXPECT_CALL(*service1.get(), Save(_))
+ .WillRepeatedly(Invoke(service1.get(), &MockService::FauxSave));
+ EXPECT_CALL(*service2.get(), Save(_))
+ .WillRepeatedly(Invoke(service2.get(), &MockService::FauxSave));
- ASSERT_FALSE(profile_->AbandonService(service1_name));
- ASSERT_TRUE(profile_->AbandonService(service2_name));
- ASSERT_TRUE(profile_->FindService(service1_name).get() == NULL);
- ASSERT_TRUE(profile_->FindService(service2_name).get() == NULL);
+ ASSERT_TRUE(profile_->AdoptService(service1));
+ ASSERT_TRUE(profile_->AdoptService(service2));
+
+ // Ensure services are in the profile now.
+ ASSERT_TRUE(profile_->ContainsService(service1));
+ ASSERT_TRUE(profile_->ContainsService(service2));
+
+ // Ensure we can't add them twice.
+ ASSERT_FALSE(profile_->AdoptService(service1));
+ ASSERT_FALSE(profile_->AdoptService(service2));
+
+ // Ensure that we can abandon individually, and that doing so is idempotent.
+ ASSERT_TRUE(profile_->AbandonService(service1));
+ ASSERT_FALSE(profile_->ContainsService(service1));
+ ASSERT_TRUE(profile_->AbandonService(service1));
+ ASSERT_TRUE(profile_->ContainsService(service2));
+
+ // Clean up.
+ ASSERT_TRUE(profile_->AbandonService(service2));
+ ASSERT_FALSE(profile_->ContainsService(service1));
+ ASSERT_FALSE(profile_->ContainsService(service2));
}
-TEST_F(ProfileTest, Finalize) {
- Profile::Identifier id;
- id.identifier = "irrelevant";
- ProfileRefPtr profile(
- new Profile(control_interface(), manager(), id, "", false));
+TEST_F(ProfileTest, ServiceMerge) {
+ ServiceRefPtr service1(new ServiceUnderTest(control_interface(),
+ dispatcher(),
+ manager()));
+ service1->set_favorite(!service1->favorite());
+ ASSERT_TRUE(profile_->AdoptService(service1));
+ ASSERT_TRUE(profile_->ContainsService(service1));
+
+ // Create new service; ask Profile to merge it with a known, matching,
+ // service; ensure that settings from |service1| wind up in |service2|.
+ ServiceRefPtr service2(new ServiceUnderTest(control_interface(),
+ dispatcher(),
+ manager()));
+ bool orig_favorite = service2->favorite();
+ ASSERT_TRUE(profile_->MergeService(service2));
+ ASSERT_EQ(service1->favorite(), service2->favorite());
+ ASSERT_NE(orig_favorite, service2->favorite());
+
+ // Clean up.
+ ASSERT_TRUE(profile_->AbandonService(service1));
+ ASSERT_FALSE(profile_->ContainsService(service1));
+ ASSERT_FALSE(profile_->ContainsService(service2));
+}
+
+TEST_F(ProfileTest, Save) {
scoped_refptr<MockService> service1(CreateMockService());
scoped_refptr<MockService> service2(CreateMockService());
EXPECT_CALL(*service1.get(), Save(_)).WillOnce(Return(true));
EXPECT_CALL(*service2.get(), Save(_)).WillOnce(Return(true));
- ASSERT_TRUE(profile->AdoptService(service1));
- ASSERT_TRUE(profile->AdoptService(service2));
+ ASSERT_TRUE(profile_->AdoptService(service1));
+ ASSERT_TRUE(profile_->AdoptService(service2));
- profile->set_storage(store_.release());
- profile->Finalize();
+ profile_->Save();
}
TEST_F(ProfileTest, EntryEnumeration) {
- scoped_refptr<MockService> service1(
- new StrictMock<MockService>(control_interface(),
- dispatcher(),
- manager()));
- scoped_refptr<MockService> service2(
- new StrictMock<MockService>(control_interface(),
- dispatcher(),
- manager()));
+ scoped_refptr<MockService> service1(CreateMockService());
+ scoped_refptr<MockService> service2(CreateMockService());
+ EXPECT_CALL(*service1.get(), Save(_))
+ .WillRepeatedly(Invoke(service1.get(), &MockService::FauxSave));
+ EXPECT_CALL(*service2.get(), Save(_))
+ .WillRepeatedly(Invoke(service2.get(), &MockService::FauxSave));
+
string service1_name(service1->UniqueName());
string service2_name(service2->UniqueName());
- EXPECT_CALL(*service1.get(), GetRpcIdentifier())
- .WillRepeatedly(Return(service1_name));
- EXPECT_CALL(*service2.get(), GetRpcIdentifier())
- .WillRepeatedly(Return(service2_name));
ASSERT_TRUE(profile_->AdoptService(service1));
ASSERT_TRUE(profile_->AdoptService(service2));
ASSERT_EQ(profile_->EnumerateEntries().size(), 2);
- ASSERT_TRUE(profile_->AbandonService(service1_name));
+ ASSERT_TRUE(profile_->AbandonService(service1));
ASSERT_EQ(profile_->EnumerateEntries()[0], service2_name);
- ASSERT_FALSE(profile_->AbandonService(service1_name));
+ ASSERT_TRUE(profile_->AbandonService(service1));
ASSERT_EQ(profile_->EnumerateEntries()[0], service2_name);
- ASSERT_TRUE(profile_->AbandonService(service2_name));
+ ASSERT_TRUE(profile_->AbandonService(service2));
ASSERT_EQ(profile_->EnumerateEntries().size(), 0);
}
diff --git a/service.h b/service.h
index 784649c..9738994 100644
--- a/service.h
+++ b/service.h
@@ -125,7 +125,7 @@
virtual std::string GetRpcIdentifier() const;
// Returns the unique persistent storage identifier for the service.
- virtual std::string GetStorageIdentifier() = 0;
+ virtual std::string GetStorageIdentifier() const = 0;
// Loads the service from persistent |storage|. Returns true on success.
virtual bool Load(StoreInterface *storage);
diff --git a/service_under_test.cc b/service_under_test.cc
new file mode 100644
index 0000000..d747df0
--- /dev/null
+++ b/service_under_test.cc
@@ -0,0 +1,42 @@
+// Copyright (c) 2011 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.
+
+#include "shill/service_under_test.h"
+
+#include <string>
+
+#include "shill/mock_adaptors.h"
+
+using std::string;
+
+namespace shill {
+
+// static
+const char ServiceUnderTest::kRpcId[] = "mock-device-rpc";
+// static
+const char ServiceUnderTest::kStorageId[] = "service";
+
+ServiceUnderTest::ServiceUnderTest(ControlInterface *control_interface,
+ EventDispatcher *dispatcher,
+ Manager *manager)
+ : Service(control_interface, dispatcher, manager, "stub") {
+}
+
+ServiceUnderTest::~ServiceUnderTest() {}
+
+void ServiceUnderTest::Connect(Error */*error*/) {}
+
+void ServiceUnderTest::Disconnect() {}
+
+string ServiceUnderTest::CalculateState() { return ""; }
+
+string ServiceUnderTest::GetRpcIdentifier() const {
+ return ServiceMockAdaptor::kRpcId;
+}
+
+string ServiceUnderTest::GetDeviceRpcId() { return kRpcId; }
+
+string ServiceUnderTest::GetStorageIdentifier() const { return kStorageId; }
+
+} // namespace shill
diff --git a/service_under_test.h b/service_under_test.h
new file mode 100644
index 0000000..015305a
--- /dev/null
+++ b/service_under_test.h
@@ -0,0 +1,38 @@
+// Copyright (c) 2011 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.
+
+#include <string>
+
+#include "shill/service.h"
+
+namespace shill {
+
+class ControlInterface;
+class Error;
+class EventDispatcher;
+class Manager;
+
+// This is a simple Service subclass with all the pure-virutal methods stubbed
+class ServiceUnderTest : public Service {
+ public:
+ static const char kRpcId[];
+ static const char kStorageId[];
+
+ ServiceUnderTest(ControlInterface *control_interface,
+ EventDispatcher *dispatcher,
+ Manager *manager);
+ virtual ~ServiceUnderTest();
+
+ virtual void Connect(Error */*error*/);
+ virtual void Disconnect();
+ virtual std::string CalculateState();
+ virtual std::string GetRpcIdentifier() const;
+ virtual std::string GetDeviceRpcId();
+ virtual std::string GetStorageIdentifier() const;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ServiceUnderTest);
+};
+
+} // namespace shill
diff --git a/service_unittest.cc b/service_unittest.cc
index 42b6614..7b4b53d 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -22,7 +22,7 @@
#include "shill/mock_manager.h"
#include "shill/mock_store.h"
#include "shill/property_store_unittest.h"
-#include "shill/service.h"
+#include "shill/service_under_test.h"
using std::map;
using std::string;
@@ -36,32 +36,6 @@
namespace shill {
-// This is a simple Service subclass with all the pure-virutal methods stubbed
-class ServiceUnderTest : public Service {
- public:
- static const char kRpcId[];
- static const char kStorageId[];
-
- ServiceUnderTest(ControlInterface *control_interface,
- EventDispatcher *dispatcher,
- Manager *manager)
- : Service(control_interface, dispatcher, manager, "stub") {}
- virtual ~ServiceUnderTest() {}
-
- virtual void Connect(Error */*error*/) {}
- virtual void Disconnect() {}
- virtual string CalculateState() { return ""; }
- virtual string GetRpcIdentifier() const { return ServiceMockAdaptor::kRpcId; }
- virtual string GetDeviceRpcId() { return kRpcId; }
- virtual string GetStorageIdentifier() { return kStorageId; }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(ServiceUnderTest);
-};
-
-const char ServiceUnderTest::kRpcId[] = "mock-device-rpc";
-const char ServiceUnderTest::kStorageId[] = "service";
-
class ServiceTest : public PropertyStoreTest {
public:
ServiceTest()
diff --git a/wifi_service.cc b/wifi_service.cc
index aa577c4..c38ee1b 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -115,7 +115,7 @@
return wifi_->TechnologyIs(type);
}
-string WiFiService::GetStorageIdentifier() {
+string WiFiService::GetStorageIdentifier() const {
return StringToLowerASCII(base::StringPrintf("%s_%s_%s_%s_%s",
flimflam::kTypeWifi,
wifi_->address().c_str(),
diff --git a/wifi_service.h b/wifi_service.h
index 4829b6c..4116a8b 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -38,7 +38,7 @@
virtual bool TechnologyIs(const Technology::Identifier type) const;
// wifi_<MAC>_<BSSID>_<mode_string>_<security_string>
- std::string GetStorageIdentifier();
+ std::string GetStorageIdentifier() const;
const std::string &mode() const;
const std::string &key_management() const;