Update error handling on sdbusplus calls

Error handling was added to the bus.call() routine,
which effectively deprecated the is_method_error()
check. Errors should now be handled in a try/catch
block.

Change-Id: I1dcbbaa91db443445ef6bb2f616bd37f06731d36
Signed-off-by: Anthony Wilson <wilsonan@us.ibm.com>

6 files changed, 203 insertions, 144 deletions

diff --git a/bmc_state_manager.cpp b/bmc_state_manager.cpp
index 6651d2f..20efcf0 100644
--- a/bmc_state_manager.cpp
+++ b/bmc_state_manager.cpp
@@ -1,5 +1,6 @@
 #include <cassert>
 #include <phosphor-logging/log.hpp>
+#include <sdbusplus/exception.hpp>
 #include <sys/sysinfo.h>
 #include "bmc_state_manager.hpp"
 
@@ -14,6 +15,7 @@ namespace manager
 namespace server = sdbusplus::xyz::openbmc_project::State::server;
 
 using namespace phosphor::logging;
+using sdbusplus::exception::SdBusError;
 
 constexpr auto obmcStandbyTarget = "obmc-standby.target";
 constexpr auto signalDone = "done";
@@ -38,17 +40,17 @@ void BMC::discoverInitialState()
 
     method.append(obmcStandbyTarget);
 
-    auto result = this->bus.call(method);
-
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
+    try
+    {
+        auto result = this->bus.call(method);
+        result.read(unitTargetPath);
+    }
+    catch (const SdBusError& e)
     {
-        log<level::ERR>("Error in bus call.");
+        log<level::ERR>("Error in GetUnit call", entry("ERROR=%s", e.what()));
         return;
     }
 
-    result.read(unitTargetPath);
-
     method = this->bus.new_method_call(
         SYSTEMD_SERVICE,
         static_cast<const std::string&>(unitTargetPath).c_str(),
@@ -56,19 +58,23 @@ void BMC::discoverInitialState()
 
     method.append("org.freedesktop.systemd1.Unit", "ActiveState");
 
-    result = this->bus.call(method);
+    try
+    {
+        auto result = this->bus.call(method);
 
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
+        // Is obmc-standby.target active or inactive?
+        result.read(currentState);
+    }
+    catch (const SdBusError& e)
     {
-        log<level::INFO>("Error in bus call.");
+        log<level::INFO>("Error in ActiveState Get",
+                         entry("ERROR=%s", e.what()));
         return;
     }
 
-    // Is obmc-standby.target active or inactive?
-    result.read(currentState);
-
-    if (currentState == activeState)
+    auto currentStateStr =
+        sdbusplus::message::variant_ns::get<std::string>(currentState);
+    if (currentStateStr == activeState)
     {
         log<level::INFO>("Setting the BMCState field",
                          entry("CURRENT_BMC_STATE=%s", "BMC_READY"));
@@ -77,8 +83,16 @@ void BMC::discoverInitialState()
         // Unsubscribe so we stop processing all other signals
         method = this->bus.new_method_call(SYSTEMD_SERVICE, SYSTEMD_OBJ_PATH,
                                            SYSTEMD_INTERFACE, "Unsubscribe");
-        this->bus.call(method);
-        this->stateSignal.release();
+        try
+        {
+            this->bus.call(method);
+            this->stateSignal.release();
+        }
+        catch (const SdBusError& e)
+        {
+            log<level::INFO>("Error in Unsubscribe",
+                             entry("ERROR=%s", e.what()));
+        }
     }
     else
     {
@@ -94,7 +108,15 @@ void BMC::subscribeToSystemdSignals()
 {
     auto method = this->bus.new_method_call(SYSTEMD_SERVICE, SYSTEMD_OBJ_PATH,
                                             SYSTEMD_INTERFACE, "Subscribe");
-    this->bus.call(method);
+
+    try
+    {
+        this->bus.call(method);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::INFO>("Error in Subscribe", entry("ERROR=%s", e.what()));
+    }
 
     return;
 }
@@ -114,7 +136,16 @@ void BMC::executeTransition(const Transition tranReq)
     // needs to be irreversible once started
     method.append(sysdUnit, "replace-irreversibly");
 
-    this->bus.call(method);
+    try
+    {
+        this->bus.call(method);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::INFO>("Error in StartUnit - replace-irreversibly",
+                         entry("ERROR=%s", e.what()));
+    }
+
     return;
 }
 
@@ -138,8 +169,17 @@ int BMC::bmcStateChange(sdbusplus::message::message& msg)
         auto method =
             this->bus.new_method_call(SYSTEMD_SERVICE, SYSTEMD_OBJ_PATH,
                                       SYSTEMD_INTERFACE, "Unsubscribe");
-        this->bus.call(method);
-        this->stateSignal.release();
+
+        try
+        {
+            this->bus.call(method);
+            this->stateSignal.release();
+        }
+        catch (const SdBusError& e)
+        {
+            log<level::INFO>("Error in Unsubscribe",
+                             entry("ERROR=%s", e.what()));
+        }
     }
 
     return 0;
