buffet: Add callback to notify about state changes Callback could be used to expose state as D-Bus property or for smarter server updates. SetProperties replaced SetPropertyValue to avoid multiple callbacks for complex state updates. BUG=brillo:1124,brillo:810,brillo:427 TEST=`FEATURES=test emerge-gizmo buffet` Change-Id: I899b14fd41b0a45cabe149431a3bebeba9c50320 Reviewed-on: https://chromium-review.googlesource.com/273341 Tested-by: Vitaly Buka <vitalybuka@chromium.org> Trybot-Ready: Vitaly Buka <vitalybuka@chromium.org> Reviewed-by: Alex Vakulenko <avakulenko@chromium.org> Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/manager.cc b/buffet/manager.cc index 78a0a8c..96319c0 100644 --- a/buffet/manager.cc +++ b/buffet/manager.cc
@@ -137,17 +137,7 @@ void Manager::UpdateState(DBusMethodResponse<> response, const chromeos::VariantDictionary& property_set) { chromeos::ErrorPtr error; - base::Time timestamp = base::Time::Now(); - bool all_success = true; - for (const auto& pair : property_set) { - if (!state_manager_->SetPropertyValue(pair.first, pair.second, timestamp, - &error)) { - // Remember that an error occurred but keep going and update the rest of - // the properties if possible. - all_success = false; - } - } - if (!all_success) + if (!state_manager_->SetProperties(property_set, &error)) response->ReplyWithError(error.get()); else response->Return();
diff --git a/buffet/states/state_manager.cc b/buffet/states/state_manager.cc index 827cd7f..57330f4 100644 --- a/buffet/states/state_manager.cc +++ b/buffet/states/state_manager.cc
@@ -30,6 +30,11 @@ CHECK(state_change_queue_) << "State change queue not specified"; } +void StateManager::AddOnChangedCallback(const base::Closure& callback) { + on_changed_.push_back(callback); + callback.Run(); // Force to read current state. +} + void StateManager::Startup() { LOG(INFO) << "Initializing StateManager."; @@ -88,6 +93,9 @@ firmware_version, base::Time::Now(), nullptr)); + + for (const auto& cb : on_changed_) + cb.Run(); } std::unique_ptr<base::DictionaryValue> StateManager::GetStateValuesAsJson( @@ -104,6 +112,23 @@ return dict; } +bool StateManager::SetProperties( + const chromeos::VariantDictionary& property_set, + chromeos::ErrorPtr* error) { + base::Time timestamp = base::Time::Now(); + bool all_success = true; + for (const auto& pair : property_set) { + if (!SetPropertyValue(pair.first, pair.second, timestamp, error)) { + // Remember that an error occurred but keep going and update the rest of + // the properties if possible. + all_success = false; + } + } + for (const auto& cb : on_changed_) + cb.Run(); + return all_success; +} + bool StateManager::SetPropertyValue(const std::string& full_property_name, const chromeos::Any& value, const base::Time& timestamp,
diff --git a/buffet/states/state_manager.h b/buffet/states/state_manager.h index 53695e2..cf2c60a 100644 --- a/buffet/states/state_manager.h +++ b/buffet/states/state_manager.h
@@ -11,6 +11,7 @@ #include <string> #include <vector> +#include <base/callback.h> #include <base/macros.h> #include <chromeos/errors/error.h> #include <chromeos/variant_dictionary.h> @@ -33,6 +34,8 @@ public: explicit StateManager(StateChangeQueueInterface* state_change_queue); + void AddOnChangedCallback(const base::Closure& callback); + // Initializes the state manager and load device state fragments. // Called by Buffet daemon at startup. void Startup(); @@ -42,12 +45,9 @@ std::unique_ptr<base::DictionaryValue> GetStateValuesAsJson( chromeos::ErrorPtr* error) const; - // Updates a single property value. |full_property_name| must be the full - // name of the property to update in format "package.property". - bool SetPropertyValue(const std::string& full_property_name, - const chromeos::Any& value, - const base::Time& timestamp, - chromeos::ErrorPtr* error); + // Updates a multiple property values. + bool SetProperties(const chromeos::VariantDictionary& property_set, + chromeos::ErrorPtr* error); // Returns all the categories the state properties are registered from. // As with GCD command handling, the category normally represent a device @@ -61,6 +61,15 @@ std::vector<StateChange> GetAndClearRecordedStateChanges(); private: + friend class StateManagerTest; + + // Updates a single property value. |full_property_name| must be the full + // name of the property to update in format "package.property". + bool SetPropertyValue(const std::string& full_property_name, + const chromeos::Any& value, + const base::Time& timestamp, + chromeos::ErrorPtr* error); + // Loads a device state fragment from a JSON object. |category| represents // a device daemon providing the state fragment or empty string for the // base state fragment. @@ -96,7 +105,8 @@ std::map<std::string, std::unique_ptr<StatePackage>> packages_; std::set<std::string> categories_; - friend class StateManagerTest; + std::vector<base::Closure> on_changed_; + DISALLOW_COPY_AND_ASSIGN(StateManager); };
diff --git a/buffet/states/state_manager_unittest.cc b/buffet/states/state_manager_unittest.cc index d71ebb6..50e6a5f 100644 --- a/buffet/states/state_manager_unittest.cc +++ b/buffet/states/state_manager_unittest.cc
@@ -7,6 +7,7 @@ #include <cstdlib> // for abs(). #include <vector> +#include <base/bind.h> #include <base/values.h> #include <gmock/gmock.h> #include <gtest/gtest.h> @@ -56,6 +57,11 @@ EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges()) .Times(0); mgr_.reset(new StateManager(&mock_state_change_queue_)); + + EXPECT_CALL(*this, OnStateChanged()).Times(1); + mgr_->AddOnChangedCallback( + base::Bind(&StateManagerTest::OnStateChanged, base::Unretained(this))); + LoadStateDefinition(GetTestSchema().get(), "default", nullptr); ASSERT_TRUE(mgr_->LoadStateDefaults(*GetTestValues().get(), nullptr)); } @@ -67,6 +73,15 @@ ASSERT_TRUE(mgr_->LoadStateDefinition(*json, category, error)); } + bool SetPropertyValue(const std::string& name, + const chromeos::Any& value, + chromeos::ErrorPtr* error) { + return mgr_->SetPropertyValue(name, value, timestamp_, error); + } + + MOCK_CONST_METHOD0(OnStateChanged, void()); + + base::Time timestamp_{base::Time::Now()}; std::unique_ptr<StateManager> mgr_; MockStateChangeQueueInterface mock_state_change_queue_; }; @@ -120,12 +135,11 @@ native_types::Object expected_prop_set{ {"terminator.target", unittests::make_string_prop_value("John Connor")}, }; - base::Time timestamp = base::Time::Now(); EXPECT_CALL(mock_state_change_queue_, - NotifyPropertiesUpdated(timestamp, expected_prop_set)) + NotifyPropertiesUpdated(timestamp_, expected_prop_set)) .WillOnce(Return(true)); - ASSERT_TRUE(mgr_->SetPropertyValue( - "terminator.target", std::string{"John Connor"}, timestamp, nullptr)); + ASSERT_TRUE(SetPropertyValue("terminator.target", std::string{"John Connor"}, + nullptr)); auto expected = R"({ 'base': { 'manufacturer': 'Skynet', @@ -140,7 +154,7 @@ TEST_F(StateManagerTest, SetPropertyValue_Error_NoName) { chromeos::ErrorPtr error; - ASSERT_FALSE(mgr_->SetPropertyValue("", int{0}, base::Time::Now(), &error)); + ASSERT_FALSE(SetPropertyValue("", int{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNameMissing, error->GetCode()); EXPECT_EQ("Property name is missing", error->GetMessage()); @@ -148,8 +162,7 @@ TEST_F(StateManagerTest, SetPropertyValue_Error_NoPackage) { chromeos::ErrorPtr error; - ASSERT_FALSE( - mgr_->SetPropertyValue("target", int{0}, base::Time::Now(), &error)); + ASSERT_FALSE(SetPropertyValue("target", int{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPackageNameMissing, error->GetCode()); EXPECT_EQ("Package name is missing in the property name", @@ -158,8 +171,7 @@ TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownPackage) { chromeos::ErrorPtr error; - ASSERT_FALSE( - mgr_->SetPropertyValue("power.level", int{0}, base::Time::Now(), &error)); + ASSERT_FALSE(SetPropertyValue("power.level", int{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); EXPECT_EQ("Unknown state property package 'power'", error->GetMessage()); @@ -167,22 +179,20 @@ TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownProperty) { chromeos::ErrorPtr error; - ASSERT_FALSE( - mgr_->SetPropertyValue("base.level", int{0}, base::Time::Now(), &error)); + ASSERT_FALSE(SetPropertyValue("base.level", int{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); EXPECT_EQ("State property 'base.level' is not defined", error->GetMessage()); } TEST_F(StateManagerTest, GetAndClearRecordedStateChanges) { - base::Time timestamp = base::Time::Now(); - EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(timestamp, _)) + EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(timestamp_, _)) .WillOnce(Return(true)); - ASSERT_TRUE(mgr_->SetPropertyValue( - "terminator.target", std::string{"John Connor"}, timestamp, nullptr)); + ASSERT_TRUE(SetPropertyValue("terminator.target", std::string{"John Connor"}, + nullptr)); std::vector<StateChange> expected_val; expected_val.emplace_back( - timestamp, + timestamp_, native_types::Object{{"terminator.target", unittests::make_string_prop_value("John Connor")}}); EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges()) @@ -194,4 +204,28 @@ changes.back().changed_properties); } +TEST_F(StateManagerTest, SetProperties) { + native_types::Object expected_prop_set{ + {"base.manufacturer", unittests::make_string_prop_value("No Name")}, + }; + EXPECT_CALL(mock_state_change_queue_, + NotifyPropertiesUpdated(_, expected_prop_set)) + .WillOnce(Return(true)); + + EXPECT_CALL(*this, OnStateChanged()).Times(1); + ASSERT_TRUE(mgr_->SetProperties( + {{"base.manufacturer", std::string("No Name")}}, nullptr)); + + auto expected = R"({ + 'base': { + 'manufacturer': 'No Name', + 'serialNumber': 'T1000' + }, + 'terminator': { + 'target': '' + } + })"; + EXPECT_JSON_EQ(expected, *mgr_->GetStateValuesAsJson(nullptr)); +} + } // namespace buffet