shill: Implement RemoveProfile
Also clean up the comment for Profile::set_storage()
BUG=chromium-os:24461
TEST=New unit tests; autotests to come
Change-Id: If092d09c7cc1b3312bdbece8ee00a128cc3e427b
Reviewed-on: https://gerrit.chromium.org/gerrit/19334
Reviewed-by: Darin Petkov <petkov@chromium.org>
Commit-Ready: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/manager.cc b/manager.cc
index 4aebe69..db587a6 100644
--- a/manager.cc
+++ b/manager.cc
@@ -224,7 +224,7 @@
this,
ident,
user_storage_format_,
- connect_profiles_to_rpc_);
+ false);
}
if (!profile->InitStorage(glib_, Profile::kCreateNew, error)) {
@@ -366,6 +366,47 @@
PopProfileInternal();
}
+void Manager::RemoveProfile(const string &name, Error *error) {
+ Profile::Identifier ident;
+ if (!Profile::ParseIdentifier(name, &ident)) {
+ Error::PopulateAndLog(error, Error::kInvalidArguments,
+ "Invalid profile name " + name);
+ return;
+ }
+
+ for (vector<ProfileRefPtr>::const_iterator it = profiles_.begin();
+ it != profiles_.end();
+ ++it) {
+ if ((*it)->MatchesIdentifier(ident)) {
+ Error::PopulateAndLog(error, Error::kInvalidArguments,
+ "Cannot remove profile name " + name +
+ " since it is on stack");
+ return;
+ }
+ }
+
+ ProfileRefPtr profile;
+ if (ident.user.empty()) {
+ profile = new DefaultProfile(control_interface_,
+ this,
+ storage_path_,
+ ident.identifier,
+ props_);
+ } else {
+ profile = new Profile(control_interface_,
+ this,
+ ident,
+ user_storage_format_,
+ false);
+ }
+
+
+ // |error| will have been populated if RemoveStorage fails.
+ profile->RemoveStorage(glib_, error);
+
+ return;
+}
+
bool Manager::HandleProfileEntryDeletion(const ProfileRefPtr &profile,
const std::string &entry_name) {
bool moved_services = false;
diff --git a/manager.h b/manager.h
index 0cb9af1..53dd223 100644
--- a/manager.h
+++ b/manager.h
@@ -114,6 +114,8 @@
void PopProfile(const std::string &name, Error *error);
// Remove the active profile.
void PopAnyProfile(Error *error);
+ // Remove the underlying persistent storage for a profile.
+ void RemoveProfile(const std::string &name, Error *error);
// Handle the event where a profile is about to remove a profile entry.
// Any Services that are dependent on this storage identifier will need
// to find new profiles. Return true if any service has been moved to a new
diff --git a/manager_dbus_adaptor.cc b/manager_dbus_adaptor.cc
index 62f3f84..f3c4318 100644
--- a/manager_dbus_adaptor.cc
+++ b/manager_dbus_adaptor.cc
@@ -107,8 +107,11 @@
return ::DBus::Path(path);
}
-void ManagerDBusAdaptor::RemoveProfile(const string &/*name*/,
- ::DBus::Error &/*error*/) {
+void ManagerDBusAdaptor::RemoveProfile(const string &name,
+ ::DBus::Error &error) {
+ Error e;
+ manager_->RemoveProfile(name, &e);
+ e.ToDBusError(&error);
}
::DBus::Path ManagerDBusAdaptor::PushProfile(const std::string &name,
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 44ecbf8..31a13f1 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -8,6 +8,7 @@
#include <glib.h>
+#include <base/file_util.h>
#include <base/logging.h>
#include <base/scoped_temp_dir.h>
#include <base/stl_util.h>
@@ -716,6 +717,69 @@
EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kMachineProfile1));
}
+TEST_F(ManagerTest, RemoveProfile) {
+ // It's much easier to use real Glib in creating a Manager for this
+ // test here since we want the storage side-effects.
+ GLib glib;
+ ScopedTempDir temp_dir;
+ ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+ Manager manager(control_interface(),
+ dispatcher(),
+ metrics(),
+ &glib,
+ run_path(),
+ storage_path(),
+ temp_dir.path().value());
+
+ const char kProfile0[] = "profile0";
+ FilePath profile_path(
+ FilePath(storage_path()).Append(string(kProfile0) + ".profile"));
+
+ ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kProfile0));
+ ASSERT_TRUE(file_util::PathExists(profile_path));
+
+ EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile0));
+
+ // Remove should fail since the profile is still on the stack.
+ {
+ Error error;
+ manager.RemoveProfile(kProfile0, &error);
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+ }
+
+ // Profile path should still exist.
+ EXPECT_TRUE(file_util::PathExists(profile_path));
+
+ EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager));
+
+ // This should succeed now that the profile is off the stack.
+ {
+ Error error;
+ manager.RemoveProfile(kProfile0, &error);
+ EXPECT_EQ(Error::kSuccess, error.type());
+ }
+
+ // Profile path should no longer exist.
+ EXPECT_FALSE(file_util::PathExists(profile_path));
+
+ // Another remove succeeds, due to a foible in file_util::Delete --
+ // it is not an error to delete a file that does not exist.
+ {
+ Error error;
+ manager.RemoveProfile(kProfile0, &error);
+ EXPECT_EQ(Error::kSuccess, error.type());
+ }
+
+ // Let's create an error case that will "work". Create a non-empty
+ // directory in the place of the profile pathname.
+ ASSERT_TRUE(file_util::CreateDirectory(profile_path.Append("foo")));
+ {
+ Error error;
+ manager.RemoveProfile(kProfile0, &error);
+ EXPECT_EQ(Error::kOperationFailed, error.type());
+ }
+}
+
// Use this matcher instead of passing RefPtrs directly into the arguments
// of EXPECT_CALL() because otherwise we may create un-cleaned-up references at
// system teardown.
diff --git a/profile.cc b/profile.cc
index 0533a03..f3d7dd2 100644
--- a/profile.cc
+++ b/profile.cc
@@ -9,6 +9,7 @@
#include <vector>
#include <base/file_path.h>
+#include <base/file_util.h>
#include <base/logging.h>
#include <base/stl_util.h>
#include <base/string_util.h>
@@ -100,6 +101,29 @@
return true;
}
+bool Profile::RemoveStorage(GLib *glib, Error *error) {
+ FilePath path;
+
+ CHECK(!storage_.get());
+
+ if (!GetStoragePath(&path)) {
+ Error::PopulateAndLog(
+ error, Error::kInvalidArguments,
+ base::StringPrintf("Could get the storage path for %s:%s",
+ name_.user.c_str(), name_.identifier.c_str()));
+ return false;
+ }
+
+ if (!file_util::Delete(path, false)) {
+ Error::PopulateAndLog(
+ error, Error::kOperationFailed,
+ base::StringPrintf("Could not remove path %s", path.value().c_str()));
+ return false;
+ }
+
+ return true;
+}
+
string Profile::GetFriendlyName() {
return (name_.user.empty() ? "" : name_.user + "/") + name_.identifier;
}
diff --git a/profile.h b/profile.h
index 08fa179..80895d2 100644
--- a/profile.h
+++ b/profile.h
@@ -58,6 +58,11 @@
InitStorageOption storage_option,
Error *error);
+ // Remove the persisitent storage for this Profile. It is an error to
+ // do so while the underlying storage is open via InitStorage() or
+ // set_storage().
+ bool RemoveStorage(GLib *glib, Error *error);
+
std::string GetFriendlyName();
virtual std::string GetRpcIdentifier();
@@ -65,7 +70,9 @@
PropertyStore *mutable_store() { return &store_; }
const PropertyStore &store() const { return store_; }
- void set_storage(StoreInterface *storage); // Takes ownership of |storage|.
+ // Set the storage inteface. This is used for testing purposes. It
+ // takes ownership of |storage|.
+ void set_storage(StoreInterface *storage);
// Begin managing the persistence of |service|.
// Returns true if |service| is new to this profile and was added,