diff --git a/chassis_state_manager.cpp b/chassis_state_manager.cpp
index 16cecec..8fd56b8 100644
--- a/chassis_state_manager.cpp
+++ b/chassis_state_manager.cpp
@@ -70,24 +70,7 @@ void Chassis::determineInitialState()
     try
     {
         auto reply = this->bus.call(method);
-        if (reply.is_method_error())
-        {
-            log<level::ERR>(
-                "Error in response message - could not get initial pgood");
-            goto fail;
-        }
-
-        try
-        {
-            reply.read(pgood);
-        }
-        catch (const SdBusError& e)
-        {
-            log<level::ERR>("Error in bus response - bad encoding of pgood",
-                            entry("ERROR=%s", e.what()),
-                            entry("REPLY_SIG=%s", reply.get_signature()));
-            goto fail;
-        }
+        reply.read(pgood);
 
         if (sdbusplus::message::variant_ns::get<int>(pgood) == 1)
         {
@@ -165,25 +148,15 @@ bool Chassis::stateActive(const std::string& target)
                                             SYSTEMD_INTERFACE, "GetUnit");
 
     method.append(target);
-    auto result = this->bus.call(method);
-
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
-    {
-        log<level::ERR>("Error in bus call - could not resolve GetUnit for:",
-                        entry(" %s", SYSTEMD_INTERFACE));
-        return false;
-    }
 
     try
     {
+        auto result = this->bus.call(method);
         result.read(unitTargetPath);
     }
     catch (const SdBusError& e)
     {
-        log<level::ERR>("Error in bus response - bad encoding for GetUnit",
-                        entry("ERROR=%s", e.what()),
-                        entry("REPLY_SIG=%s", result.get_signature()));
+        log<level::ERR>("Error in GetUnit call", entry("ERROR=%s", e.what()));
         return false;
     }
 
@@ -193,17 +166,19 @@ bool Chassis::stateActive(const std::string& target)
         SYSTEMD_PROPERTY_IFACE, "Get");
 
     method.append(SYSTEMD_INTERFACE_UNIT, "ActiveState");
-    result = this->bus.call(method);
 
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
+    try
     {
-        log<level::ERR>("Error in bus call - could not resolve Get for:",
-                        entry(" %s", SYSTEMD_PROPERTY_IFACE));
+        auto result = this->bus.call(method);
+        result.read(currentState);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in ActiveState Get",
+                        entry("ERROR=%s", e.what()));
         return false;
     }
 
-    result.read(currentState);
     const auto& currentStateStr =
         sdbusplus::message::variant_ns::get<std::string>(currentState);
     return currentStateStr == ACTIVE_STATE ||
diff --git a/discover_system_state.cpp b/discover_system_state.cpp
index 92d3416..3a38152 100644
--- a/discover_system_state.cpp
+++ b/discover_system_state.cpp
@@ -4,6 +4,7 @@
 #include <string>
 #include <config.h>
 #include <systemd/sd-bus.h>
+#include <sdbusplus/exception.hpp>
 #include <sdbusplus/server.hpp>
 #include <phosphor-logging/log.hpp>
 #include <phosphor-logging/elog-errors.hpp>
@@ -22,6 +23,7 @@ namespace manager
 using namespace phosphor::logging;
 using namespace sdbusplus::xyz::openbmc_project::Common::Error;
 using namespace sdbusplus::xyz::openbmc_project::Control::Power::server;
+using sdbusplus::exception::SdBusError;
 
 constexpr auto MAPPER_BUSNAME = "xyz.openbmc_project.ObjectMapper";
 constexpr auto MAPPER_PATH = "/xyz/openbmc_project/object_mapper";
@@ -36,23 +38,27 @@ std::string getService(sdbusplus::bus::bus& bus, std::string path,
                                       MAPPER_INTERFACE, "GetObject");
 
     mapper.append(path, std::vector<std::string>({interface}));
-    auto mapperResponseMsg = bus.call(mapper);
 
