Merge: Add write callback into SaveSettings function
Saving critical settings needs confirmation.
When command alters device config, it should be set "Done" only after
settings are actually saved.
BUG:25776798
Reviewed-on: https://weave-review.googlesource.com/2199
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
(cherry picked from commit 42e508f2559e019d2fcc8f88adfd184b7a6bc3a4)
Change-Id: I693e3c17b3f2f707c8df7af29eefd48362980bce
Reviewed-on: https://weave-review.googlesource.com/2421
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/examples/daemon/common/daemon.h b/examples/daemon/common/daemon.h
index 4cccff3..6dc021d 100644
--- a/examples/daemon/common/daemon.h
+++ b/examples/daemon/common/daemon.h
@@ -69,10 +69,11 @@
};
Daemon(const Options& opts)
- : config_store_{new weave::examples::FileConfigStore(
- opts.disable_security_,
- opts.model_id_)},
- task_runner_{new weave::examples::EventTaskRunner},
+ : task_runner_{new weave::examples::EventTaskRunner},
+ config_store_{
+ new weave::examples::FileConfigStore(opts.disable_security_,
+ opts.model_id_,
+ task_runner_.get())},
http_client_{new weave::examples::CurlHttpClient(task_runner_.get())},
network_{new weave::examples::EventNetworkImpl(task_runner_.get())},
bluetooth_{new weave::examples::BluetoothImpl} {
@@ -114,8 +115,8 @@
LOG(INFO) << "Device registered: " << device->GetSettings().cloud_id;
}
- std::unique_ptr<weave::examples::FileConfigStore> config_store_;
std::unique_ptr<weave::examples::EventTaskRunner> task_runner_;
+ std::unique_ptr<weave::examples::FileConfigStore> config_store_;
std::unique_ptr<weave::examples::CurlHttpClient> http_client_;
std::unique_ptr<weave::examples::EventNetworkImpl> network_;
std::unique_ptr<weave::examples::BluetoothImpl> bluetooth_;
diff --git a/examples/provider/file_config_store.cc b/examples/provider/file_config_store.cc
index af887a7..31efaa7 100644
--- a/examples/provider/file_config_store.cc
+++ b/examples/provider/file_config_store.cc
@@ -12,14 +12,19 @@
#include <string>
#include <vector>
+#include <base/bind.h>
+
namespace weave {
namespace examples {
const char kSettingsDir[] = "/var/lib/weave/";
FileConfigStore::FileConfigStore(bool disable_security,
- const std::string& model_id)
- : disable_security_{disable_security}, model_id_{model_id} {}
+ const std::string& model_id,
+ provider::TaskRunner* task_runner)
+ : disable_security_{disable_security},
+ model_id_{model_id},
+ task_runner_{task_runner} {}
std::string FileConfigStore::GetPath(const std::string& name) const {
std::string path{kSettingsDir};
@@ -72,11 +77,14 @@
}
void FileConfigStore::SaveSettings(const std::string& name,
- const std::string& settings) {
+ const std::string& settings,
+ const DoneCallback& callback) {
CHECK(mkdir(kSettingsDir, S_IRWXU) == 0 || errno == EEXIST);
LOG(INFO) << "Saving settings to " << GetPath(name);
std::ofstream str(GetPath(name));
str << settings;
+ if (!callback.is_null())
+ task_runner_->PostDelayedTask(FROM_HERE, base::Bind(callback, nullptr), {});
}
} // namespace examples
diff --git a/examples/provider/file_config_store.h b/examples/provider/file_config_store.h
index 214194e..e7398d1 100644
--- a/examples/provider/file_config_store.h
+++ b/examples/provider/file_config_store.h
@@ -10,18 +10,22 @@
#include <vector>
#include <weave/provider/config_store.h>
+#include <weave/provider/task_runner.h>
namespace weave {
namespace examples {
class FileConfigStore : public provider::ConfigStore {
public:
- FileConfigStore(bool disable_security, const std::string& model_id);
+ FileConfigStore(bool disable_security,
+ const std::string& model_id,
+ provider::TaskRunner* task_runner);
bool LoadDefaults(Settings* settings) override;
std::string LoadSettings(const std::string& name) override;
void SaveSettings(const std::string& name,
- const std::string& settings) override;
+ const std::string& settings,
+ const DoneCallback& callback) override;
std::string LoadSettings() override;
@@ -29,6 +33,7 @@
std::string GetPath(const std::string& name) const;
const bool disable_security_;
const std::string model_id_;
+ provider::TaskRunner* task_runner_{nullptr};
};
} // namespace examples
diff --git a/include/weave/provider/config_store.h b/include/weave/provider/config_store.h
index 991d750..128eccc 100644
--- a/include/weave/provider/config_store.h
+++ b/include/weave/provider/config_store.h
@@ -13,6 +13,7 @@
#include <base/callback.h>
#include <base/time/time.h>
#include <weave/enum_to_string.h>
+#include <weave/error.h>
#include <weave/settings.h>
namespace weave {
@@ -48,9 +49,13 @@
// persistent storage (file, flash, etc).
// For example:
// void FileConfigStore::SaveSettings(const std::string& name,
-// const std::string& settings) {
+// const std::string& settings,
+// const DoneCallback& callback) {
// std::ofstream str("/var/lib/weave/weave_" + name + ".json");
// str << settings;
+// if (!callback.is_null())
+// task_runner_->PostDelayedTask(FROM_HERE, base::Bind(callback, nullptr),
+// {});
// }
// It is highly recommended to protected data using encryption with
// hardware backed key.
@@ -75,8 +80,10 @@
// modifications. Data stored in settings can be sensitive, so it's highly
// recommended to protect data, e.g. using encryption.
// |name| is the name of settings blob. Could be used as filename.
+ // Implementation must call or post callback
virtual void SaveSettings(const std::string& name,
- const std::string& settings) = 0;
+ const std::string& settings,
+ const DoneCallback& callback) = 0;
// Deprecated: only for migration of old configs to version with |name|.
virtual std::string LoadSettings() = 0;
diff --git a/include/weave/provider/test/mock_config_store.h b/include/weave/provider/test/mock_config_store.h
index cdae693..e6411d6 100644
--- a/include/weave/provider/test/mock_config_store.h
+++ b/include/weave/provider/test/mock_config_store.h
@@ -40,11 +40,19 @@
"device_id": "TEST_DEVICE_ID"
})"));
EXPECT_CALL(*this, LoadSettings("config")).WillRepeatedly(Return(""));
- EXPECT_CALL(*this, SaveSettings("config", _)).WillRepeatedly(Return());
+ EXPECT_CALL(*this, SaveSettings("config", _, _))
+ .WillRepeatedly(testing::WithArgs<1, 2>(testing::Invoke(
+ [](const std::string& json, const DoneCallback& callback) {
+ if (!callback.is_null())
+ callback.Run(nullptr);
+ })));
}
MOCK_METHOD1(LoadDefaults, bool(Settings*));
MOCK_METHOD1(LoadSettings, std::string(const std::string&));
- MOCK_METHOD2(SaveSettings, void(const std::string&, const std::string&));
+ MOCK_METHOD3(SaveSettings,
+ void(const std::string&,
+ const std::string&,
+ const DoneCallback&));
MOCK_METHOD0(LoadSettings, std::string());
};
diff --git a/src/config.cc b/src/config.cc
index cf564c8..44d20dd 100644
--- a/src/config.cc
+++ b/src/config.cc
@@ -18,6 +18,7 @@
#include "src/data_encoding.h"
#include "src/privet/privet_types.h"
#include "src/string_utils.h"
+#include "src/bind_lambda.h"
namespace weave {
@@ -271,7 +272,9 @@
base::JSONWriter::WriteWithOptions(
dict, base::JSONWriter::OPTIONS_PRETTY_PRINT, &json_string);
- config_store_->SaveSettings(kConfigName, json_string);
+ config_store_->SaveSettings(
+ kConfigName, json_string,
+ base::Bind([](ErrorPtr error) { CHECK(!error); }));
}
Config::Transaction::~Transaction() {
diff --git a/src/config_unittest.cc b/src/config_unittest.cc
index 4517428..fbb558a 100644
--- a/src/config_unittest.cc
+++ b/src/config_unittest.cc
@@ -258,9 +258,10 @@
EXPECT_CALL(*this, OnConfigChanged(_)).Times(1);
- EXPECT_CALL(config_store_, SaveSettings(kConfigName, _))
- .WillOnce(WithArgs<1>(Invoke([](const std::string& json) {
- auto expected = R"({
+ EXPECT_CALL(config_store_, SaveSettings(kConfigName, _, _))
+ .WillOnce(WithArgs<1, 2>(
+ Invoke([](const std::string& json, const DoneCallback& callback) {
+ auto expected = R"({
'version': 1,
'api_key': 'set_api_key',
'client_id': 'set_client_id',
@@ -281,8 +282,9 @@
'secret': 'AQIDBAU=',
'service_url': 'set_service_url'
})";
- EXPECT_JSON_EQ(expected, *test::CreateValue(json));
- })));
+ EXPECT_JSON_EQ(expected, *test::CreateValue(json));
+ callback.Run(nullptr);
+ })));
change.Commit();
}
diff --git a/src/weave_unittest.cc b/src/weave_unittest.cc
index c22eaa1..ebc66cd 100644
--- a/src/weave_unittest.cc
+++ b/src/weave_unittest.cc
@@ -204,11 +204,6 @@
})));
}
- void InitConfigStore() {
- EXPECT_CALL(config_store_, SaveSettings("config", _))
- .WillRepeatedly(Return());
- }
-
void InitNetwork() {
EXPECT_CALL(network_, AddConnectionChangedCallback(_))
.WillRepeatedly(Invoke(
@@ -268,7 +263,6 @@
}
void InitDefaultExpectations() {
- InitConfigStore();
InitNetwork();
EXPECT_CALL(wifi_, StartAccessPoint(MatchesRegex("TEST_NAME.*prv")))
.WillOnce(Return());
@@ -361,13 +355,11 @@
}
TEST_F(WeaveTest, StartMinimal) {
- InitConfigStore();
device_ = weave::Device::Create(&config_store_, &task_runner_, &http_client_,
&network_, nullptr, nullptr, &wifi_, nullptr);
}
TEST_F(WeaveTest, StartNoWifi) {
- InitConfigStore();
InitNetwork();
InitHttpServer();
InitDnsSd();
@@ -451,7 +443,6 @@
void SetUp() override {
WeaveTest::SetUp();
- InitConfigStore();
InitHttpServer();
InitNetwork();
InitDnsSd();
