Improve error handling for exceptions and asserts
The phosphor-psu-code-manager application currently exits abnormally due
to the following conditions:
* Uncaught exception
* False assert() statement
An abnormal exit can result in a core dump and/or a BMC dump. It also
causes the service to be restarted. If the failure condition remains,
the restarts will fail repeatedly, and systemd will stop trying to start
the service.
Improve error handling for exceptions in the following ways:
* Add try/catch blocks to the following locations:
* Code that calls functions that throw and needs to handle exceptions.
* For example, code looping over PSU objects may need to handle an
exception for one PSU and then continue to the remaining PSUs.
* D-Bus PropertiesChanged and InterfacesAdded event handlers.
* Do not allow exceptions to escape to the sdbusplus stack frames.
* main()
* Last line of defense; catching avoids a core dump.
* Write exception error message to the journal if appropriate
Replace assert statements with exceptions or error messages to the
journal.
Tested:
* Tested all modified functions/methods.
* Verified that all exceptions were caught and logged to the journal if
appropriate.
* Verified that asserts were replaced by exceptions and logging.
* See complete test plan at
https://gist.github.com/smccarney/b4bf568639fedd269c9737234fa2803d
Change-Id: I933386e94f43a915b301d6aef7d91691816a0548
Signed-off-by: Shawn McCarney <shawnmm@us.ibm.com>
diff --git a/src/activation.cpp b/src/activation.cpp
index c52e1b6..400f03c 100644
--- a/src/activation.cpp
+++ b/src/activation.cpp
@@ -7,8 +7,11 @@
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/lg2.hpp>
-#include <cassert>
+#include <exception>
#include <filesystem>
+#include <format>
+#include <stdexcept>
+#include <vector>
namespace phosphor
{
@@ -68,37 +71,46 @@
std::string newStateUnit{};
std::string newStateResult{};
- // Read the msg and populate each variable
- msg.read(newStateID, newStateObjPath, newStateUnit, newStateResult);
-
- if (newStateUnit == psuUpdateUnit)
+ try
{
- if (newStateResult == "done")
+ // Read the msg and populate each variable
+ msg.read(newStateID, newStateObjPath, newStateUnit, newStateResult);
+
+ if (newStateUnit == psuUpdateUnit)
{
- onUpdateDone();
+ if (newStateResult == "done")
+ {
+ onUpdateDone();
+ }
+ if (newStateResult == "failed" || newStateResult == "dependency")
+ {
+ onUpdateFailed();
+ }
}
- if (newStateResult == "failed" || newStateResult == "dependency")
- {
- onUpdateFailed();
- }
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error("Unable to handle unit state change event: {ERROR}", "ERROR",
+ e);
}
}
bool Activation::doUpdate(const std::string& psuInventoryPath)
{
currentUpdatingPsu = psuInventoryPath;
- psuUpdateUnit = getUpdateService(currentUpdatingPsu);
try
{
+ psuUpdateUnit = getUpdateService(currentUpdatingPsu);
auto method = bus.new_method_call(SYSTEMD_BUSNAME, SYSTEMD_PATH,
SYSTEMD_INTERFACE, "StartUnit");
method.append(psuUpdateUnit, "replace");
bus.call_noreply(method);
return true;
}
- catch (const sdbusplus::exception_t& e)
+ catch (const std::exception& e)
{
- lg2::error("Error starting service: {ERROR}", "ERROR", e);
+ lg2::error("Error starting update service for PSU {PSU}: {ERROR}",
+ "PSU", psuInventoryPath, "ERROR", e);
onUpdateFailed();
return false;
}
@@ -235,13 +247,23 @@
void Activation::deleteImageManagerObject()
{
// Get the Delete object for <versionID> inside image_manager
- constexpr auto versionServiceStr = "xyz.openbmc_project.Software.Version";
+ std::vector<std::string> services;
constexpr auto deleteInterface = "xyz.openbmc_project.Object.Delete";
- std::string versionService;
- auto services = utils::getServices(bus, objPath.c_str(), deleteInterface);
+ try
+ {
+ services = utils::getServices(bus, objPath.c_str(), deleteInterface);
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error(
+ "Unable to find services to Delete object path {PATH}: {ERROR}",
+ "PATH", objPath, "ERROR", e);
+ }
// We need to find the phosphor-version-software-manager's version service
// to invoke the delete interface
+ constexpr auto versionServiceStr = "xyz.openbmc_project.Software.Version";
+ std::string versionService;
for (const auto& service : services)
{
if (service.find(versionServiceStr) != std::string::npos)
@@ -258,39 +280,48 @@
}
// Call the Delete object for <versionID> inside image_manager
- auto method = bus.new_method_call(versionService.c_str(), objPath.c_str(),
- deleteInterface, "Delete");
try
{
+ auto method = bus.new_method_call(
+ versionService.c_str(), objPath.c_str(), deleteInterface, "Delete");
bus.call(method);
}
- catch (const sdbusplus::exception_t& e)
+ catch (const std::exception& e)
{
- lg2::error(
- "Error performing call to Delete object path {PATH}: {ERROR}",
- "PATH", objPath, "ERROR", e);
+ lg2::error("Unable to Delete object path {PATH}: {ERROR}", "PATH",
+ objPath, "ERROR", e);
}
}
bool Activation::isCompatible(const std::string& psuInventoryPath)
{
- auto service =
- utils::getService(bus, psuInventoryPath.c_str(), ASSET_IFACE);
- auto psuManufacturer = utils::getProperty<std::string>(
- bus, service.c_str(), psuInventoryPath.c_str(), ASSET_IFACE,
- MANUFACTURER);
- auto psuModel = utils::getModel(psuInventoryPath);
- if (psuModel != model)
+ bool isCompat{false};
+ try
{
+ auto service =
+ utils::getService(bus, psuInventoryPath.c_str(), ASSET_IFACE);
+ auto psuManufacturer = utils::getProperty<std::string>(
+ bus, service.c_str(), psuInventoryPath.c_str(), ASSET_IFACE,
+ MANUFACTURER);
+ auto psuModel = utils::getModel(psuInventoryPath);
// The model shall match
- return false;
+ if (psuModel == model)
+ {
+ // If PSU inventory has manufacturer property, it shall match
+ if (psuManufacturer.empty() || (psuManufacturer == manufacturer))
+ {
+ isCompat = true;
+ }
+ }
}
- if (!psuManufacturer.empty())
+ catch (const std::exception& e)
{
- // If PSU inventory has manufacturer property, it shall match
- return psuManufacturer == manufacturer;
+ lg2::error(
+ "Unable to determine if PSU {PSU} is compatible with firmware "
+ "versionId {VERSION_ID}: {ERROR}",
+ "PSU", psuInventoryPath, "VERSION_ID", versionId, "ERROR", e);
}
- return true;
+ return isCompat;
}
void Activation::storeImage()
@@ -332,7 +363,11 @@
std::string service = PSU_UPDATE_SERVICE;
auto p = service.find('@');
- assert(p != std::string::npos);
+ if (p == std::string::npos)
+ {
+ throw std::runtime_error{std::format(
+ "Invalid PSU update service name: {}", PSU_UPDATE_SERVICE)};
+ }
service.insert(p + 1, args);
return service;
}