-    if (mapperResponseMsg.is_method_error())
+    std::map<std::string, std::vector<std::string>> mapperResponse;
+    try
     {
-        log<level::ERR>("Error in mapper call", entry("PATH=%s", path.c_str()),
-                        entry("INTERFACE=%s", interface.c_str()));
-        throw std::runtime_error("Error in mapper call");
-    }
+        auto mapperResponseMsg = bus.call(mapper);
 
-    std::map<std::string, std::vector<std::string>> mapperResponse;
-    mapperResponseMsg.read(mapperResponse);
-    if (mapperResponse.empty())
+        mapperResponseMsg.read(mapperResponse);
+        if (mapperResponse.empty())
+        {
+            log<level::ERR>("Error reading mapper response",
+                            entry("PATH=%s", path.c_str()),
+                            entry("INTERFACE=%s", interface.c_str()));
+            throw std::runtime_error("Error reading mapper response");
+        }
+    }
+    catch (const SdBusError& e)
     {
-        log<level::ERR>("Error reading mapper response",
+        log<level::ERR>("Error in mapper call", entry("ERROR=%s", e.what()),
                         entry("PATH=%s", path.c_str()),
                         entry("INTERFACE=%s", interface.c_str()));
-        throw std::runtime_error("Error reading mapper response");
+        throw;
     }
 
     return mapperResponse.begin()->first;
@@ -68,17 +74,19 @@ std::string getProperty(sdbusplus::bus::bus& bus, std::string path,
                                       PROPERTY_INTERFACE, "Get");
 
     method.append(interface, propertyName);
-    auto reply = bus.call(method);
 
-    if (reply.is_method_error())
+    try
     {
-        log<level::ERR>("Error in property Get",
+        auto reply = bus.call(method);
+        reply.read(property);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in property Get", entry("ERROR=%s", e.what()),
                         entry("PROPERTY=%s", propertyName.c_str()));
-        throw std::runtime_error("Error in property Get");
+        throw;
     }
 
-    reply.read(property);
-
     if (sdbusplus::message::variant_ns::get<std::string>(property).empty())
     {
         log<level::ERR>("Error reading property response",
@@ -148,15 +156,20 @@ int main(int argc, char** argv)
         settings.powerRestorePolicy.c_str(), "org.freedesktop.DBus.Properties",
         "Get");
     method.append(powerRestoreIntf, "PowerRestorePolicy");
-    auto reply = bus.call(method);
-    if (reply.is_method_error())
+
+    sdbusplus::message::variant<std::string> result;
+    try
     {
-        log<level::ERR>("Error in PowerRestorePolicy Get");
+        auto reply = bus.call(method);
+        reply.read(result);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in PowerRestorePolicy Get",
+                        entry("ERROR=%s", e.what()));
         elog<InternalFailure>();
     }
 
-    sdbusplus::message::variant<std::string> result;
-    reply.read(result);
     auto powerPolicy = sdbusplus::message::variant_ns::get<std::string>(result);
 
     log<level::INFO>("Host power is off, checking power policy",
diff --git a/host_check_main.cpp b/host_check_main.cpp
index 54a182a..311e6e2 100644
--- a/host_check_main.cpp
+++ b/host_check_main.cpp
@@ -4,6 +4,7 @@
 #include <fstream>
 #include <cstdio>
 #include <sdbusplus/bus.hpp>
+#include <sdbusplus/exception.hpp>
 #include <phosphor-logging/log.hpp>
 #include <xyz/openbmc_project/Control/Host/server.hpp>
 #include <config.h>
@@ -11,6 +12,7 @@
 using namespace std::literals;
 using namespace phosphor::logging;
 using namespace sdbusplus::xyz::openbmc_project::Control::server;
+using sdbusplus::exception::SdBusError;
 
 // Required strings for sending the msg to check on host
 constexpr auto MAPPER_BUSNAME = "xyz.openbmc_project.ObjectMapper";
@@ -59,17 +61,21 @@ void sendHeartbeat(sdbusplus::bus::bus& bus)
 
     mapper.append(CONTROL_HOST_PATH,
                   std::vector<std::string>({CONTROL_HOST_INTERFACE}));
-    auto mapperResponseMsg = bus.call(mapper);
 
-    if (mapperResponseMsg.is_method_error())
+    std::map<std::string, std::vector<std::string>> mapperResponse;
+
+    try
     {
-        log<level::ERR>("Error in mapper call for control host");
-        // TODO openbmc/openbmc#851 - Once available, throw returned error
-        throw std::runtime_error("Error in mapper call for control host");
+        auto mapperResponseMsg = bus.call(mapper);
+        mapperResponseMsg.read(mapperResponse);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in mapper call for control host",
+                        entry("ERROR=%s", e.what()));
+        throw;
     }
 
-    std::map<std::string, std::vector<std::string>> mapperResponse;
-    mapperResponseMsg.read(mapperResponse);
     if (mapperResponse.empty())
     {
         log<level::ERR>("Error reading mapper resp for control host");
@@ -83,11 +89,15 @@ void sendHeartbeat(sdbusplus::bus::bus& bus)
                                       CONTROL_HOST_INTERFACE, "Execute");
     method.append(convertForMessage(Host::Command::Heartbeat).c_str());
 
-    auto reply = bus.call(method);
-    if (reply.is_method_error())
+    try
+    {
+        auto reply = bus.call(method);
+    }
+    catch (const SdBusError& e)
     {
-        log<level::ERR>("Error in call to control host Execute");
-        throw std::runtime_error("Error in call to control host Execute");
+        log<level::ERR>("Error in call to control host Execute",
+                        entry("ERROR=%s", e.what()));
+        throw;
     }
 
     return;
diff --git a/host_state_manager.cpp b/host_state_manager.cpp
index aaac46c..7d661dd 100644
--- a/host_state_manager.cpp
+++ b/host_state_manager.cpp
@@ -8,6 +8,7 @@
 #include <cereal/types/tuple.hpp>
 #include <cereal/archives/json.hpp>
 #include <fstream>
+#include <sdbusplus/exception.hpp>
 #include <sdbusplus/server.hpp>
 #include <phosphor-logging/log.hpp>
 #include <phosphor-logging/elog-errors.hpp>
@@ -34,6 +35,7 @@ namespace osstatus =
     sdbusplus::xyz::openbmc_project::State::OperatingSystem::server;
 using namespace phosphor::logging;
 namespace fs = std::experimental::filesystem;
+using sdbusplus::exception::SdBusError;
 
 // host-shutdown notifies host of shutdown and that leads to host-stop being
 // called so initiate a host shutdown with the -shutdown target and consider the
@@ -133,35 +135,37 @@ bool Host::stateActive(const std::string& target)
                                             SYSTEMD_INTERFACE, "GetUnit");
 
     method.append(target);
