Revisit the simple_key_value_store into a class.
The simple_key_value_store was implemented as two separated functions
to parse and assemble a string containing several lines of key=value
pairs. The representation of that was passed to the caller as a
map<string, string> who would use the map operations to modify it.
Also, the inteded use for these strings was to parse and write text
files on the filesystem.
This key=value store is used to store strings and boolean values,
and will be reused for the policy manager config provider.
This patch reworks those functions as a class and adds support for
reading and writing boolean values and does the file read and write
operations as well.
BUG=chromium:359674
TEST=Unittest extended.
Change-Id: I4890c4a4ca81c1a4857e9893ea827c3fa7815aab
Reviewed-on: https://chromium-review.googlesource.com/195489
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/hwid_override.cc b/hwid_override.cc
index 2c648c0..600a012 100644
--- a/hwid_override.cc
+++ b/hwid_override.cc
@@ -25,13 +25,12 @@
HwidOverride::~HwidOverride() {}
std::string HwidOverride::Read(const base::FilePath& root) {
- base::FilePath kFile(root.value() + "/etc/lsb-release");
- string file_data;
- map<string, string> data;
- if (base::ReadFileToString(kFile, &file_data)) {
- data = simple_key_value_store::ParseString(file_data);
- }
- return data[kHwidOverrideKey];
+ KeyValueStore lsb_release;
+ lsb_release.Load(root.value() + "/etc/lsb-release");
+ string result;
+ if (lsb_release.GetString(kHwidOverrideKey, &result))
+ return result;
+ return "";
}
} // namespace chromeos_update_engine
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index 3564b22..667ed46 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -133,19 +133,15 @@
<< ", existing target channel = " << target_channel_
<< ", download channel = " << download_channel_;
TEST_AND_RETURN_FALSE(IsValidChannel(new_target_channel));
+ KeyValueStore lsb_release;
base::FilePath kFile(root_ + kStatefulPartition + "/etc/lsb-release");
- string file_data;
- map<string, string> data;
- if (base::ReadFileToString(kFile, &file_data)) {
- data = simple_key_value_store::ParseString(file_data);
- }
- data[kUpdateChannelKey] = new_target_channel;
- data[kIsPowerwashAllowedKey] = is_powerwash_allowed ? "true" : "false";
- file_data = simple_key_value_store::AssembleString(data);
+
+ lsb_release.Load(kFile.value());
+ lsb_release.SetString(kUpdateChannelKey, new_target_channel);
+ lsb_release.SetBoolean(kIsPowerwashAllowedKey, is_powerwash_allowed);
+
TEST_AND_RETURN_FALSE(base::CreateDirectory(kFile.DirName()));
- TEST_AND_RETURN_FALSE(
- file_util::WriteFile(kFile, file_data.data(), file_data.size()) ==
- static_cast<int>(file_data.size()));
+ TEST_AND_RETURN_FALSE(lsb_release.Save(kFile.value()));
target_channel_ = new_target_channel;
is_powerwash_allowed_ = is_powerwash_allowed;
return true;
@@ -221,13 +217,12 @@
it != files.end(); ++it) {
// TODO(adlr): make sure files checked are owned as root (and all their
// parents are recursively, too).
- string file_data;
- if (!utils::ReadFile(root_ + *it, &file_data))
+ KeyValueStore data;
+ if (!data.Load(root_ + *it))
continue;
- map<string, string> data = simple_key_value_store::ParseString(file_data);
- if (utils::MapContainsKey(data, key)) {
- const string& value = data[key];
+ string value;
+ if (data.GetString(key, &value)) {
if (validator && !CALL_MEMBER_FN(*this, validator)(value)) {
continue;
}
diff --git a/simple_key_value_store.cc b/simple_key_value_store.cc
index 24e9bc9..266d2e4 100644
--- a/simple_key_value_store.cc
+++ b/simple_key_value_store.cc
@@ -3,46 +3,79 @@
// found in the LICENSE file.
#include "update_engine/simple_key_value_store.h"
+
#include <map>
#include <string>
#include <vector>
+
+#include <base/file_util.h>
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
+#include "update_engine/utils.h"
+
using std::map;
using std::string;
using std::vector;
namespace chromeos_update_engine {
-namespace simple_key_value_store {
-// Parses a string.
-map<std::string, std::string> ParseString(const string& str) {
+bool KeyValueStore::Load(const string& filename) {
+ string file_data;
+ if (!base::ReadFileToString(base::FilePath(filename), &file_data))
+ return false;
+
// Split along '\n', then along '='
- std::map<std::string, std::string> ret;
vector<string> lines;
- base::SplitStringDontTrim(str, '\n', &lines);
- for (vector<string>::const_iterator it = lines.begin();
- it != lines.end(); ++it) {
- string::size_type pos = it->find('=');
+ base::SplitStringDontTrim(file_data, '\n', &lines);
+ for (auto& it : lines) {
+ if (it.empty() || it[0] == '#')
+ continue;
+ string::size_type pos = it.find('=');
if (pos == string::npos)
continue;
- ret[it->substr(0, pos)] = it->substr(pos + 1);
+ store_[it.substr(0, pos)] = it.substr(pos + 1);
}
- return ret;
+ return true;
}
-string AssembleString(const std::map<string, string>& data) {
- string ret;
- for (std::map<string, string>::const_iterator it = data.begin();
- it != data.end(); ++it) {
- ret += it->first;
- ret += "=";
- ret += it->second;
- ret += "\n";
- }
- return ret;
+bool KeyValueStore::Save(const string& filename) const {
+ string data;
+ for (auto& it : store_)
+ data += it.first + "=" + it.second + "\n";
+
+ return utils::WriteFile(filename.c_str(), data.c_str(), data.size());
}
-} // namespace simple_key_value_store
+bool KeyValueStore::GetString(const string& key, string* value) const {
+ auto it = store_.find(key);
+ if (it == store_.end())
+ return false;
+ *value = it->second;
+ return true;
+}
+
+void KeyValueStore::SetString(const string& key, const string& value) {
+ store_[key] = value;
+}
+
+bool KeyValueStore::GetBoolean(const string& key, bool* value) const {
+ auto it = store_.find(key);
+ if (it == store_.end())
+ return false;
+ if (it->second == "true") {
+ *value = true;
+ return true;
+ } else if (it-> second == "false") {
+ *value = false;
+ return true;
+ }
+
+ return false;
+}
+
+void KeyValueStore::SetBoolean(const string& key, bool value) {
+ store_[key] = value ? "true" : "false";
+}
+
} // namespace chromeos_update_engine
diff --git a/simple_key_value_store.h b/simple_key_value_store.h
index b8d2da4..eb5a4e2 100644
--- a/simple_key_value_store.h
+++ b/simple_key_value_store.h
@@ -13,14 +13,41 @@
#include <string>
namespace chromeos_update_engine {
-namespace simple_key_value_store {
-// Parses a string.
-std::map<std::string, std::string> ParseString(const std::string& str);
+class KeyValueStore {
+ public:
+ // Creates an empty KeyValueStore.
+ KeyValueStore() {}
-std::string AssembleString(const std::map<std::string, std::string>& data);
+ // Loads the key=value pairs from the given filename. Lines starting with
+ // '#' and empty lines are ignored. Adds all the readed key=values to the
+ // store, overriding those already defined but persisting the ones that
+ // aren't present on the passed file.
+ // Returns whether reading the file succeeded.
+ bool Load(const std::string& filename);
-} // namespace simple_key_value_store
+ // Saves the current store to the given |filename| file. Returns whether the
+ // file creation succeeded.
+ bool Save(const std::string& filename) const;
+
+ // Getter for the given key. Returns whether the key was found on the store.
+ bool GetString(const std::string& key, std::string* value) const;
+
+ // Setter for the given key. It overrides the key if already exists.
+ void SetString(const std::string& key, const std::string& value);
+
+ // Boolean getter. Returns whether the key was found on the store and if it
+ // has a valid value ("true" or "false").
+ bool GetBoolean(const std::string& key, bool* value) const;
+
+ // Boolean setter. Sets the value as "true" or "false".
+ void SetBoolean(const std::string& key, bool value);
+
+ private:
+ // The map storing all the key-value pairs.
+ std::map<std::string, std::string> store_;
+};
+
} // namespace chromeos_update_engine
#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_SIMPLE_KEY_VALUE_STORE_H_
diff --git a/simple_key_value_store_unittest.cc b/simple_key_value_store_unittest.cc
index b85d3ad..ae51be8 100644
--- a/simple_key_value_store_unittest.cc
+++ b/simple_key_value_store_unittest.cc
@@ -2,44 +2,108 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <map>
-#include <string>
-#include <gtest/gtest.h>
-#include <base/strings/string_util.h>
#include "update_engine/simple_key_value_store.h"
+#include <map>
+#include <string>
+
+#include <gtest/gtest.h>
+#include <base/file_util.h>
+#include <base/strings/string_util.h>
+
+#include "update_engine/test_utils.h"
+
+using base::FilePath;
+using base::ReadFileToString;
using std::map;
using std::string;
namespace chromeos_update_engine {
-class SimpleKeyValueStoreTest : public ::testing::Test {};
+class KeyValueStoreTest : public ::testing::Test {
+ protected:
+ ScopedTempFile file_;
+ KeyValueStore store_; // KeyValueStore under test.
+};
-TEST(SimpleKeyValueStoreTest, SimpleTest) {
+TEST_F(KeyValueStoreTest, CommentsAreIgnored) {
+ string blob = "# comment\nA=B\n\n\n#another=comment\n\n";
+ ASSERT_TRUE(WriteFileString(file_.GetPath(), blob));
+ EXPECT_TRUE(store_.Load(file_.GetPath()));
+
+ EXPECT_TRUE(store_.Save(file_.GetPath()));
+ string read_blob;
+ ASSERT_TRUE(ReadFileToString(FilePath(file_.GetPath()), &read_blob));
+ EXPECT_EQ("A=B\n", read_blob);
+}
+
+TEST_F(KeyValueStoreTest, EmptyTest) {
+ ASSERT_TRUE(WriteFileString(file_.GetPath(), ""));
+ EXPECT_TRUE(store_.Load(file_.GetPath()));
+
+ EXPECT_TRUE(store_.Save(file_.GetPath()));
+ string read_blob;
+ ASSERT_TRUE(ReadFileToString(FilePath(file_.GetPath()), &read_blob));
+ EXPECT_EQ("", read_blob);
+}
+
+TEST_F(KeyValueStoreTest, LoadAndReloadTest) {
string blob = "A=B\nC=\n=\nFOO=BAR=BAZ\nBAR=BAX\nMISSING=NEWLINE";
- map<string, string> parts = simple_key_value_store::ParseString(blob);
- string combined = simple_key_value_store::AssembleString(parts);
- map<string, string> combined_parts =
- simple_key_value_store::ParseString(combined);
- map<string, string>* maps[] = { &parts, &combined_parts };
- for (size_t i = 0; i < arraysize(maps); i++) {
- map<string, string>* test_map = maps[i];
- EXPECT_EQ(6, test_map->size()) << "i = " << i;
- EXPECT_EQ("B", (*test_map)["A"]) << "i = " << i;
- EXPECT_EQ("", (*test_map)["C"]) << "i = " << i;
- EXPECT_EQ("", (*test_map)[""]) << "i = " << i;
- EXPECT_EQ("BAR=BAZ", (*test_map)["FOO"]) << "i = " << i;
- EXPECT_EQ("BAX", (*test_map)["BAR"]) << "i = " << i;
- EXPECT_EQ("NEWLINE", (*test_map)["MISSING"]) << "i = " << i;
+ ASSERT_TRUE(WriteFileString(file_.GetPath(), blob));
+ EXPECT_TRUE(store_.Load(file_.GetPath()));
+
+ map<string, string> expected = {
+ {"A", "B"}, {"C", ""}, {"", ""}, {"FOO", "BAR=BAZ"}, {"BAR", "BAX"},
+ {"MISSING", "NEWLINE"}};
+
+ // Test expected values
+ string value;
+ for (auto& it : expected) {
+ EXPECT_TRUE(store_.GetString(it.first, &value));
+ EXPECT_EQ(it.second, value) << "Testing key: " << it.first;
+ }
+
+ // Save, load and test again.
+ EXPECT_TRUE(store_.Save(file_.GetPath()));
+ KeyValueStore new_store;
+ EXPECT_TRUE(new_store.Load(file_.GetPath()));
+
+ for (auto& it : expected) {
+ EXPECT_TRUE(new_store.GetString(it.first, &value)) << "key: " << it.first;
+ EXPECT_EQ(it.second, value) << "key: " << it.first;
}
}
-TEST(SimpleKeyValueStoreTest, EmptyTest) {
- map<string, string> empty_map = simple_key_value_store::ParseString("");
- EXPECT_TRUE(empty_map.empty());
- string str = simple_key_value_store::AssembleString(empty_map);
- if (!str.empty())
- EXPECT_EQ("\n", str); // Optionally may end in newline
+TEST_F(KeyValueStoreTest, SimpleBooleanTest) {
+ bool result;
+ EXPECT_FALSE(store_.GetBoolean("A", &result));
+
+ store_.SetBoolean("A", true);
+ EXPECT_TRUE(store_.GetBoolean("A", &result));
+ EXPECT_TRUE(result);
+
+ store_.SetBoolean("A", false);
+ EXPECT_TRUE(store_.GetBoolean("A", &result));
+ EXPECT_FALSE(result);
+}
+
+TEST_F(KeyValueStoreTest, BooleanParsingTest) {
+ string blob = "TRUE=true\nfalse=false\nvar=false\nDONT_SHOUT=TRUE\n";
+ WriteFileString(file_.GetPath(), blob);
+ EXPECT_TRUE(store_.Load(file_.GetPath()));
+
+ map<string, bool> expected = {
+ {"TRUE", true}, {"false", false}, {"var", false}};
+ bool value;
+ EXPECT_FALSE(store_.GetBoolean("DONT_SHOUT", &value));
+ string str_value;
+ EXPECT_TRUE(store_.GetString("DONT_SHOUT", &str_value));
+
+ // Test expected values
+ for (auto& it : expected) {
+ EXPECT_TRUE(store_.GetBoolean(it.first, &value)) << "key: " << it.first;
+ EXPECT_EQ(it.second, value) << "key: " << it.first;
+ }
}
} // namespace chromeos_update_engine