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>

5 files changed, 36 insertions, 18 deletions

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 time
 {
 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 @@ uint64_t BmcEpoch::elapsed(uint64_t value)
     */
     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 @@ namespace time
 {
 
 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 @@ bool EpochBase::setTime(const microseconds& usec)
     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 @@ uint64_t HostEpoch::elapsed(uint64_t value)
         (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 @@ namespace time
 
 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 @@ TEST_F(TestBmcEpoch, setElapsedNotAllowed)
     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 @@ namespace time
 
 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 @@ class TestHostEpoch : public testing::Test
             microseconds diff = 1min;
             EXPECT_THROW(
                 hostEpoch.elapsed(hostEpoch.elapsed() + diff.count()),
-                InsufficientPermission);
+                NotAllowed);
             EXPECT_EQ(0, getOffset().count());
         }