-    auto result = this->bus.call(method);
 
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
+    try
+    {
+        auto result = this->bus.call(method);
+        result.read(unitTargetPath);
+    }
+    catch (const SdBusError& e)
     {
-        log<level::ERR>("Error in bus call - could not resolve GetUnit for:",
-                        entry(" %s", SYSTEMD_INTERFACE));
+        log<level::ERR>("Error in GetUnit call", entry("ERROR=%s", e.what()));
         return false;
     }
 
-    result.read(unitTargetPath);
-
     method = this->bus.new_method_call(
         SYSTEMD_SERVICE,
         static_cast<const std::string&>(unitTargetPath).c_str(),
         SYSTEMD_PROPERTY_IFACE, "Get");
 
     method.append(SYSTEMD_INTERFACE_UNIT, "ActiveState");
-    result = this->bus.call(method);
 
-    // Check that the bus call didn't result in an error
-    if (result.is_method_error())
+    try
     {
-        log<level::ERR>("Error in bus call - could not resolve Get for:",
-                        entry(" %s", SYSTEMD_PROPERTY_IFACE));
+        auto result = this->bus.call(method);
+        result.read(currentState);
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in ActiveState Get",
+                        entry("ERROR=%s", e.what()));
         return false;
     }
 
-    result.read(currentState);
     const auto& currentStateStr =
         sdbusplus::message::variant_ns::get<std::string>(currentState);
     return currentStateStr == ACTIVE_STATE ||
@@ -176,44 +180,48 @@ bool Host::isAutoReboot()
         settings.service(settings.autoReboot, autoRebootIntf).c_str(),
         settings.autoReboot.c_str(), "org.freedesktop.DBus.Properties", "Get");
     method.append(autoRebootIntf, "AutoReboot");
-    auto reply = bus.call(method);
-    if (reply.is_method_error())
+
+    try
     {
-        log<level::ERR>("Error in AutoReboot Get");
-        return false;
-    }
+        auto reply = bus.call(method);
 
-    sdbusplus::message::variant<bool> result;
-    reply.read(result);
-    auto autoReboot = sdbusplus::message::variant_ns::get<bool>(result);
-    auto rebootCounterParam = reboot::RebootAttempts::attemptsLeft();
+        sdbusplus::message::variant<bool> result;
+        reply.read(result);
+        auto autoReboot = sdbusplus::message::variant_ns::get<bool>(result);
+        auto rebootCounterParam = reboot::RebootAttempts::attemptsLeft();
 
