Remove Deferred Updates
Remove deferred consumption of settings Manual/NTP and allow instant
consumption.
Tested:
Manually set the timeMode to NTP on the WEB and successfully.
busctl get-property xyz.openbmc_project.Settings
/xyz/openbmc_project/time/sync_method
xyz.openbmc_project.Time.Synchronization TimeSyncMethod
s "xyz.openbmc_project.Time.Synchronization.Method.NTP"
Manually set the date time successfully by D-Bus when timeMode is MANUAL
busctl set-property xyz.openbmc_project.Time.Manager
/xyz/openbmc_project/time/bmc xyz.openbmc_project.Time.EpochTime Elapsed
t 1514765953791262
Manually set the date time failed by D-Bus when timeMode is NTP.
busctl set-property xyz.openbmc_project.Time.Manager
/xyz/openbmc_project/time/bmc xyz.openbmc_project.Time.EpochTime Elapsed
t 1514765953791262
Failed to set property Elapsed on interface
xyz.openbmc_project.TIme.EpochTime: The operation failed
~# journalctl -b | grep timemanager
Jan 01 00:15:26 fp5280g2 phosphor-timemanager[309]: Error in setting
system time
Jan 01 00:15:26 fp5280g2 phosphor-timemanager[309]: The operation
failed
Refer: https://lists.ozlabs.org/pipermail/openbmc/2020-April/021409.html
Signed-off-by: George Liu <liuxiwei@inspur.com>
Change-Id: I7be25a9d0f56615bad6800a0b07df7f84fc0acc3
diff --git a/README.md b/README.md
index 5b1d5cd..1dc3e40 100644
--- a/README.md
+++ b/README.md
@@ -89,15 +89,4 @@
This results in [openbmc/openbmc#3459][1], and the related test cases are
updated to cooperate with this behavior change.
-### Special note on host on
-When the host is on, the changes of the above time mode are not applied but
-deferred. The changes of the mode are saved to persistent storage.
-
-When the host is off, the saved mode are read from persistent storage and are
-applied.
-
-Note: A user can set the time mode in the settings daemon at any time,
-but the time manager applying them is governed by the above condition.
-
-
[1]: https://github.com/openbmc/openbmc/issues/3459
diff --git a/main.cpp b/main.cpp
index 1b94439..2f40341 100644
--- a/main.cpp
+++ b/main.cpp
@@ -24,10 +24,8 @@
// Add sdbusplus ObjectManager
sdbusplus::server::manager::manager bmcEpochObjManager(bus, OBJPATH_BMC);
- phosphor::time::Manager manager(bus);
phosphor::time::BmcEpoch bmc(bus, OBJPATH_BMC);
-
- manager.addListener(&bmc);
+ phosphor::time::Manager manager(bus);
bus.request_name(BUSNAME);
diff --git a/manager.cpp b/manager.cpp
index 92a7a3c..4cc0b57 100644
--- a/manager.cpp
+++ b/manager.cpp
@@ -2,17 +2,17 @@
#include "utils.hpp"
+#include <assert.h>
+
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/elog.hpp>
#include <phosphor-logging/log.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
-#include <xyz/openbmc_project/State/Host/server.hpp>
namespace rules = sdbusplus::bus::match::rules;
namespace // anonymous
{
-constexpr auto HOST_CURRENT_STATE = "CurrentHostState";
constexpr auto SYSTEMD_TIME_SERVICE = "org.freedesktop.timedate1";
constexpr auto SYSTEMD_TIME_PATH = "/org/freedesktop/timedate1";
@@ -27,26 +27,14 @@
using namespace phosphor::logging;
-const std::set<std::string> Manager::managedProperties = {PROPERTY_TIME_MODE};
-
Manager::Manager(sdbusplus::bus::bus& bus) : bus(bus), settings(bus)
{
using namespace sdbusplus::bus::match::rules;
- hostStateChangeMatch =
- std::make_unique<decltype(hostStateChangeMatch)::element_type>(
- bus, propertiesChanged(settings.hostState, settings::hostStateIntf),
- std::bind(std::mem_fn(&Manager::onHostStateChanged), this,
- std::placeholders::_1));
settingsMatches.emplace_back(
bus, propertiesChanged(settings.timeSyncMethod, settings::timeSyncIntf),
std::bind(std::mem_fn(&Manager::onSettingsChanged), this,
std::placeholders::_1));
- checkHostOn();
-
- // Restore settings from persistent storage
- restoreSettings();
-
// Check the settings daemon to process the new settings
auto mode = getSetting(settings.timeSyncMethod.c_str(),
settings::timeSyncIntf, PROPERTY_TIME_MODE);
@@ -54,53 +42,14 @@
onPropertyChanged(PROPERTY_TIME_MODE, mode);
}
-void Manager::addListener(PropertyChangeListner* listener)
-{
- // Notify listener about the initial value
- listener->onModeChanged(timeMode);
-
- listeners.insert(listener);
-}
-
-void Manager::restoreSettings()
-{
- auto mode = utils::readData<std::string>(modeFile);
- if (!mode.empty())
- {
- timeMode = utils::strToMode(mode);
- }
-}
-
-void Manager::checkHostOn()
-{
- using Host = sdbusplus::xyz::openbmc_project::State::server::Host;
- auto hostService = utils::getService(bus, settings.hostState.c_str(),
- settings::hostStateIntf);
- auto stateStr = utils::getProperty<std::string>(
- bus, hostService.c_str(), settings.hostState.c_str(),
- settings::hostStateIntf, HOST_CURRENT_STATE);
- auto state = Host::convertHostStateFromString(stateStr);
- hostOn = (state == Host::HostState::Running);
-}
-
void Manager::onPropertyChanged(const std::string& key,
const std::string& value)
{
- if (hostOn)
- {
- // If host is on, set the values as requested time mode.
- // And when host becomes off, notify the listeners.
- setPropertyAsRequested(key, value);
- }
- else
- {
- // If host is off, notify listeners
- if (key == PROPERTY_TIME_MODE)
- {
- setCurrentTimeMode(value);
- onTimeModeChanged(value);
- }
- }
+ assert(key == PROPERTY_TIME_MODE);
+
+ // Notify listeners
+ setCurrentTimeMode(value);
+ onTimeModeChanged(value);
}
int Manager::onSettingsChanged(sdbusplus::message::message& msg)
@@ -123,30 +72,6 @@
return 0;
}
-void Manager::setPropertyAsRequested(const std::string& key,
- const std::string& value)
-{
- if (key == PROPERTY_TIME_MODE)
- {
- setRequestedMode(value);
- }
- else
- {
- // The key shall be already the supported one
- using InvalidArgumentError =
- sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument;
- using namespace xyz::openbmc_project::Common;
- elog<InvalidArgumentError>(
- InvalidArgument::ARGUMENT_NAME(key.c_str()),
- InvalidArgument::ARGUMENT_VALUE(value.c_str()));
- }
-}
-
-void Manager::setRequestedMode(const std::string& mode)
-{
- requestedMode = mode;
-}
-
void Manager::updateNtpSetting(const std::string& value)
{
bool isNtp =
@@ -168,50 +93,6 @@
}
}
-void Manager::onHostStateChanged(sdbusplus::message::message& msg)
-{
- using Interface = std::string;
- using Property = std::string;
- using Value = std::string;
- using Properties = std::map<Property, std::variant<Value>>;
- using Host = sdbusplus::xyz::openbmc_project::State::server::Host;
-
- Interface interface;
- Properties properties;
-
- msg.read(interface, properties);
-
- for (const auto& p : properties)
- {
- if (p.first == HOST_CURRENT_STATE)
- {
- auto state = Host::convertHostStateFromString(
- std::get<std::string>(p.second));
- onHostState(state == Host::HostState::Running);
- break;
- }
- }
-}
-
-void Manager::onHostState(bool on)
-{
- hostOn = on;
- if (hostOn)
- {
- log<level::INFO>("Changing time settings is *deferred* now");
- return;
- }
- log<level::INFO>("Changing time settings allowed now");
- if (!requestedMode.empty())
- {
- if (setCurrentTimeMode(requestedMode))
- {
- onTimeModeChanged(requestedMode);
- }
- setRequestedMode({}); // Clear requested mode
- }
-}
-
bool Manager::setCurrentTimeMode(const std::string& mode)
{
auto newMode = utils::strToMode(mode);
@@ -220,7 +101,6 @@
log<level::INFO>("Time mode is changed",
entry("MODE=%s", mode.c_str()));
timeMode = newMode;
- utils::writeData(modeFile, mode);
return true;
}
else
@@ -231,10 +111,6 @@
void Manager::onTimeModeChanged(const std::string& mode)
{
- for (const auto listener : listeners)
- {
- listener->onModeChanged(timeMode);
- }
// When time_mode is updated, update the NTP setting
updateNtpSetting(mode);
}
diff --git a/manager.hpp b/manager.hpp
index 2d5d131..c93c58f 100644
--- a/manager.hpp
+++ b/manager.hpp
@@ -8,7 +8,6 @@
#include <sdbusplus/bus.hpp>
#include <sdbusplus/bus/match.hpp>
-#include <set>
#include <string>
namespace phosphor
@@ -34,11 +33,6 @@
Manager& operator=(Manager&&) = delete;
~Manager() = default;
- /** @brief Add a listener that will be called
- * when property is changed
- **/
- void addListener(PropertyChangeListner* listener);
-
private:
/** @brief Persistent sdbusplus DBus connection */
sdbusplus::bus::bus& bus;
@@ -46,30 +40,12 @@
/** @brief The match of settings property change */
std::vector<sdbusplus::bus::match::match> settingsMatches;
- /** @brief The match of host state change */
- std::unique_ptr<sdbusplus::bus::match::match> hostStateChangeMatch;
-
- /** @brief The container to hold all the listeners */
- std::set<PropertyChangeListner*> listeners;
-
/** @brief Settings objects of intereset */
settings::Objects settings;
- /** @brief The value to indicate if host is on */
- bool hostOn = false;
-
- /** @brief The requested time mode when host is on*/
- std::string requestedMode;
-
/** @brief The current time mode */
Mode timeMode = DEFAULT_TIME_MODE;
- /** @brief Restore saved settings */
- void restoreSettings();
-
- /** @brief Check if host is on and update hostOn variable */
- void checkHostOn();
-
/** @brief Get setting from settingsd service
*
* @param[in] path - The dbus object path
@@ -113,33 +89,6 @@
*/
void onPropertyChanged(const std::string& key, const std::string& value);
- /** @brief Notified on host state has changed
- *
- * @param[in] msg - sdbusplus dbusmessage
- */
- void onHostStateChanged(sdbusplus::message::message& msg);
-
- /** @brief Notified on host state has changed
- *
- * @param[in] on - Indicate if the host is on or off
- */
- void onHostState(bool on);
-
- /** @brief Set the property as requested time mode
- *
- * @param[in] key - The property name
- * @param[in] value - The property value
- */
- void setPropertyAsRequested(const std::string& key,
- const std::string& value);
-
- /** @brief Set the current mode to user requested one
- * if conditions allow it
- *
- * @param[in] mode - The string of time mode
- */
- void setRequestedMode(const std::string& mode);
-
/** @brief Update the NTP setting to systemd time service
*
* @param[in] value - The time mode value, e.g. "NTP" or "MANUAL"
@@ -157,23 +106,6 @@
/** @brief The string of time mode property */
static constexpr auto PROPERTY_TIME_MODE = "TimeSyncMethod";
-
- using Updater = std::function<void(const std::string&)>;
-
- /** @brief Map the property string to functions that shall
- * be called when the property is changed
- */
- const std::map<std::string, Updater> propertyUpdaters = {
- {PROPERTY_TIME_MODE,
- std::bind(&Manager::setCurrentTimeMode, this, std::placeholders::_1)}};
-
- /** @brief The properties that manager shall notify the
- * listeners when changed
- */
- static const std::set<std::string> managedProperties;
-
- /** @brief The file name of saved time mode */
- static constexpr auto modeFile = "/var/lib/obmc/saved_time_mode";
};
} // namespace time
diff --git a/settings.cpp b/settings.cpp
index 61a575b..65df0ea 100644
--- a/settings.cpp
+++ b/settings.cpp
@@ -17,7 +17,7 @@
Objects::Objects(sdbusplus::bus::bus& bus) : bus(bus)
{
- std::vector<std::string> settingsIntfs = {timeSyncIntf, hostStateIntf};
+ std::vector<std::string> settingsIntfs = {timeSyncIntf};
auto depth = 0;
auto mapperCall = bus.new_method_call(mapperService, mapperPath, mapperIntf,
@@ -54,10 +54,6 @@
{
timeSyncMethod = path;
}
- else if (hostStateIntf == interface)
- {
- hostState = path;
- }
}
}
}
diff --git a/settings.hpp b/settings.hpp
index 12209c0..b547339 100644
--- a/settings.hpp
+++ b/settings.hpp
@@ -12,7 +12,6 @@
constexpr auto root = "/";
constexpr auto timeSyncIntf = "xyz.openbmc_project.Time.Synchronization";
-constexpr auto hostStateIntf = "xyz.openbmc_project.State.Host";
/** @class Objects
* @brief Fetch paths of settings D-bus objects of interest upon construction
@@ -46,9 +45,6 @@
/** @brief time sync method settings object */
Path timeSyncMethod;
- /** @brief host state object */
- Path hostState;
-
private:
sdbusplus::bus::bus& bus;
};
diff --git a/test/Makefile.am b/test/Makefile.am
index e0457cb..cacedcd 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -6,7 +6,6 @@
check_PROGRAMS += test
test_SOURCES = \
- TestEpochBase.cpp \
TestBmcEpoch.cpp \
TestManager.cpp \
TestUtils.cpp
diff --git a/test/TestBmcEpoch.cpp b/test/TestBmcEpoch.cpp
index db43854..5ed1f3c 100644
--- a/test/TestBmcEpoch.cpp
+++ b/test/TestBmcEpoch.cpp
@@ -51,6 +51,15 @@
}
};
+TEST_F(TestBmcEpoch, onModeChange)
+{
+ bmcEpoch->onModeChanged(Mode::NTP);
+ EXPECT_EQ(Mode::NTP, getTimeMode());
+
+ bmcEpoch->onModeChanged(Mode::Manual);
+ EXPECT_EQ(Mode::Manual, getTimeMode());
+}
+
TEST_F(TestBmcEpoch, empty)
{
// Default mode is MANUAL
diff --git a/test/TestEpochBase.cpp b/test/TestEpochBase.cpp
deleted file mode 100644
index d3d1b69..0000000
--- a/test/TestEpochBase.cpp
+++ /dev/null
@@ -1,40 +0,0 @@
-#include "epoch_base.hpp"
-#include "types.hpp"
-
-#include <sdbusplus/bus.hpp>
-
-#include <gtest/gtest.h>
-
-namespace phosphor
-{
-namespace time
-{
-
-class TestEpochBase : public testing::Test
-{
- public:
- sdbusplus::bus::bus bus;
- EpochBase epochBase;
-
- TestEpochBase() : bus(sdbusplus::bus::new_default()), epochBase(bus, "")
- {
- // Empty
- }
-
- Mode getMode()
- {
- return epochBase.timeMode;
- }
-};
-
-TEST_F(TestEpochBase, onModeChange)
-{
- epochBase.onModeChanged(Mode::NTP);
- EXPECT_EQ(Mode::NTP, getMode());
-
- epochBase.onModeChanged(Mode::Manual);
- EXPECT_EQ(Mode::Manual, getMode());
-}
-
-} // namespace time
-} // namespace phosphor
diff --git a/test/TestManager.cpp b/test/TestManager.cpp
index 95f5734..b1cf4ec 100644
--- a/test/TestManager.cpp
+++ b/test/TestManager.cpp
@@ -18,15 +18,9 @@
public:
sdbusplus::bus::bus bus;
Manager manager;
- MockPropertyChangeListner listener1;
- MockPropertyChangeListner listener2;
TestManager() : bus(sdbusplus::bus::new_default()), manager(bus)
{
- // Add two mocked listeners so that we can test
- // the behavior related to listeners
- manager.addListener(&listener1);
- manager.addListener(&listener2);
}
// Proxies for Manager's private members and functions
@@ -34,129 +28,26 @@
{
return manager.timeMode;
}
- bool hostOn()
- {
- return manager.hostOn;
- }
- std::string getRequestedMode()
- {
- return manager.requestedMode;
- }
void notifyPropertyChanged(const std::string& key, const std::string& value)
{
manager.onPropertyChanged(key, value);
}
- void notifyOnHostState(bool hostOn)
- {
- manager.onHostState(hostOn);
- }
};
-TEST_F(TestManager, DISABLED_empty)
-{
- EXPECT_FALSE(hostOn());
- EXPECT_EQ("", getRequestedMode());
-
- // Default mode is MANUAL
- EXPECT_EQ(Mode::Manual, getTimeMode());
-}
-
-TEST_F(TestManager, DISABLED_hostStateChange)
-{
- notifyOnHostState(true);
- EXPECT_TRUE(hostOn());
- notifyOnHostState(false);
- EXPECT_FALSE(hostOn());
-}
-
TEST_F(TestManager, DISABLED_propertyChanged)
{
- // When host is off, property change will be notified to listeners
- EXPECT_FALSE(hostOn());
-
- // Check mocked listeners shall receive notifications on property changed
- EXPECT_CALL(listener1, onModeChanged(Mode::Manual)).Times(1);
- EXPECT_CALL(listener2, onModeChanged(Mode::Manual)).Times(1);
-
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.Manual");
-
- EXPECT_EQ("", getRequestedMode());
-
- // When host is on, property changes are saved as requested ones
- notifyOnHostState(true);
-
- // Check mocked listeners shall not receive notifications
- EXPECT_CALL(listener1, onModeChanged(Mode::Manual)).Times(0);
- EXPECT_CALL(listener2, onModeChanged(Mode::Manual)).Times(0);
+ EXPECT_EQ(Mode::Manual, getTimeMode());
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.NTP");
+ EXPECT_EQ(Mode::NTP, getTimeMode());
- EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.NTP",
- getRequestedMode());
-
- // When host becomes off, the requested mode shall be notified
- // to listeners, and be cleared
- EXPECT_CALL(listener1, onModeChanged(Mode::NTP)).Times(1);
- EXPECT_CALL(listener2, onModeChanged(Mode::NTP)).Times(1);
-
- notifyOnHostState(false);
-
- EXPECT_EQ("", getRequestedMode());
-
- // When host is on, and invalid property is changed,
- // verify the code asserts because it shall never occur
- notifyOnHostState(true);
ASSERT_DEATH(notifyPropertyChanged("invalid property", "whatever"), "");
}
-TEST_F(TestManager, DISABLED_propertyChangedAndChangedbackWhenHostOn)
-{
- // Property is now MANUAL/HOST
- notifyPropertyChanged(
- "TimeSyncMethod",
- "xyz.openbmc_project.Time.Synchronization.Method.Manual");
-
- // Set host on
- notifyOnHostState(true);
-
- // Check mocked listeners shall not receive notifications
- EXPECT_CALL(listener1, onModeChanged(_)).Times(0);
- EXPECT_CALL(listener2, onModeChanged(_)).Times(0);
-
- notifyPropertyChanged(
- "TimeSyncMethod",
- "xyz.openbmc_project.Time.Synchronization.Method.NTP");
-
- // Saved as requested mode
- EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.NTP",
- getRequestedMode());
-
- // Property changed back to MANUAL/HOST
- notifyPropertyChanged(
- "TimeSyncMethod",
- "xyz.openbmc_project.Time.Synchronization.Method.Manual");
-
- // Requested mode shall be updated
- EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.Manual",
- getRequestedMode());
-
- // Because the latest mode is the same as when host is off,
- // The listeners shall not be notified, and requested mode
- // shall be cleared
- EXPECT_CALL(listener1, onModeChanged(_)).Times(0);
- EXPECT_CALL(listener2, onModeChanged(_)).Times(0);
-
- notifyOnHostState(false);
-
- EXPECT_EQ("", getRequestedMode());
-}
-
-// TODO: if gmock is ready, add case to test
-// updateNtpSetting() and updateNetworkSetting()
-
} // namespace time
} // namespace phosphor
diff --git a/utils.hpp b/utils.hpp
index 0ec55f3..150e711 100644
--- a/utils.hpp
+++ b/utils.hpp
@@ -2,7 +2,6 @@
#include "types.hpp"
-#include <fstream>
#include <phosphor-logging/log.hpp>
#include <sdbusplus/bus.hpp>
@@ -15,39 +14,6 @@
using namespace phosphor::logging;
-/** @brief Read data with type T from file
- *
- * @param[in] fileName - The name of file to read from
- *
- * @return The data with type T
- */
-template <typename T>
-T readData(const char* fileName)
-{
- T data{};
- std::ifstream fs(fileName);
- if (fs.is_open())
- {
- fs >> data;
- }
- return data;
-}
-
-/** @brief Write data with type T to file
- *
- * @param[in] fileName - The name of file to write to
- * @param[in] data - The data with type T to write to file
- */
-template <typename T>
-void writeData(const char* fileName, T&& data)
-{
- std::ofstream fs(fileName, std::ios::out);
- if (fs.is_open())
- {
- fs << std::forward<T>(data);
- }
-}
-
/** @brief The template function to get property from the requested dbus path
*
* @param[in] bus - The Dbus bus object