Use proper errors when failing to set time
Previously it reports InsufficientPermission when it is not allowed to
set time.
Now phosphor-dbus-interfaces defines proper errors for such case, so
report NotAllowed error when it is not allowed to set time, and report
Failed error when it failed to set time.
Tested: Get NotAllowed and Failed error with expected metadata from
journal log.
Change-Id: I53610bf27ffc3f62608cea6fd0e66ca859d94675
Signed-off-by: Lei YU <mine260309@gmail.com>
diff --git a/bmc_epoch.cpp b/bmc_epoch.cpp
index c06c464..8c251ee 100644
--- a/bmc_epoch.cpp
+++ b/bmc_epoch.cpp
@@ -1,9 +1,11 @@
#include "bmc_epoch.hpp"
+#include "utils.hpp"
#include <phosphor-logging/elog.hpp>
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/log.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
+#include <xyz/openbmc_project/Time/error.hpp>
#include <sys/timerfd.h>
#include <unistd.h>
@@ -24,7 +26,9 @@
{
namespace server = sdbusplus::xyz::openbmc_project::Time::server;
using namespace phosphor::logging;
-using namespace sdbusplus::xyz::openbmc_project::Common::Error;
+
+using NotAllowedError =
+ sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
BmcEpoch::BmcEpoch(sdbusplus::bus::bus& bus,
const char* objPath)
@@ -107,8 +111,12 @@
*/
if (timeOwner == Owner::Host)
{
- log<level::ERR>("Setting BmcTime with HOST owner is not allowed");
- elog<InsufficientPermission>();
+ using namespace xyz::openbmc_project::Time;
+ elog<NotAllowedError>(
+ NotAllowed::OWNER(utils::ownerToStr(timeOwner).c_str()),
+ NotAllowed::SYNC_METHOD(utils::modeToStr(timeMode).c_str()),
+ NotAllowed::REASON(
+ "Setting BmcTime with HOST owner is not allowed"));
}
auto time = microseconds(value);
diff --git a/epoch_base.cpp b/epoch_base.cpp
index bab5d3b..e130578 100644
--- a/epoch_base.cpp
+++ b/epoch_base.cpp
@@ -1,6 +1,9 @@
#include "epoch_base.hpp"
+#include <phosphor-logging/elog.hpp>
+#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/log.hpp>
+#include <xyz/openbmc_project/Time/error.hpp>
#include <iomanip>
#include <sstream>
@@ -19,6 +22,8 @@
{
using namespace phosphor::logging;
+using FailedError =
+ sdbusplus::xyz::openbmc_project::Time::Error::Failed;
EpochBase::EpochBase(sdbusplus::bus::bus& bus,
const char* objPath)
@@ -50,10 +55,10 @@
auto reply = bus.call(method);
if (reply.is_method_error())
{
- // TODO: When sdbus supports exception on property
- // it can just throw exception instead of returning bool
log<level::ERR>("Error in setting system time");
- return false;
+ using namespace xyz::openbmc_project::Time;
+ elog<FailedError>(
+ Failed::REASON("Systemd failed to set time"));
}
return true;
}
diff --git a/host_epoch.cpp b/host_epoch.cpp
index ea3026c..c3e6400 100644
--- a/host_epoch.cpp
+++ b/host_epoch.cpp
@@ -3,17 +3,19 @@
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/log.hpp>
-#include <xyz/openbmc_project/Common/error.hpp>
+#include <xyz/openbmc_project/Time/error.hpp>
namespace phosphor
{
namespace time
{
-using namespace sdbusplus::xyz::openbmc_project::Common::Error;
using namespace sdbusplus::xyz::openbmc_project::Time;
using namespace phosphor::logging;
using namespace std::chrono;
+using NotAllowedError =
+ sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
+
HostEpoch::HostEpoch(sdbusplus::bus::bus& bus,
const char* objPath)
: EpochBase(bus, objPath),
@@ -53,8 +55,11 @@
(timeMode == Mode::NTP
&& (timeOwner == Owner::Host || timeOwner == Owner::Both)))
{
- log<level::ERR>("Setting HostTime is not allowed");
- elog<InsufficientPermission>();
+ using namespace xyz::openbmc_project::Time;
+ elog<NotAllowedError>(
+ NotAllowed::OWNER(utils::ownerToStr(timeOwner).c_str()),
+ NotAllowed::SYNC_METHOD(utils::modeToStr(timeMode).c_str()),
+ NotAllowed::REASON("Setting HostTime is not allowed"));
}
auto time = microseconds(value);
diff --git a/test/TestBmcEpoch.cpp b/test/TestBmcEpoch.cpp
index 2d0bc72..18f3218 100644
--- a/test/TestBmcEpoch.cpp
+++ b/test/TestBmcEpoch.cpp
@@ -6,7 +6,7 @@
#include <gtest/gtest.h>
#include <memory>
#include <sdbusplus/bus.hpp>
-#include <xyz/openbmc_project/Common/error.hpp>
+#include <xyz/openbmc_project/Time/error.hpp>
namespace phosphor
{
@@ -15,8 +15,8 @@
using ::testing::_;
using namespace std::chrono;
-using InsufficientPermission =
- sdbusplus::xyz::openbmc_project::Common::Error::InsufficientPermission;
+using NotAllowed =
+ sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
class TestBmcEpoch : public testing::Test
{
@@ -93,7 +93,7 @@
setTimeOwner(Owner::Host);
EXPECT_THROW(
bmcEpoch->elapsed(epochNow),
- InsufficientPermission);
+ NotAllowed);
}
TEST_F(TestBmcEpoch, setElapsedOK)
diff --git a/test/TestHostEpoch.cpp b/test/TestHostEpoch.cpp
index 6c460ad..1004dad 100644
--- a/test/TestHostEpoch.cpp
+++ b/test/TestHostEpoch.cpp
@@ -3,7 +3,7 @@
#include "config.h"
#include "types.hpp"
-#include <xyz/openbmc_project/Common/error.hpp>
+#include <xyz/openbmc_project/Time/error.hpp>
#include <sdbusplus/bus.hpp>
#include <gtest/gtest.h>
@@ -16,8 +16,8 @@
using namespace std::chrono;
using namespace std::chrono_literals;
-using InsufficientPermission =
- sdbusplus::xyz::openbmc_project::Common::Error::InsufficientPermission;
+using NotAllowed =
+ sdbusplus::xyz::openbmc_project::Time::Error::NotAllowed;
const constexpr microseconds USEC_ZERO{0};
@@ -80,7 +80,7 @@
microseconds diff = 1min;
EXPECT_THROW(
hostEpoch.elapsed(hostEpoch.elapsed() + diff.count()),
- InsufficientPermission);
+ NotAllowed);
EXPECT_EQ(0, getOffset().count());
}