Remove TimeOwner Feature
The TimeOwner feature is confusing from feedback from the community and
hence removing the feature.
Remove the TimeOwner feature in the phosphor-time-manager repo and
needed settings objects.
Tested: Manually set the date time on the web and successfully update
the date time of BMC (eg: 2020/01/01 08:07:50).
busctrl get-property xyz.openbmc_project.Time.Manager
/xyz/openbmc_project/time/bmc
xyz.openbmc_project.Time.EpochTime Elapsed
t 1577837156385836
Refer: https://lists.ozlabs.org/pipermail/openbmc/2020-April/021409.html
Signed-off-by: George Liu <liuxiwei@inspur.com>
Change-Id: Id47eb0a03e0e94eeff29d2b77dccefb89cded7b8
diff --git a/test/Makefile.am b/test/Makefile.am
index a8cd3b7..e0457cb 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -8,7 +8,6 @@
test_SOURCES = \
TestEpochBase.cpp \
TestBmcEpoch.cpp \
- TestHostEpoch.cpp \
TestManager.cpp \
TestUtils.cpp
diff --git a/test/TestBmcEpoch.cpp b/test/TestBmcEpoch.cpp
index eb116e5..db43854 100644
--- a/test/TestBmcEpoch.cpp
+++ b/test/TestBmcEpoch.cpp
@@ -1,12 +1,10 @@
#include "config.h"
#include "bmc_epoch.hpp"
-#include "mocked_bmc_time_change_listener.hpp"
#include "types.hpp"
#include <memory>
#include <sdbusplus/bus.hpp>
-#include <xyz/openbmc_project/Time/error.hpp>
#include <gtest/gtest.h>
@@ -15,16 +13,13 @@
namespace time
{
-using ::testing::_;
using namespace std::chrono;
-using NotAllowed = sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
class TestBmcEpoch : public testing::Test
{
public:
sdbusplus::bus::bus bus;
sd_event* event;
- MockBmcTimeChangeListener listener;
std::unique_ptr<BmcEpoch> bmcEpoch;
TestBmcEpoch() : bus(sdbusplus::bus::new_default())
@@ -33,7 +28,6 @@
sd_event_default(&event);
bus.attach_event(event, SD_EVENT_PRIORITY_NORMAL);
bmcEpoch = std::make_unique<BmcEpoch>(bus, OBJPATH_BMC);
- bmcEpoch->setBmcTimeChangeListener(&listener);
}
~TestBmcEpoch()
@@ -47,14 +41,6 @@
{
return bmcEpoch->timeMode;
}
- Owner getTimeOwner()
- {
- return bmcEpoch->timeOwner;
- }
- void setTimeOwner(Owner owner)
- {
- bmcEpoch->timeOwner = owner;
- }
void setTimeMode(Mode mode)
{
bmcEpoch->timeMode = mode;
@@ -67,9 +53,8 @@
TEST_F(TestBmcEpoch, empty)
{
- // Default mode/owner is MANUAL/BOTH
+ // Default mode is MANUAL
EXPECT_EQ(Mode::Manual, getTimeMode());
- EXPECT_EQ(Owner::Both, getTimeOwner());
}
TEST_F(TestBmcEpoch, getElapsed)
@@ -80,18 +65,6 @@
EXPECT_GE(t2, t1);
}
-TEST_F(TestBmcEpoch, setElapsedNotAllowed)
-{
- auto epochNow =
- duration_cast<microseconds>(system_clock::now().time_since_epoch())
- .count();
-
- // In Host owner, setting time is not allowed
- setTimeMode(Mode::Manual);
- setTimeOwner(Owner::Host);
- EXPECT_THROW(bmcEpoch->elapsed(epochNow), NotAllowed);
-}
-
TEST_F(TestBmcEpoch, setElapsedOK)
{
// TODO: setting time will call sd-bus functions and it will fail on host
@@ -99,12 +72,5 @@
// But for now we can not test it
}
-TEST_F(TestBmcEpoch, onTimeChange)
-{
- // On BMC time change, the listner is expected to be notified
- EXPECT_CALL(listener, onBmcTimeChanged(_)).Times(1);
- triggerTimeChange();
-}
-
} // namespace time
} // namespace phosphor
diff --git a/test/TestEpochBase.cpp b/test/TestEpochBase.cpp
index b193969..d3d1b69 100644
--- a/test/TestEpochBase.cpp
+++ b/test/TestEpochBase.cpp
@@ -25,10 +25,6 @@
{
return epochBase.timeMode;
}
- Owner getOwner()
- {
- return epochBase.timeOwner;
- }
};
TEST_F(TestEpochBase, onModeChange)
@@ -40,20 +36,5 @@
EXPECT_EQ(Mode::Manual, getMode());
}
-TEST_F(TestEpochBase, onOwnerChange)
-{
- epochBase.onOwnerChanged(Owner::BMC);
- EXPECT_EQ(Owner::BMC, getOwner());
-
- epochBase.onOwnerChanged(Owner::Host);
- EXPECT_EQ(Owner::Host, getOwner());
-
- epochBase.onOwnerChanged(Owner::Split);
- EXPECT_EQ(Owner::Split, getOwner());
-
- epochBase.onOwnerChanged(Owner::Both);
- EXPECT_EQ(Owner::Both, getOwner());
-}
-
} // namespace time
} // namespace phosphor
diff --git a/test/TestHostEpoch.cpp b/test/TestHostEpoch.cpp
deleted file mode 100644
index 7ddf92f..0000000
--- a/test/TestHostEpoch.cpp
+++ /dev/null
@@ -1,298 +0,0 @@
-#include "config.h"
-
-#include "host_epoch.hpp"
-#include "types.hpp"
-#include "utils.hpp"
-
-#include <sdbusplus/bus.hpp>
-#include <xyz/openbmc_project/Time/error.hpp>
-
-#include <gtest/gtest.h>
-
-namespace phosphor
-{
-namespace time
-{
-
-using namespace std::chrono;
-using namespace std::chrono_literals;
-using NotAllowed = sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
-
-const constexpr microseconds USEC_ZERO{0};
-
-class TestHostEpoch : public testing::Test
-{
- public:
- sdbusplus::bus::bus bus;
- HostEpoch hostEpoch;
-
- static constexpr auto FILE_NOT_EXIST = "path/to/file-not-exist";
- static constexpr auto FILE_OFFSET = "saved_host_offset";
- const microseconds delta = 2s;
-
- TestHostEpoch() :
- bus(sdbusplus::bus::new_default()), hostEpoch(bus, OBJPATH_HOST)
- {
- // Make sure the file does not exist
- std::remove(FILE_NOT_EXIST);
- }
- ~TestHostEpoch()
- {
- // Cleanup test file
- std::remove(FILE_OFFSET);
- }
-
- // Proxies for HostEpoch's private members and functions
- Mode getTimeMode()
- {
- return hostEpoch.timeMode;
- }
- Owner getTimeOwner()
- {
- return hostEpoch.timeOwner;
- }
- microseconds getOffset()
- {
- return hostEpoch.offset;
- }
- void setOffset(microseconds us)
- {
- hostEpoch.offset = us;
- }
- void setTimeOwner(Owner owner)
- {
- hostEpoch.onOwnerChanged(owner);
- }
- void setTimeMode(Mode mode)
- {
- hostEpoch.onModeChanged(mode);
- }
-
- void checkSettingTimeNotAllowed()
- {
- // By default offset shall be 0
- EXPECT_EQ(0, getOffset().count());
-
- // Set time is not allowed,
- // so verify offset is still 0 after set time
- microseconds diff = 1min;
- EXPECT_THROW(hostEpoch.elapsed(hostEpoch.elapsed() + diff.count()),
- NotAllowed);
- EXPECT_EQ(0, getOffset().count());
- }
-
- void checkSetSplitTimeInFuture()
- {
- // Get current time, and set future +1min time
- auto t1 = hostEpoch.elapsed();
- EXPECT_NE(0, t1);
- microseconds diff = 1min;
- auto t2 = t1 + diff.count();
- hostEpoch.elapsed(t2);
-
- // Verify that the offset shall be positive,
- // and less or equal to diff, and shall be not too less.
- auto offset = getOffset();
- EXPECT_GT(offset, USEC_ZERO);
- EXPECT_LE(offset, diff);
- diff -= delta;
- EXPECT_GE(offset, diff);
-
- // Now get time shall be around future +1min time
- auto epochNow =
- duration_cast<microseconds>(system_clock::now().time_since_epoch())
- .count();
- auto elapsedGot = hostEpoch.elapsed();
- EXPECT_LT(epochNow, elapsedGot);
- auto epochDiff = elapsedGot - epochNow;
- diff = 1min;
- EXPECT_GT(epochDiff, (diff - delta).count());
- EXPECT_LT(epochDiff, (diff + delta).count());
- }
- void checkSetSplitTimeInPast()
- {
- // Get current time, and set past -1min time
- auto t1 = hostEpoch.elapsed();
- EXPECT_NE(0, t1);
- microseconds diff = 1min;
- auto t2 = t1 - diff.count();
- hostEpoch.elapsed(t2);
-
- // Verify that the offset shall be negative, and the absolute value
- // shall be equal or greater than diff, and shall not be too greater
- auto offset = getOffset();
- EXPECT_LT(offset, USEC_ZERO);
- offset = -offset;
- EXPECT_GE(offset, diff);
- diff += 10s;
- EXPECT_LE(offset, diff);
-
- // Now get time shall be around past -1min time
- auto epochNow =
- duration_cast<microseconds>(system_clock::now().time_since_epoch())
- .count();
- auto elapsedGot = hostEpoch.elapsed();
- EXPECT_LT(elapsedGot, epochNow);
- auto epochDiff = epochNow - elapsedGot;
- diff = 1min;
- EXPECT_GT(epochDiff, (diff - delta).count());
- EXPECT_LT(epochDiff, (diff + delta).count());
- }
-};
-
-TEST_F(TestHostEpoch, empty)
-{
- // Default mode/owner is MANUAL/BOTH
- EXPECT_EQ(Mode::Manual, getTimeMode());
- EXPECT_EQ(Owner::Both, getTimeOwner());
-}
-
-TEST_F(TestHostEpoch, readDataFileNotExist)
-{
- // When file does not exist, the default offset shall be 0
- microseconds offset(0);
- auto value = utils::readData<decltype(offset)::rep>(FILE_NOT_EXIST);
- EXPECT_EQ(0, value);
-}
-
-TEST_F(TestHostEpoch, writeAndReadData)
-{
- // Write offset to file
- microseconds offsetToWrite(1234567);
- utils::writeData<decltype(offsetToWrite)::rep>(FILE_OFFSET,
- offsetToWrite.count());
-
- // Read it back
- microseconds offsetToRead;
- offsetToRead =
- microseconds(utils::readData<decltype(offsetToRead)::rep>(FILE_OFFSET));
- EXPECT_EQ(offsetToWrite, offsetToRead);
-}
-
-TEST_F(TestHostEpoch, setElapsedInNtpBmc)
-{
- // Set time in NTP/BMC is not allowed
- setTimeMode(Mode::NTP);
- setTimeOwner(Owner::BMC);
- checkSettingTimeNotAllowed();
-}
-
-TEST_F(TestHostEpoch, setElapsedInNtpHost)
-{
- // Set time in NTP/HOST is not allowed
- setTimeMode(Mode::NTP);
- setTimeOwner(Owner::Host);
- checkSettingTimeNotAllowed();
-}
-
-TEST_F(TestHostEpoch, setElapsedInNtpSplit)
-{
- // Set time in NTP/SPLIT, offset will be set
- setTimeMode(Mode::NTP);
- setTimeOwner(Owner::Split);
-
- checkSetSplitTimeInFuture();
-
- // Reset offset
- setOffset(USEC_ZERO);
- checkSetSplitTimeInPast();
-}
-
-TEST_F(TestHostEpoch, setElapsedInNtpBoth)
-{
- // Set time in NTP/BOTH is not allowed
- setTimeMode(Mode::NTP);
- setTimeOwner(Owner::Both);
- checkSettingTimeNotAllowed();
-}
-
-TEST_F(TestHostEpoch, setElapsedInManualBmc)
-{
- // Set time in MANUAL/BMC is not allowed
- setTimeMode(Mode::Manual);
- setTimeOwner(Owner::BMC);
- checkSettingTimeNotAllowed();
-}
-
-TEST_F(TestHostEpoch, setElapsedInManualHost)
-{
- // Set time in MANUAL/HOST, time will be set to BMC
- // However it requies gmock to test this case
- // TODO: when gmock is ready, test this case.
- setTimeMode(Mode::Manual);
- setTimeOwner(Owner::Host);
-}
-
-TEST_F(TestHostEpoch, setElapsedInManualSplit)
-{
- // Set to SPLIT owner so that offset will be set
- setTimeMode(Mode::Manual);
- setTimeOwner(Owner::Split);
-
- checkSetSplitTimeInFuture();
-
- // Reset offset
- setOffset(USEC_ZERO);
- checkSetSplitTimeInPast();
-}
-
-TEST_F(TestHostEpoch, setElapsedInManualBoth)
-{
- // Set time in MANUAL/BOTH, time will be set to BMC
- // However it requies gmock to test this case
- // TODO: when gmock is ready, test this case.
- setTimeMode(Mode::Manual);
- setTimeOwner(Owner::Both);
-}
-
-TEST_F(TestHostEpoch, setElapsedInSplitAndBmcTimeIsChanged)
-{
- // Set to SPLIT owner so that offset will be set
- setTimeOwner(Owner::Split);
-
- // Get current time, and set future +1min time
- auto t1 = hostEpoch.elapsed();
- EXPECT_NE(0, t1);
- microseconds diff = 1min;
- auto t2 = t1 + diff.count();
- hostEpoch.elapsed(t2);
-
- // Verify that the offset shall be positive,
- // and less or equal to diff, and shall be not too less.
- auto offset = getOffset();
- EXPECT_GT(offset, USEC_ZERO);
- EXPECT_LE(offset, diff);
- diff -= delta;
- EXPECT_GE(offset, diff);
-
- // Now BMC time is changed to future +1min
- hostEpoch.onBmcTimeChanged(microseconds(t2));
-
- // Verify that the offset shall be around zero since it's almost
- // the same as BMC time
- offset = getOffset();
- if (offset.count() < 0)
- {
- offset = microseconds(-offset.count());
- }
- EXPECT_LE(offset, delta);
-}
-
-TEST_F(TestHostEpoch, clearOffsetOnOwnerChange)
-{
- EXPECT_EQ(USEC_ZERO, getOffset());
-
- setTimeOwner(Owner::Split);
- hostEpoch.onBmcTimeChanged(microseconds(hostEpoch.elapsed()) + 1min);
-
- // Now offset shall be non zero
- EXPECT_NE(USEC_ZERO, getOffset());
-
- setTimeOwner(Owner::Both);
-
- // Now owner is BOTH, the offset shall be cleared
- EXPECT_EQ(USEC_ZERO, getOffset());
-}
-
-} // namespace time
-} // namespace phosphor
diff --git a/test/TestManager.cpp b/test/TestManager.cpp
index c0763c8..95f5734 100644
--- a/test/TestManager.cpp
+++ b/test/TestManager.cpp
@@ -34,10 +34,6 @@
{
return manager.timeMode;
}
- Owner getTimeOwner()
- {
- return manager.timeOwner;
- }
bool hostOn()
{
return manager.hostOn;
@@ -46,10 +42,6 @@
{
return manager.requestedMode;
}
- std::string getRequestedOwner()
- {
- return manager.requestedOwner;
- }
void notifyPropertyChanged(const std::string& key, const std::string& value)
{
manager.onPropertyChanged(key, value);
@@ -64,11 +56,9 @@
{
EXPECT_FALSE(hostOn());
EXPECT_EQ("", getRequestedMode());
- EXPECT_EQ("", getRequestedOwner());
- // Default mode/owner is MANUAL/BOTH
+ // Default mode is MANUAL
EXPECT_EQ(Mode::Manual, getTimeMode());
- EXPECT_EQ(Owner::Both, getTimeOwner());
}
TEST_F(TestManager, DISABLED_hostStateChange)
@@ -86,50 +76,36 @@
// Check mocked listeners shall receive notifications on property changed
EXPECT_CALL(listener1, onModeChanged(Mode::Manual)).Times(1);
- EXPECT_CALL(listener1, onOwnerChanged(Owner::Host)).Times(1);
EXPECT_CALL(listener2, onModeChanged(Mode::Manual)).Times(1);
- EXPECT_CALL(listener2, onOwnerChanged(Owner::Host)).Times(1);
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.Manual");
- notifyPropertyChanged("TimeOwner",
- "xyz.openbmc_project.Time.Owner.Owners.Host");
EXPECT_EQ("", getRequestedMode());
- EXPECT_EQ("", getRequestedOwner());
// 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(listener1, onOwnerChanged(Owner::Host)).Times(0);
EXPECT_CALL(listener2, onModeChanged(Mode::Manual)).Times(0);
- EXPECT_CALL(listener2, onOwnerChanged(Owner::Host)).Times(0);
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.NTP");
- notifyPropertyChanged("TimeOwner",
- "xyz.openbmc_project.Time.Owner.Owners.Split");
EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.NTP",
getRequestedMode());
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Split",
- getRequestedOwner());
- // When host becomes off, the requested mode/owner shall be notified
+ // 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(listener1, onOwnerChanged(Owner::Split)).Times(1);
EXPECT_CALL(listener2, onModeChanged(Mode::NTP)).Times(1);
- EXPECT_CALL(listener2, onOwnerChanged(Owner::Split)).Times(1);
notifyOnHostState(false);
EXPECT_EQ("", getRequestedMode());
- EXPECT_EQ("", getRequestedOwner());
// When host is on, and invalid property is changed,
// verify the code asserts because it shall never occur
@@ -143,55 +119,40 @@
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.Manual");
- notifyPropertyChanged("TimeOwner",
- "xyz.openbmc_project.Time.Owner.Owners.Host");
// Set host on
notifyOnHostState(true);
// Check mocked listeners shall not receive notifications
EXPECT_CALL(listener1, onModeChanged(_)).Times(0);
- EXPECT_CALL(listener1, onOwnerChanged(_)).Times(0);
EXPECT_CALL(listener2, onModeChanged(_)).Times(0);
- EXPECT_CALL(listener2, onOwnerChanged(_)).Times(0);
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.NTP");
- notifyPropertyChanged("TimeOwner",
- "xyz.openbmc_project.Time.Owner.Owners.Split");
- // Saved as requested mode/owner
+ // Saved as requested mode
EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.NTP",
getRequestedMode());
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Split",
- getRequestedOwner());
// Property changed back to MANUAL/HOST
notifyPropertyChanged(
"TimeSyncMethod",
"xyz.openbmc_project.Time.Synchronization.Method.Manual");
- notifyPropertyChanged("TimeOwner",
- "xyz.openbmc_project.Time.Owner.Owners.Host");
- // Requested mode/owner shall be updated
+ // Requested mode shall be updated
EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.Manual",
getRequestedMode());
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Host",
- getRequestedOwner());
- // Because the latest mode/owner is the same as when host is off,
- // The listeners shall not be notified, and requested mode/owner
+ // 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(listener1, onOwnerChanged(_)).Times(0);
EXPECT_CALL(listener2, onModeChanged(_)).Times(0);
- EXPECT_CALL(listener2, onOwnerChanged(_)).Times(0);
notifyOnHostState(false);
EXPECT_EQ("", getRequestedMode());
- EXPECT_EQ("", getRequestedOwner());
}
// TODO: if gmock is ready, add case to test
diff --git a/test/TestUtils.cpp b/test/TestUtils.cpp
index 1cbc852..3d38171 100644
--- a/test/TestUtils.cpp
+++ b/test/TestUtils.cpp
@@ -29,23 +29,6 @@
EXPECT_THROW(strToMode("whatever"), InvalidEnumString);
}
-TEST(TestUtil, strToOwner)
-{
- EXPECT_EQ(Owner::BMC,
- strToOwner("xyz.openbmc_project.Time.Owner.Owners.BMC"));
- EXPECT_EQ(Owner::Host,
- strToOwner("xyz.openbmc_project.Time.Owner.Owners.Host"));
- EXPECT_EQ(Owner::Split,
- strToOwner("xyz.openbmc_project.Time.Owner.Owners.Split"));
- EXPECT_EQ(Owner::Both,
- strToOwner("xyz.openbmc_project.Time.Owner.Owners.Both"));
-
- // All unrecognized strings result in InvalidEnumString exception
- EXPECT_THROW(strToOwner(""), InvalidEnumString);
- EXPECT_THROW(strToOwner("Split"), InvalidEnumString);
- EXPECT_THROW(strToOwner("xyz"), InvalidEnumString);
-}
-
TEST(TestUtil, modeToStr)
{
EXPECT_EQ("xyz.openbmc_project.Time.Synchronization.Method.NTP",
@@ -57,21 +40,6 @@
EXPECT_ANY_THROW(modeToStr(static_cast<Mode>(100)));
}
-TEST(TestUtil, ownerToStr)
-{
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.BMC",
- ownerToStr(Owner::BMC));
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Host",
- ownerToStr(Owner::Host));
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Split",
- ownerToStr(Owner::Split));
- EXPECT_EQ("xyz.openbmc_project.Time.Owner.Owners.Both",
- ownerToStr(Owner::Both));
-
- // All unrecognized strings result in exception
- EXPECT_ANY_THROW(ownerToStr(static_cast<Owner>(100)));
-}
-
} // namespace utils
} // namespace time
} // namespace phosphor
diff --git a/test/mocked_bmc_time_change_listener.hpp b/test/mocked_bmc_time_change_listener.hpp
deleted file mode 100644
index 364b333..0000000
--- a/test/mocked_bmc_time_change_listener.hpp
+++ /dev/null
@@ -1,18 +0,0 @@
-#pragma once
-#include "bmc_time_change_listener.hpp"
-
-#include <gmock/gmock.h>
-
-namespace phosphor
-{
-namespace time
-{
-
-class MockBmcTimeChangeListener : public BmcTimeChangeListener
-{
- public:
- MOCK_METHOD1(onBmcTimeChanged, void(const std::chrono::microseconds&));
-};
-
-} // namespace time
-} // namespace phosphor
diff --git a/test/mocked_property_change_listener.hpp b/test/mocked_property_change_listener.hpp
index 5d7a3b5..b5d6511 100644
--- a/test/mocked_property_change_listener.hpp
+++ b/test/mocked_property_change_listener.hpp
@@ -12,7 +12,6 @@
{
public:
MOCK_METHOD1(onModeChanged, void(Mode mode));
- MOCK_METHOD1(onOwnerChanged, void(Owner owner));
};
} // namespace time