shill: improve out-of-disk handlng
At present, shill deals poorly with the case where no default profile
exists, and no default profile can be created. In particular, it aborts
in this case. Consequently if the disk is full, and no default profile
is present, shill gets in to a crash loop.
Remedy this by adding a StubStorage class, and using this to back
the DefaultProfile, if we can't back the DefaultProfile with a file
on disk.
One of the alternatives considered was to back the DefaultProfile
using a file in /tmp. This approach seems simpler, in that we don't
need to worry about securing the file in /tmp, or about the
possibility that /tmp is also full.
While there:
- Add DISALLOW_COPY_AND_ASSIGN to the Manager class.
- Make Manager::LoadProperties and Profile::LoadManagerProperties
return void instead of bool. Previously, Profile::LoadManagerProperties
would always return true. That would cause Manager::LoadProperties
to always return true. Seems simpler to return void.
- Fix a grammar nit in profile.cc.
- Fix a typo in profile.h.
BUG=chromium:355140
TEST=network_DiskFull, unit tests
Note that the network_DiskFull may report failures due to
metrics_daemon not running. However, the test should not show
any errors related to shill or flimflam.
Change-Id: Ic081a40d7680ce035ead1459a08bf63e7989f0d6
Reviewed-on: https://chromium-review.googlesource.com/193693
Tested-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/default_profile.cc b/default_profile.cc
index b79ea52..7987c11 100644
--- a/default_profile.cc
+++ b/default_profile.cc
@@ -78,7 +78,7 @@
DefaultProfile::~DefaultProfile() {}
-bool DefaultProfile::LoadManagerProperties(Manager::Properties *manager_props) {
+void DefaultProfile::LoadManagerProperties(Manager::Properties *manager_props) {
storage()->GetBool(kStorageId, kStorageArpGateway,
&manager_props->arp_gateway);
storage()->GetString(kStorageId, kStorageHostName, &manager_props->host_name);
@@ -113,7 +113,6 @@
manager_props->portal_check_interval_seconds =
PortalDetector::kDefaultCheckIntervalSeconds;
}
- return true;
}
bool DefaultProfile::ConfigureService(const ServiceRefPtr &service) {
diff --git a/default_profile.h b/default_profile.h
index 387d3d0..46f91cb 100644
--- a/default_profile.h
+++ b/default_profile.h
@@ -37,7 +37,7 @@
// Loads global configuration into manager properties. This should
// only be called by the Manager.
- virtual bool LoadManagerProperties(Manager::Properties *manager_props);
+ virtual void LoadManagerProperties(Manager::Properties *manager_props);
// Override the Profile superclass implementation to accept all Ethernet
// services, since these should have an affinity for the default profile.
diff --git a/default_profile_unittest.cc b/default_profile_unittest.cc
index b03cbb4..edbcfed 100644
--- a/default_profile_unittest.cc
+++ b/default_profile_unittest.cc
@@ -198,7 +198,7 @@
profile_->set_storage(storage.release());
- ASSERT_TRUE(profile_->LoadManagerProperties(&manager_props));
+ profile_->LoadManagerProperties(&manager_props);
EXPECT_TRUE(manager_props.arp_gateway);
EXPECT_EQ("", manager_props.host_name);
EXPECT_FALSE(manager_props.offline_mode);
@@ -262,7 +262,7 @@
profile_->set_storage(storage.release());
Manager::Properties manager_props;
- ASSERT_TRUE(profile_->LoadManagerProperties(&manager_props));
+ profile_->LoadManagerProperties(&manager_props);
EXPECT_FALSE(manager_props.arp_gateway);
EXPECT_EQ(host_name, manager_props.host_name);
EXPECT_TRUE(manager_props.offline_mode);
diff --git a/manager.cc b/manager.cc
index 667ce2e..b3a62b1 100644
--- a/manager.cc
+++ b/manager.cc
@@ -304,7 +304,7 @@
CHECK(Profile::ParseIdentifier(
DefaultProfile::kDefaultId, &default_profile_id));
PushProfileInternal(default_profile_id, &path, &error);
- CHECK(error.IsSuccess()); // Must have a default profile.
+ CHECK(!profiles_.empty()); // Must have a default profile.
// Push user profiles onto the stack.
for (const auto &profile_id : identifiers) {
@@ -399,17 +399,14 @@
storage_path_,
ident.identifier,
props_));
- if (!default_profile->InitStorage(glib_, Profile::kOpenExisting, error)) {
- // |error| will have been populated by InitStorage().
- return;
+ if (!default_profile->InitStorage(glib_, Profile::kOpenExisting, NULL)) {
+ LOG(ERROR) << "Failed to open default profile.";
+ // Try to continue anyway, so that we can be useful in cases
+ // where the disk is full.
+ default_profile->InitStubStorage();
}
- if (!LoadProperties(default_profile)) {
- Error::PopulateAndLog(error, Error::kInvalidArguments,
- "Could not load Manager properties from profile " +
- Profile::IdentifierToString(ident));
- return;
- }
+ LoadProperties(default_profile);
profile = default_profile;
} else {
profile = new Profile(control_interface_,
@@ -1088,12 +1085,9 @@
}
}
-bool Manager::LoadProperties(const scoped_refptr<DefaultProfile> &profile) {
- if (!profile->LoadManagerProperties(&props_)) {
- return false;
- }
+void Manager::LoadProperties(const scoped_refptr<DefaultProfile> &profile) {
+ profile->LoadManagerProperties(&props_);
SetIgnoredDNSSearchPaths(props_.ignored_dns_search_paths, NULL);
- return true;
}
void Manager::AddTerminationAction(const string &name,
diff --git a/manager.h b/manager.h
index b426289..88d3cd6 100644
--- a/manager.h
+++ b/manager.h
@@ -9,6 +9,7 @@
#include <string>
#include <vector>
+#include <base/basictypes.h>
#include <base/cancelable_callback.h>
#include <base/file_path.h>
#include <base/memory/ref_counted.h>
@@ -457,7 +458,7 @@
bool UnloadService(std::vector<ServiceRefPtr>::iterator *service_iterator);
// Load Manager default properties from |profile|.
- bool LoadProperties(const scoped_refptr<DefaultProfile> &profile);
+ void LoadProperties(const scoped_refptr<DefaultProfile> &profile);
// Configure the device with profile data from all current profiles.
void LoadDeviceFromProfiles(const DeviceRefPtr &device);
@@ -618,6 +619,8 @@
// Stores the state of the highest ranked connected service.
std::string connection_state_;
+
+ DISALLOW_COPY_AND_ASSIGN(Manager);
};
} // namespace shill
diff --git a/manager_unittest.cc b/manager_unittest.cc
index d75315c..845b15b 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -912,12 +912,26 @@
run_path(),
storage_path(),
temp_dir.path().value());
+ vector<ProfileRefPtr> &profiles = GetProfiles(&manager);
// Pushing an invalid profile should fail.
EXPECT_EQ(Error::kInvalidArguments, TestPushProfile(&manager, ""));
- // Pushing a default profile that does not exist should fail.
- EXPECT_EQ(Error::kNotFound, TestPushProfile(&manager, "default"));
+ // Create and push a default profile. Should succeed.
+ const char kDefaultProfile0[] = "default";
+ ASSERT_EQ(Error::kSuccess, TestCreateProfile(&manager, kDefaultProfile0));
+ EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kDefaultProfile0));
+ EXPECT_EQ(Error::kSuccess, TestPopProfile(&manager, kDefaultProfile0));
+
+ // Pushing a default profile that does not exist on disk will _not_
+ // fail, because we'll use temporary storage for it.
+ const char kMissingDefaultProfile[] = "missingdefault";
+ EXPECT_EQ(Error::kSuccess,
+ TestPushProfile(&manager, kMissingDefaultProfile));
+ EXPECT_EQ(1, profiles.size());
+ EXPECT_EQ(Error::kSuccess,
+ TestPopProfile(&manager, kMissingDefaultProfile));
+ EXPECT_EQ(0, profiles.size());
const char kProfile0[] = "~user/profile0";
const char kProfile1[] = "~user/profile1";
@@ -997,7 +1011,7 @@
// The service should now revert to the ephemeral profile.
EXPECT_EQ(GetEphemeralProfile(&manager), service->profile());
- // Pop the remaining two services off the stack.
+ // Pop the remaining two profiles off the stack.
EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager));
EXPECT_EQ(Error::kSuccess, TestPopAnyProfile(&manager));
@@ -1027,7 +1041,6 @@
// Add two user profiles to the top of the stack.
EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile0));
EXPECT_EQ(Error::kSuccess, TestPushProfile(&manager, kProfile1));
- vector<ProfileRefPtr> &profiles = GetProfiles(&manager);
EXPECT_EQ(4, profiles.size());
// PopAllUserProfiles should remove both user profiles, leaving the two
diff --git a/profile.cc b/profile.cc
index b28fbb3..911be12 100644
--- a/profile.cc
+++ b/profile.cc
@@ -24,6 +24,7 @@
#include "shill/property_accessor.h"
#include "shill/service.h"
#include "shill/store_interface.h"
+#include "shill/stub_storage.h"
using base::FilePath;
using std::set;
@@ -94,7 +95,7 @@
base::StringPrintf("Could not open profile storage for %s:%s",
name_.user.c_str(), name_.identifier.c_str()));
if (already_exists) {
- // The profile contents is corrupt, or we do not have access to
+ // The profile contents are corrupt, or we do not have access to
// this file. Move this file out of the way so a future open attempt
// will succeed, assuming the failure reason was the former.
storage->MarkAsCorrupted();
@@ -115,6 +116,10 @@
return true;
}
+void Profile::InitStubStorage() {
+ set_storage(new StubStorage());
+}
+
bool Profile::RemoveStorage(GLib *glib, Error *error) {
FilePath path;
diff --git a/profile.h b/profile.h
index d3b72d6..7c7e147 100644
--- a/profile.h
+++ b/profile.h
@@ -70,7 +70,11 @@
InitStorageOption storage_option,
Error *error);
- // Remove the persisitent storage for this Profile. It is an error to
+ // Set up stub storage for this Profile. The data will NOT be
+ // persisted. In most cases, you should prefer InitStorage.
+ void InitStubStorage();
+
+ // Remove the persistent 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);
diff --git a/stub_storage.h b/stub_storage.h
new file mode 100644
index 0000000..8a358b8
--- /dev/null
+++ b/stub_storage.h
@@ -0,0 +1,104 @@
+// Copyright (c) 2014 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.
+
+#ifndef SHILL_STUB_STORAGE_
+#define SHILL_STUB_STORAGE_
+
+#include <set>
+#include <string>
+#include <vector>
+
+#include "shill/store_interface.h"
+
+namespace shill {
+
+// A stub implementation of StoreInterface.
+class StubStorage : public StoreInterface {
+ public:
+ virtual ~StubStorage() {}
+
+ virtual bool Flush() override { return false; }
+ virtual std::set<std::string> GetGroups() const override {
+ return {};
+ }
+ virtual std::set<std::string> GetGroupsWithKey(
+ const std::string &key) const override {
+ return {};
+ }
+ virtual std::set<std::string> GetGroupsWithProperties(
+ const KeyValueStore &properties) const override {
+ return {};
+ }
+ virtual bool ContainsGroup(const std::string &group) const override {
+ return false;
+ }
+ virtual bool DeleteKey(const std::string &group, const std::string &key)
+ override { return false; }
+ virtual bool DeleteGroup(const std::string &group) override { return false; }
+ virtual bool SetHeader(const std::string &header) override { return false; }
+ virtual bool GetString(const std::string &group,
+ const std::string &key,
+ std::string *value) const override {
+ return false;
+ }
+ virtual bool SetString(const std::string &group,
+ const std::string &key,
+ const std::string &value) override {
+ return false;
+ }
+ virtual bool GetBool(const std::string &group,
+ const std::string &key,
+ bool *value) const override {
+ return false;
+ }
+ virtual bool SetBool(const std::string &group,
+ const std::string &key,
+ bool value) override {
+ return false;
+ }
+ virtual bool GetInt(const std::string &group,
+ const std::string &key,
+ int *value) const override {
+ return false;
+ }
+ virtual bool SetInt(const std::string &group,
+ const std::string &key,
+ int value) override {
+ return false;
+ }
+ virtual bool GetUint64(const std::string &group,
+ const std::string &key,
+ uint64 *value) const override {
+ return false;
+ }
+ virtual bool SetUint64(const std::string &group,
+ const std::string &key,
+ uint64 value) override {
+ return false;
+ }
+ virtual bool GetStringList(const std::string &group,
+ const std::string &key,
+ std::vector<std::string> *value) const override {
+ return false;
+ }
+ virtual bool SetStringList(const std::string &group,
+ const std::string &key,
+ const std::vector<std::string> &value) override {
+ return false;
+ }
+ virtual bool GetCryptedString(const std::string &group,
+ const std::string &key,
+ std::string *value) override {
+ return false;
+ }
+ virtual bool SetCryptedString(const std::string &group,
+ const std::string &key,
+ const std::string &value) override {
+ return false;
+ }
+};
+
+} // namespace shill
+
+#endif // SHILL_STUB_STORAGE_