shill: stop passing around format strings for profiles
Prior to this CL, shill passed around format strings, to specify
where the user profiles should be stored. While this makes things
more configurable, it also a) makes things more complex, and
b) causes g++ to emit warnings that some format strings can't be
checked statically.
The location of user profiles doesn't need the full power of
printf-format style configurability. We really only need the
ability to specify which directory the user profiles are
stored in.
Note that this changes requires updating some unit tests,
which actually did take advantage of the flexibility. In
particular, some of the Manager and Profile unit tests placed
user profiles directly in a user profile directory, rather
than creating a user-specific sub-directory first. Creating
the user-specific directories seems like the right thing to
do, since that it what the normal (non-test) code does.
BUG=chromium:293668
TEST=unit tests
Change-Id: Ic1afeec84a7797105c9a49b8261a9677e17d91ee
Reviewed-on: https://chromium-review.googlesource.com/197061
Tested-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/manager.cc b/manager.cc
index 92d94be..c6a9621 100644
--- a/manager.cc
+++ b/manager.cc
@@ -88,11 +88,11 @@
GLib *glib,
const string &run_directory,
const string &storage_directory,
- const string &user_storage_format)
+ const string &user_storage_directory)
: dispatcher_(dispatcher),
run_path_(FilePath(run_directory)),
storage_path_(FilePath(storage_directory)),
- user_storage_format_(user_storage_format),
+ user_storage_path_(user_storage_directory),
user_profile_list_path_(FilePath(Profile::kUserProfileListPathname)),
adaptor_(control_interface->CreateManagerAdaptor(this)),
device_info_(control_interface, dispatcher, metrics, this),
@@ -332,7 +332,7 @@
metrics_,
this,
ident,
- user_storage_format_,
+ user_storage_path_,
true);
}
@@ -405,7 +405,7 @@
metrics_,
this,
ident,
- user_storage_format_,
+ user_storage_path_,
connect_profiles_to_rpc_);
if (!profile->InitStorage(glib_, Profile::kOpenExisting, error)) {
// |error| will have been populated by InitStorage().
@@ -595,7 +595,7 @@
metrics_,
this,
ident,
- user_storage_format_,
+ user_storage_path_,
false);
}
diff --git a/manager.h b/manager.h
index 4e5ba64..194616f 100644
--- a/manager.h
+++ b/manager.h
@@ -83,7 +83,7 @@
GLib *glib,
const std::string &run_directory,
const std::string &storage_directory,
- const std::string &user_storage_format);
+ const std::string &user_storage_directory);
virtual ~Manager();
void AddDeviceToBlackList(const std::string &device_name);
@@ -553,7 +553,7 @@
EventDispatcher *dispatcher_;
const base::FilePath run_path_;
const base::FilePath storage_path_;
- const std::string user_storage_format_;
+ const std::string user_storage_path_;
base::FilePath user_profile_list_path_; // Changed in tests.
scoped_ptr<ManagerAdaptorInterface> adaptor_;
scoped_ptr<DBusManager> dbus_manager_;
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 335be36..077f3e9 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -198,11 +198,14 @@
}
bool CreateBackingStoreForService(ScopedTempDir *temp_dir,
+ const string &user_identifier,
const string &profile_identifier,
const string &service_name) {
GLib glib;
KeyFileStore store(&glib);
- store.set_path(temp_dir->path().Append(profile_identifier + ".profile"));
+ store.set_path(temp_dir->path().Append(
+ base::StringPrintf("%s/%s.profile", user_identifier.c_str(),
+ profile_identifier.c_str())));
return store.Open() &&
store.SetString(service_name, "rather", "irrelevant") &&
store.Close();
@@ -846,6 +849,7 @@
{
Error error;
string path;
+ ASSERT_TRUE(base::CreateDirectory(temp_dir.path().Append("user")));
manager.CreateProfile(kProfile, &path, &error);
EXPECT_EQ(Error::kSuccess, error.type());
EXPECT_EQ("/profile_rpc", path);
@@ -870,6 +874,9 @@
temp_dir.path().value());
const char kProfile0[] = "~user/profile0";
const char kPurgedMessage[] = "This message should be purged";
+
+ ASSERT_TRUE(base::CreateDirectory(temp_dir.path().Append("user")));
+
// Create a profile and push it on the stack, leave one uncreated
ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kProfile0));
EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile0));
@@ -935,6 +942,7 @@
const char kProfile0[] = "~user/profile0";
const char kProfile1[] = "~user/profile1";
+ ASSERT_TRUE(base::CreateDirectory(temp_dir.path().Append("user")));
// Create a couple of profiles.
ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kProfile0));
@@ -975,7 +983,7 @@
ASSERT_EQ(GetEphemeralProfile(&manager), service->profile());
// Create storage for a profile that contains the service storage name.
- ASSERT_TRUE(CreateBackingStoreForService(&temp_dir, kProfile2Id,
+ ASSERT_TRUE(CreateBackingStoreForService(&temp_dir, "user", kProfile2Id,
kServiceName));
// When we push the profile, the service should move away from the
@@ -988,7 +996,7 @@
// Insert another profile that should supersede ownership of the service.
const char kProfile3Id[] = "profile3";
const string kProfile3 = base::StringPrintf("~user/%s", kProfile3Id);
- ASSERT_TRUE(CreateBackingStoreForService(&temp_dir, kProfile3Id,
+ ASSERT_TRUE(CreateBackingStoreForService(&temp_dir, "user", kProfile3Id,
kServiceName));
EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile3));
EXPECT_EQ(kProfile3, "~" + service->profile()->GetFriendlyName());
@@ -4070,6 +4078,7 @@
const char kProfile1[] = "~user/profile1";
string profile_rpc_path;
Error error;
+ ASSERT_TRUE(base::CreateDirectory(temp_dir.path().Append("user")));
manager.CreateProfile(kProfile0, &profile_rpc_path, &error);
manager.PushProfile(kProfile0, &profile_rpc_path, &error);
manager.CreateProfile(kProfile1, &profile_rpc_path, &error);
@@ -4173,6 +4182,7 @@
const char kProfile0[] = "~user/profile0";
const char kProfile1[] = "~user/profile1";
const char kProfile2[] = "~user/profile2";
+ ASSERT_TRUE(base::CreateDirectory(temp_dir.path().Append("user")));
TestCreateProfile(manager.get(), kProfile0);
TestCreateProfile(manager.get(), kProfile1);
TestCreateProfile(manager.get(), kProfile2);
diff --git a/profile.cc b/profile.cc
index 1de8fe3..0ec0121 100644
--- a/profile.cc
+++ b/profile.cc
@@ -8,7 +8,6 @@
#include <string>
#include <vector>
-#include <base/files/file_path.h>
#include <base/file_util.h>
#include <base/stl_util.h>
#include <base/strings/string_split.h>
@@ -41,12 +40,12 @@
Metrics *metrics,
Manager *manager,
const Identifier &name,
- const string &user_storage_format,
+ const string &user_storage_directory,
bool connect_to_rpc)
: metrics_(metrics),
manager_(manager),
name_(name),
- storage_format_(user_storage_format) {
+ storage_path_(user_storage_directory) {
if (connect_to_rpc)
adaptor_.reset(control_interface->CreateProfileAdaptor(this));
@@ -343,10 +342,10 @@
LOG(ERROR) << "Non-default profiles cannot be stored globally.";
return false;
}
- FilePath dir(base::StringPrintf(storage_format_.c_str(), name_.user.c_str()));
// TODO(petkov): Validate the directory permissions, etc.
- *path = dir.Append(base::StringPrintf("%s.profile",
- name_.identifier.c_str()));
+ *path = storage_path_.Append(
+ base::StringPrintf("%s/%s.profile", name_.user.c_str(),
+ name_.identifier.c_str()));
return true;
}
diff --git a/profile.h b/profile.h
index 7c7e147..8c390ef 100644
--- a/profile.h
+++ b/profile.h
@@ -10,6 +10,7 @@
#include <vector>
#include <base/memory/scoped_ptr.h>
+#include <base/files/file_path.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/event_dispatcher.h"
@@ -60,7 +61,7 @@
Metrics *metrics,
Manager *manager,
const Identifier &name,
- const std::string &user_storage_format,
+ const std::string &user_storage_directory,
bool connect_to_rpc);
virtual ~Profile();
@@ -223,8 +224,8 @@
// Properties to be gotten via PropertyStore calls.
Identifier name_;
- // Format string used to generate paths to user profile directories.
- const std::string storage_format_;
+ // Path to user profile directory.
+ const base::FilePath storage_path_;
// Allows this profile to be backed with on-disk storage.
scoped_ptr<StoreInterface> storage_;
diff --git a/profile_unittest.cc b/profile_unittest.cc
index 0f6f446..247ea07 100644
--- a/profile_unittest.cc
+++ b/profile_unittest.cc
@@ -7,7 +7,6 @@
#include <string>
#include <vector>
-#include <base/files/file_path.h>
#include <base/file_util.h>
#include <base/memory/scoped_ptr.h>
#include <base/strings/stringprintf.h>
@@ -207,7 +206,7 @@
TEST_F(ProfileTest, GetStoragePath) {
static const char kUser[] = "chronos";
static const char kIdentifier[] = "someprofile";
- static const char kFormat[] = "/a/place/for/%s";
+ static const char kDirectory[] = "/a/place/for/";
FilePath path;
Profile::Identifier id(kIdentifier);
ProfileRefPtr profile(
@@ -215,11 +214,10 @@
EXPECT_FALSE(profile->GetStoragePath(&path));
id.user = kUser;
profile =
- new Profile(control_interface(), metrics(), manager(), id, kFormat,
+ new Profile(control_interface(), metrics(), manager(), id, kDirectory,
false);
EXPECT_TRUE(profile->GetStoragePath(&path));
- string suffix = base::StringPrintf("/%s.profile", kIdentifier);
- EXPECT_EQ(base::StringPrintf(kFormat, kUser) + suffix, path.value());
+ EXPECT_EQ("/a/place/for/chronos/someprofile.profile", path.value());
}
TEST_F(ProfileTest, ServiceManagement) {
@@ -411,6 +409,8 @@
TEST_F(ProfileTest, InitStorage) {
Profile::Identifier id("theUser", "theIdentifier");
+ ASSERT_TRUE(base::CreateDirectory(
+ base::FilePath(storage_path()).Append("theUser")));
// Profile doesn't exist but we wanted it to.
EXPECT_FALSE(ProfileInitStorage(id, Profile::kOpenExisting, false,
@@ -445,9 +445,9 @@
Error::kSuccess));
// Corrupt the profile storage.
- string suffix(base::StringPrintf("/%s.profile", id.identifier.c_str()));
FilePath final_path(
- base::StringPrintf(storage_path().c_str(), id.user.c_str()) + suffix);
+ base::StringPrintf("%s/%s/%s.profile", storage_path().c_str(),
+ id.user.c_str(), id.identifier.c_str()));
string data = "]corrupt_data[";
EXPECT_EQ(data.size(),
file_util::WriteFile(final_path, data.data(), data.size()));
diff --git a/shill_config.cc b/shill_config.cc
index 6ef23f0..8780d0c 100644
--- a/shill_config.cc
+++ b/shill_config.cc
@@ -11,7 +11,7 @@
// static
const char Config::kDefaultStorageDirectory[] = "/var/cache/shill";
// static
-const char Config::kDefaultUserStorageFormat[] = RUNDIR "/user_profiles/%s";
+const char Config::kDefaultUserStorageDirectory[] = RUNDIR "/user_profiles/";
Config::Config() {}
@@ -25,8 +25,8 @@
return kDefaultStorageDirectory;
}
-std::string Config::GetUserStorageDirectoryFormat() {
- return kDefaultUserStorageFormat;
+std::string Config::GetUserStorageDirectory() {
+ return kDefaultUserStorageDirectory;
}
} // namespace shill
diff --git a/shill_config.h b/shill_config.h
index d09adba..97cdfd9 100644
--- a/shill_config.h
+++ b/shill_config.h
@@ -18,12 +18,12 @@
virtual std::string GetRunDirectory();
virtual std::string GetStorageDirectory();
- virtual std::string GetUserStorageDirectoryFormat();
+ virtual std::string GetUserStorageDirectory();
private:
static const char kDefaultRunDirectory[];
static const char kDefaultStorageDirectory[];
- static const char kDefaultUserStorageFormat[];
+ static const char kDefaultUserStorageDirectory[];
DISALLOW_COPY_AND_ASSIGN(Config);
};
diff --git a/shill_daemon.cc b/shill_daemon.cc
index 589e45c..14d74c2 100644
--- a/shill_daemon.cc
+++ b/shill_daemon.cc
@@ -48,7 +48,7 @@
&glib_,
config->GetRunDirectory(),
config->GetStorageDirectory(),
- config->GetUserStorageDirectoryFormat())),
+ config->GetUserStorageDirectory())),
callback80211_metrics_(metrics_.get()) {
}