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/utils.cpp b/src/utils.cpp
index abea750..27b6eea 100644
--- a/src/utils.cpp
+++ b/src/utils.cpp
@@ -7,8 +7,13 @@
#include <phosphor-logging/lg2.hpp>
#include <algorithm>
+#include <cerrno>
+#include <cstring>
+#include <exception>
+#include <format>
#include <fstream>
#include <sstream>
+#include <stdexcept>
namespace utils
{
@@ -22,16 +27,34 @@
namespace internal
{
+
+/**
+ * @brief Concatenate the specified values, separated by spaces, and return
+ * the resulting string.
+ *
+ * @param[in] ts - Parameter pack of values to concatenate
+ *
+ * @return Parameter values separated by spaces
+ */
template <typename... Ts>
std::string concat_string(const Ts&... ts)
{
std::stringstream s;
- ((s << ts << " "), ...) << std::endl;
+ ((s << ts << " "), ...);
return s.str();
}
-// Helper function to run command
-// Returns return code and the stdout
+/**
+ * @brief Execute the specified command.
+ *
+ * @details Returns a pair containing the exit status and command output.
+ * Throws an exception if an error occurs. Note that a command that
+ * returns a non-zero exit status is not considered an error.
+ *
+ * @param[in] ts - Parameter pack of command and parameters
+ *
+ * @return Exit status and standard output from the command
+ */
template <typename... Ts>
std::pair<int, std::string> exec(const Ts&... ts)
{
@@ -42,7 +65,9 @@
FILE* pipe = popen(cmd.c_str(), "r");
if (!pipe)
{
- throw std::runtime_error("popen() failed!");
+ throw std::runtime_error{
+ std::format("Unable to execute command '{}': popen() failed: {}",
+ cmd, std::strerror(errno))};
}
while (fgets(buffer.data(), buffer.size(), pipe) != nullptr)
{
@@ -53,6 +78,7 @@
}
} // namespace internal
+
const UtilsInterface& getUtils()
{
static Utils utils;
@@ -74,7 +100,7 @@
reply.read(paths);
}
- catch (const sdbusplus::exception_t&)
+ catch (const std::exception& e)
{
// Inventory base path not there yet.
}
@@ -87,7 +113,8 @@
auto services = getServices(bus, path, interface);
if (services.empty())
{
- return {};
+ throw std::runtime_error{std::format(
+ "No service found for path {}, interface {}", path, interface)};
}
return services[0];
}
@@ -95,36 +122,32 @@
std::vector<std::string> Utils::getServices(
sdbusplus::bus_t& bus, const char* path, const char* interface) const
{
- auto mapper = bus.new_method_call(MAPPER_BUSNAME, MAPPER_PATH,
- MAPPER_INTERFACE, "GetObject");
-
- mapper.append(path, std::vector<std::string>({interface}));
+ std::vector<std::string> services;
try
{
+ auto mapper = bus.new_method_call(MAPPER_BUSNAME, MAPPER_PATH,
+ MAPPER_INTERFACE, "GetObject");
+
+ mapper.append(path, std::vector<std::string>({interface}));
+
auto mapperResponseMsg = bus.call(mapper);
std::vector<std::pair<std::string, std::vector<std::string>>>
mapperResponse;
mapperResponseMsg.read(mapperResponse);
- if (mapperResponse.empty())
- {
- lg2::error("Error reading mapper response");
- throw std::runtime_error("Error reading mapper response");
- }
- std::vector<std::string> ret;
- ret.reserve(mapperResponse.size());
+ services.reserve(mapperResponse.size());
for (const auto& i : mapperResponse)
{
- ret.emplace_back(i.first);
+ services.emplace_back(i.first);
}
- return ret;
}
- catch (const sdbusplus::exception_t& ex)
+ catch (const std::exception& e)
{
- lg2::error("GetObject call failed: path={PATH}, interface={INTERFACE}",
- "PATH", path, "INTERFACE", interface);
- throw std::runtime_error("GetObject call failed");
+ throw std::runtime_error{
+ std::format("Unable to find services for path {}, interface {}: {}",
+ path, interface, e.what())};
}
+ return services;
}
std::string Utils::getVersionId(const std::string& version) const
@@ -156,35 +179,74 @@
std::string Utils::getVersion(const std::string& inventoryPath) const
{
- // Invoke vendor-specific tool to get the version string, e.g.
- // psutils --get-version
- // /xyz/openbmc_project/inventory/system/chassis/motherboard/powersupply0
- auto [rc, r] = internal::exec(PSU_VERSION_UTIL, inventoryPath);
- return (rc == 0) ? r : "";
+ std::string version;
+ try
+ {
+ // Invoke vendor-specific tool to get the version string, e.g.
+ // psutils --get-version
+ // /xyz/openbmc_project/inventory/system/chassis/motherboard/powersupply0
+ auto [rc, output] = internal::exec(PSU_VERSION_UTIL, inventoryPath);
+ if (rc == 0)
+ {
+ version = output;
+ }
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error("Unable to get firmware version for PSU {PSU}: {ERROR}",
+ "PSU", inventoryPath, "ERROR", e);
+ }
+ return version;
}
std::string Utils::getModel(const std::string& inventoryPath) const
{
- // Invoke vendor-specific tool to get the model string, e.g.
- // psutils --get-model
- // /xyz/openbmc_project/inventory/system/chassis/motherboard/powersupply0
- auto [rc, r] = internal::exec(PSU_MODEL_UTIL, inventoryPath);
- return (rc == 0) ? r : "";
+ std::string model;
+ try
+ {
+ // Invoke vendor-specific tool to get the model string, e.g.
+ // psutils --get-model
+ // /xyz/openbmc_project/inventory/system/chassis/motherboard/powersupply0
+ auto [rc, output] = internal::exec(PSU_MODEL_UTIL, inventoryPath);
+ if (rc == 0)
+ {
+ model = output;
+ }
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error("Unable to get model for PSU {PSU}: {ERROR}", "PSU",
+ inventoryPath, "ERROR", e);
+ }
+ return model;
}
std::string Utils::getLatestVersion(const std::set<std::string>& versions) const
{
- if (versions.empty())
+ std::string latestVersion;
+ try
{
- return {};
+ if (!versions.empty())
+ {
+ std::stringstream args;
+ for (const auto& s : versions)
+ {
+ args << s << " ";
+ }
+ auto [rc, output] =
+ internal::exec(PSU_VERSION_COMPARE_UTIL, args.str());
+ if (rc == 0)
+ {
+ latestVersion = output;
+ }
+ }
}
- std::stringstream args;
- for (const auto& s : versions)
+ catch (const std::exception& e)
{
- args << s << " ";
+ lg2::error("Unable to get latest PSU firmware version: {ERROR}",
+ "ERROR", e);
}
- auto [rc, r] = internal::exec(PSU_VERSION_COMPARE_UTIL, args.str());
- return (rc == 0) ? r : "";
+ return latestVersion;
}
bool Utils::isAssociated(const std::string& psuInventoryPath,
@@ -200,24 +262,24 @@
const char* path, const char* interface,
const char* propertyName) const
{
- auto method = bus.new_method_call(service, path,
- "org.freedesktop.DBus.Properties", "Get");
- method.append(interface, propertyName);
+ any anyValue{};
try
{
- PropertyType value{};
+ auto method = bus.new_method_call(
+ service, path, "org.freedesktop.DBus.Properties", "Get");
+ method.append(interface, propertyName);
auto reply = bus.call(method);
+ PropertyType value{};
reply.read(value);
- return any(value);
+ anyValue = value;
}
- catch (const sdbusplus::exception_t& ex)
+ catch (const std::exception& e)
{
- lg2::error(
- "GetProperty call failed: path={PATH}, interface={INTERFACE}, "
- "property={PROPERTY}",
- "PATH", path, "INTERFACE", interface, "PROPERTY", propertyName);
- throw std::runtime_error("GetProperty call failed");
+ throw std::runtime_error{std::format(
+ "Unable to get property {} for path {} and interface {}: {}",
+ propertyName, path, interface, e.what())};
}
+ return anyValue;
}
} // namespace utils