[shill] Start cleaning up some of the naming confusion
We have, so far, not been tidy wrt our objects naming themselves,
for human-readable stuff, RPC-path stuff, etc.
This starts to clean up some of that confusion.
BUG=chromium-os:17744
TEST=unit tests
Change-Id: If4d7f61ba51e527984328a0ccdf4dec461b36074
Reviewed-on: http://gerrit.chromium.org/gerrit/4311
Reviewed-by: Chris Masone <cmasone@chromium.org>
Tested-by: Chris Masone <cmasone@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 8e90222..674052f 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -98,7 +98,7 @@
dispatcher,
manager,
this,
- "service-" + link_name())),
+ "service-" + link_name_)),
service_registered_(false) {
store_.RegisterConstString(flimflam::kCarrierProperty, &carrier_);
store_.RegisterBool(flimflam::kCellularAllowRoamingProperty, &allow_roaming_);
@@ -126,7 +126,7 @@
store_.RegisterConstBool(flimflam::kScanningProperty, &scanning_);
store_.RegisterUint16(flimflam::kScanIntervalProperty, &scan_interval_);
- VLOG(2) << "Cellular device " << link_name() << " initialized.";
+ VLOG(2) << "Cellular device " << link_name_ << " initialized.";
}
Cellular::~Cellular() {
diff --git a/default_profile.cc b/default_profile.cc
index 78929b4..3f74760 100644
--- a/default_profile.cc
+++ b/default_profile.cc
@@ -11,12 +11,13 @@
#include "shill/manager.h"
namespace shill {
+const char DefaultProfile::kDefaultId[] = "default";
DefaultProfile::DefaultProfile(ControlInterface *control_interface,
GLib *glib,
Manager *manager,
const Manager::Properties &manager_props)
- : Profile(control_interface, glib, manager) {
+ : Profile(control_interface, glib, manager, Identifier(kDefaultId), true) {
store_.RegisterConstString(flimflam::kCheckPortalListProperty,
&manager_props.check_portal_list);
store_.RegisterConstString(flimflam::kCountryProperty,
diff --git a/default_profile.h b/default_profile.h
index 87e03fa..36374ae 100644
--- a/default_profile.h
+++ b/default_profile.h
@@ -29,6 +29,8 @@
virtual ~DefaultProfile();
private:
+ static const char kDefaultId[];
+
DISALLOW_COPY_AND_ASSIGN(DefaultProfile);
};
diff --git a/device.cc b/device.cc
index 1e9127a..16bf12f 100644
--- a/device.cc
+++ b/device.cc
@@ -39,8 +39,8 @@
reconnect_(true),
interface_index_(interface_index),
running_(false),
- manager_(manager),
link_name_(link_name),
+ manager_(manager),
adaptor_(control_interface->CreateDeviceAdaptor(this)) {
store_.RegisterConstString(flimflam::kAddressProperty, &hardware_address_);
@@ -125,7 +125,7 @@
const string& Device::UniqueName() const {
// TODO(pstew): link_name is only run-time unique and won't persist
- return link_name();
+ return link_name_;
}
void Device::DestroyIPConfig() {
@@ -139,7 +139,7 @@
bool Device::AcquireDHCPConfig() {
DestroyIPConfig();
- ipconfig_ = DHCPProvider::GetInstance()->CreateConfig(link_name());
+ ipconfig_ = DHCPProvider::GetInstance()->CreateConfig(link_name_);
ipconfig_->RegisterUpdateCallback(
NewCallback(this, &Device::IPConfigUpdatedCallback));
return ipconfig_->RequestIP();
diff --git a/device.h b/device.h
index b17acc9..6866390 100644
--- a/device.h
+++ b/device.h
@@ -62,8 +62,6 @@
std::string GetRpcIdentifier();
- const std::string &link_name() const { return link_name_; }
-
PropertyStore *store() { return &store_; }
// Returns a string that is guaranteed to uniquely identify this Device
@@ -103,6 +101,7 @@
std::vector<ServiceRefPtr> services_;
int interface_index_;
bool running_;
+ const std::string link_name_;
Manager *manager_;
IPConfigRefPtr ipconfig_;
@@ -115,7 +114,6 @@
std::vector<std::string> AvailableIPConfigs();
std::string GetRpcConnectionIdentifier();
- const std::string link_name_;
scoped_ptr<DeviceAdaptorInterface> adaptor_;
DISALLOW_COPY_AND_ASSIGN(Device);
diff --git a/ephemeral_profile.cc b/ephemeral_profile.cc
index c5a3630..417666e 100644
--- a/ephemeral_profile.cc
+++ b/ephemeral_profile.cc
@@ -21,7 +21,7 @@
EphemeralProfile::EphemeralProfile(ControlInterface *control_interface,
GLib *glib,
Manager *manager)
- : Profile(control_interface, glib, manager) {
+ : Profile(control_interface, glib, manager, Identifier(), false) {
}
EphemeralProfile::~EphemeralProfile() {}
diff --git a/ethernet.cc b/ethernet.cc
index a54d704..9266dfd 100644
--- a/ethernet.cc
+++ b/ethernet.cc
@@ -70,7 +70,7 @@
void Ethernet::LinkEvent(unsigned int flags, unsigned int change) {
Device::LinkEvent(flags, change);
if ((flags & IFF_LOWER_UP) != 0 && !link_up_) {
- LOG(INFO) << link_name() << " is up; should start L3!";
+ LOG(INFO) << link_name_ << " is up; should start L3!";
link_up_ = true;
manager_->RegisterService(service_);
if (service_->auto_connect()) {
diff --git a/manager.cc b/manager.cc
index ce7a2a4..c45900b 100644
--- a/manager.cc
+++ b/manager.cc
@@ -250,7 +250,7 @@
}
string Manager::GetActiveProfileName() {
- return ActiveProfile()->name();
+ return ActiveProfile()->GetFriendlyName();
}
} // namespace shill
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 6c3eb9e..47dc5d2 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -181,7 +181,8 @@
TEST_F(ManagerTest, MoveService) {
// 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 Profile(&control_interface_, &glib_, &manager_));
+ ProfileRefPtr profile(
+ new MockProfile(&control_interface_, &glib_, &manager_, ""));
{
ServiceRefPtr s2(
new MockService(&control_interface_,
diff --git a/mock_profile.cc b/mock_profile.cc
index 628d9d2..0fc99a0 100644
--- a/mock_profile.cc
+++ b/mock_profile.cc
@@ -21,7 +21,14 @@
MockProfile::MockProfile(ControlInterface *control_interface,
GLib *glib,
Manager *manager)
- : Profile(control_interface, glib, manager) {
+ : Profile(control_interface, glib, manager, Identifier("mock"), false) {
+}
+
+MockProfile::MockProfile(ControlInterface *control_interface,
+ GLib *glib,
+ Manager *manager,
+ const std::string &identifier)
+ : Profile(control_interface, glib, manager, Identifier(identifier), false) {
}
MockProfile::~MockProfile() {}
diff --git a/mock_profile.h b/mock_profile.h
index 1a4575e..c87709f 100644
--- a/mock_profile.h
+++ b/mock_profile.h
@@ -23,10 +23,12 @@
MockProfile(ControlInterface *control_interface,
GLib *glib,
Manager *manager);
+ MockProfile(ControlInterface *control_interface,
+ GLib *glib,
+ Manager *manager,
+ const std::string &identifier);
virtual ~MockProfile();
- MOCK_METHOD1(MoveToActiveProfile, bool(const std::string &));
-
private:
DISALLOW_COPY_AND_ASSIGN(MockProfile);
};
diff --git a/profile.cc b/profile.cc
index cf6fab7..ef5182b 100644
--- a/profile.cc
+++ b/profile.cc
@@ -31,13 +31,18 @@
Profile::Profile(ControlInterface *control_interface,
GLib *glib,
- Manager *manager)
+ Manager *manager,
+ const Identifier &name,
+ bool connect_to_rpc)
: manager_(manager),
- adaptor_(control_interface->CreateProfileAdaptor(this)),
+ name_(name),
storage_(glib) {
+ if (connect_to_rpc)
+ adaptor_.reset(control_interface->CreateProfileAdaptor(this));
+
// flimflam::kCheckPortalListProperty: Registered in DefaultProfile
// flimflam::kCountryProperty: Registered in DefaultProfile
- store_.RegisterConstString(flimflam::kNameProperty, &name_);
+ store_.RegisterConstString(flimflam::kNameProperty, &name_.identifier);
// flimflam::kOfflineModeProperty: Registered in DefaultProfile
// flimflam::kPortalURLProperty: Registered in DefaultProfile
@@ -45,13 +50,21 @@
HelpRegisterDerivedStrings(flimflam::kServicesProperty,
&Profile::EnumerateAvailableServices,
NULL);
- // HelpRegisterDerivedStrings(flimflam::kEntriesProperty,
- // &Profile::EnumerateEntries,
- // NULL);
+ HelpRegisterDerivedStrings(flimflam::kEntriesProperty,
+ &Profile::EnumerateEntries,
+ NULL);
}
Profile::~Profile() {}
+string Profile::GetFriendlyName() {
+ return (name_.user.empty() ? "" : name_.user + "/") + name_.identifier;
+}
+
+string Profile::GetRpcIdentifier() {
+ return adaptor_->GetRpcIdentifier();
+}
+
bool Profile::AdoptService(const ServiceRefPtr &service) {
if (ContainsKey(services_, service->UniqueName()))
return false;
@@ -137,11 +150,6 @@
return true;
}
-string Profile::GetRpcPath(const Identifier &identifier) {
- string user = identifier.user.empty() ? "" : identifier.user + "/";
- return "/profile/" + user + identifier.identifier;
-}
-
bool Profile::GetStoragePath(const Identifier &identifier, FilePath *path) {
FilePath dir(
identifier.user.empty() ?
diff --git a/profile.h b/profile.h
index ff96aaf..7e5b4d1 100644
--- a/profile.h
+++ b/profile.h
@@ -30,6 +30,12 @@
class Profile : public base::RefCounted<Profile> {
public:
struct Identifier {
+ Identifier() {}
+ explicit Identifier(const std::string &i) : identifier(i) {}
+ Identifier(const std::string &u, const std::string &i)
+ : user(u),
+ identifier(i) {
+ }
std::string user; // Empty for global.
std::string identifier;
};
@@ -37,10 +43,16 @@
static const char kGlobalStorageDir[];
static const char kUserStorageDirFormat[];
- Profile(ControlInterface *control_interface, GLib *glib, Manager *manager);
+ Profile(ControlInterface *control_interface,
+ GLib *glib,
+ Manager *manager,
+ const Identifier &name,
+ bool connect_to_rpc);
virtual ~Profile();
- const std::string &name() { return name_; }
+ std::string GetFriendlyName();
+
+ std::string GetRpcIdentifier();
PropertyStore *store() { return &store_; }
@@ -76,11 +88,6 @@
// on success.
static bool ParseIdentifier(const std::string &raw, Identifier *parsed);
- // Returns the RPC object path for a profile identified by
- // |identifier|. |identifier| must be a valid identifier, possibly parsed and
- // validated through Profile::ParseIdentifier.
- static std::string GetRpcPath(const Identifier &identifier);
-
// Sets |path| to the persistent store file path for a profile identified by
// |identifier|. Returns true on success, and false if unable to determine an
// appropriate file location. |identifier| must be a valid identifier,
@@ -113,13 +120,14 @@
Strings(Profile::*get)(void),
bool(Profile::*set)(const Strings&));
- scoped_ptr<ProfileAdaptorInterface> adaptor_;
+
+ // Properties to be gotten via PropertyStore calls.
+ Identifier name_;
// Persistent store associated with the profile.
KeyFileStore storage_;
- // Properties to be get/set via PropertyStore calls.
- std::string name_;
+ scoped_ptr<ProfileAdaptorInterface> adaptor_;
DISALLOW_COPY_AND_ASSIGN(Profile);
};
diff --git a/profile_dbus_adaptor.cc b/profile_dbus_adaptor.cc
index 6ac46a8..ce55f45 100644
--- a/profile_dbus_adaptor.cc
+++ b/profile_dbus_adaptor.cc
@@ -26,7 +26,7 @@
const char ProfileDBusAdaptor::kPath[] = "/profile/";
ProfileDBusAdaptor::ProfileDBusAdaptor(DBus::Connection* conn, Profile *profile)
- : DBusAdaptor(conn, kPath),
+ : DBusAdaptor(conn, kPath + profile->GetFriendlyName()),
profile_(profile) {
}
diff --git a/profile_unittest.cc b/profile_unittest.cc
index 115afe8..87e721c 100644
--- a/profile_unittest.cc
+++ b/profile_unittest.cc
@@ -25,7 +25,7 @@
class ProfileTest : public PropertyStoreTest {
public:
ProfileTest()
- : profile_(new Profile(&control_interface_, &glib_, &manager_)) {
+ : profile_(new MockProfile(&control_interface_, &glib_, &manager_, "")) {
}
protected:
@@ -74,16 +74,17 @@
EXPECT_EQ(kIdentifier2, identifier.identifier);
}
-TEST_F(ProfileTest, GetRpcPath) {
+TEST_F(ProfileTest, GetFriendlyName) {
static const char kUser[] = "theUser";
static const char kIdentifier[] = "theIdentifier";
- static const char kPathPrefix[] = "/profile/";
- Profile::Identifier identifier;
- identifier.identifier = kIdentifier;
- EXPECT_EQ(string(kPathPrefix) + kIdentifier, Profile::GetRpcPath(identifier));
- identifier.user = kUser;
- EXPECT_EQ(string(kPathPrefix) + kUser + "/" + kIdentifier,
- Profile::GetRpcPath(identifier));
+ Profile::Identifier id;
+ id.identifier = kIdentifier;
+ ProfileRefPtr profile(
+ new Profile(&control_interface_, &glib_, &manager_, id, false));
+ EXPECT_EQ(kIdentifier, profile->GetFriendlyName());
+ id.user = kUser;
+ profile = new Profile(&control_interface_, &glib_, &manager_, id, false);
+ EXPECT_EQ(string(kUser) + "/" + kIdentifier, profile->GetFriendlyName());
}
TEST_F(ProfileTest, GetStoragePath) {
diff --git a/wifi.cc b/wifi.cc
index 8428c45..69deb00 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -72,7 +72,7 @@
// kind of thing.
store_.RegisterConstBool(flimflam::kScanningProperty, &scan_pending_);
store_.RegisterUint16(flimflam::kScanIntervalProperty, &scan_interval_);
- VLOG(2) << "WiFi device " << link_name() << " initialized.";
+ VLOG(2) << "WiFi device " << link_name_ << " initialized.";
}
WiFi::~WiFi() {}
@@ -86,7 +86,7 @@
try {
std::map<string, DBus::Variant> create_interface_args;
create_interface_args["Ifname"].writer().
- append_string(link_name().c_str());
+ append_string(link_name_.c_str());
create_interface_args["Driver"].writer().
append_string(kSupplicantWiFiDriver);
// TODO(quiche) create_interface_args["ConfigFile"].writer().append_string
@@ -96,7 +96,7 @@
} catch (const DBus::Error e) { // NOLINT
if (!strcmp(e.name(), kSupplicantErrorInterfaceExists)) {
interface_path =
- supplicant_process_proxy_->GetInterface(link_name());
+ supplicant_process_proxy_->GetInterface(link_name_);
// XXX crash here, if device missing?
} else {
// XXX