-    if (autoReboot)
-    {
-        if (rebootCounterParam > 0)
-        {
-            // Reduce BOOTCOUNT by 1
-            log<level::INFO>("Auto reboot enabled, rebooting");
-            return true;
-        }
-        else if (rebootCounterParam == 0)
+        if (autoReboot)
         {
-            // Reset reboot counter and go to quiesce state
-            log<level::INFO>("Auto reboot enabled. "
-                             "HOST BOOTCOUNT already set to 0.");
-            attemptsLeft(BOOT_COUNT_MAX_ALLOWED);
-            return false;
+            if (rebootCounterParam > 0)
+            {
+                // Reduce BOOTCOUNT by 1
+                log<level::INFO>("Auto reboot enabled, rebooting");
+                return true;
+            }
+            else if (rebootCounterParam == 0)
+            {
+                // Reset reboot counter and go to quiesce state
+                log<level::INFO>("Auto reboot enabled. "
+                                 "HOST BOOTCOUNT already set to 0.");
+                attemptsLeft(BOOT_COUNT_MAX_ALLOWED);
+                return false;
+            }
+            else
+            {
+                log<level::INFO>("Auto reboot enabled. "
+                                 "HOST BOOTCOUNT has an invalid value.");
+                return false;
+            }
         }
         else
         {
-            log<level::INFO>("Auto reboot enabled. "
-                             "HOST BOOTCOUNT has an invalid value.");
+            log<level::INFO>("Auto reboot disabled.");
             return false;
         }
     }
-    else
+    catch (const SdBusError& e)
     {
-        log<level::INFO>("Auto reboot disabled.");
+        log<level::ERR>("Error in AutoReboot Get", entry("ERROR=%s", e.what()));
         return false;
     }
 }
diff --git a/settings.cpp b/settings.cpp
index 70bf04e..7f9f236 100644
--- a/settings.cpp
+++ b/settings.cpp
@@ -1,5 +1,6 @@
 #include <phosphor-logging/elog-errors.hpp>
 #include <phosphor-logging/log.hpp>
+#include <sdbusplus/exception.hpp>
 #include "xyz/openbmc_project/Common/error.hpp"
 #include "settings.hpp"
 
@@ -8,6 +9,7 @@ namespace settings
 
 using namespace phosphor::logging;
 using namespace sdbusplus::xyz::openbmc_project::Common::Error;
+using sdbusplus::exception::SdBusError;
 
 constexpr auto mapperService = "xyz.openbmc_project.ObjectMapper";
 constexpr auto mapperPath = "/xyz/openbmc_project/object_mapper";
@@ -23,20 +25,26 @@ Objects::Objects(sdbusplus::bus::bus& bus) : bus(bus)
     mapperCall.append(root);
     mapperCall.append(depth);
     mapperCall.append(settingsIntfs);
-    auto response = bus.call(mapperCall);
-    if (response.is_method_error())
-    {
-        log<level::ERR>("Error in mapper GetSubTree");
-        elog<InternalFailure>();
-    }
 
     using Interfaces = std::vector<Interface>;
     using MapperResponse = std::map<Path, std::map<Service, Interfaces>>;
     MapperResponse result;
-    response.read(result);
-    if (result.empty())
+
+    try
     {
-        log<level::ERR>("Invalid response from mapper");
+        auto response = bus.call(mapperCall);
+
+        response.read(result);
+        if (result.empty())
+        {
+            log<level::ERR>("Invalid response from mapper");
+            elog<InternalFailure>();
+        }
+    }
+    catch (const SdBusError& e)
+    {
+        log<level::ERR>("Error in mapper GetSubTree",
+                        entry("ERROR=%s", e.what()));
         elog<InternalFailure>();
     }
 
@@ -69,15 +77,20 @@ Service Objects::service(const Path& path, const Interface& interface) const
     mapperCall.append(path);
     mapperCall.append(Interfaces({interface}));
 
-    auto response = bus.call(mapperCall);
-    if (response.is_method_error())
+    std::map<Service, Interfaces> result;
+
+    try
+    {
+        auto response = bus.call(mapperCall);
+        response.read(result);
+    }
+    catch (const SdBusError& e)
     {
-        log<level::ERR>("Error in mapper GetObject");
+        log<level::ERR>("Error in mapper GetObject",
+                        entry("ERROR=%s", e.what()));
         elog<InternalFailure>();
     }
 
-    std::map<Service, Interfaces> result;
-    response.read(result);
     if (result.empty())
     {
         log<level::ERR>("Invalid response from mapper");