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;
}
diff --git a/src/activation.hpp b/src/activation.hpp
index dc708e9..015f524 100644
--- a/src/activation.hpp
+++ b/src/activation.hpp
@@ -16,6 +16,7 @@
#include <xyz/openbmc_project/Software/ExtendedVersion/server.hpp>
#include <queue>
+#include <string>
class TestActivation;
@@ -238,6 +239,8 @@
/** @brief Construct the systemd service name
*
+ * @details Throws an exception if an error occurs
+ *
* @param[in] psuInventoryPath - The PSU inventory to be updated.
*
* @return The escaped string of systemd unit to do the PSU update.
diff --git a/src/item_updater.cpp b/src/item_updater.cpp
index 2760245..62d2145 100644
--- a/src/item_updater.cpp
+++ b/src/item_updater.cpp
@@ -2,16 +2,18 @@
#include "item_updater.hpp"
+#include "runtime_warning.hpp"
#include "utils.hpp"
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/lg2.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
-#include <cassert>
+#include <exception>
#include <filesystem>
#include <format>
#include <set>
+#include <stdexcept>
namespace
{
@@ -32,14 +34,27 @@
using SVersion = server::Version;
using VersionPurpose = SVersion::VersionPurpose;
-void ItemUpdater::createActivation(sdbusplus::message_t& m)
+void ItemUpdater::onVersionInterfacesAddedMsg(sdbusplus::message_t& msg)
{
- sdbusplus::message::object_path objPath;
- std::map<std::string, std::map<std::string, std::variant<std::string>>>
- interfaces;
- m.read(objPath, interfaces);
+ try
+ {
+ sdbusplus::message::object_path objPath;
+ InterfacesAddedMap interfaces;
+ msg.read(objPath, interfaces);
- std::string path(std::move(objPath));
+ std::string path(std::move(objPath));
+ onVersionInterfacesAdded(path, interfaces);
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error("Unable to handle version InterfacesAdded event: {ERROR}",
+ "ERROR", e);
+ }
+}
+
+void ItemUpdater::onVersionInterfacesAdded(const std::string& path,
+ const InterfacesAddedMap& interfaces)
+{
std::string filePath;
auto purpose = VersionPurpose::Unknown;
std::string version;
@@ -123,7 +138,6 @@
createVersionObject(path, versionId, version, purpose);
versions.emplace(versionId, std::move(versionPtr));
}
- return;
}
void ItemUpdater::erase(const std::string& versionId)
@@ -207,8 +221,15 @@
}
auto it = activations.find(versionId);
- assert(it != activations.end());
- psuPathActivationMap.emplace(psuInventoryPath, it->second);
+ if (it == activations.end())
+ {
+ lg2::error("Unable to find Activation for version ID {VERSION_ID}",
+ "VERSION_ID", versionId);
+ }
+ else
+ {
+ psuPathActivationMap.emplace(psuInventoryPath, it->second);
+ }
}
std::unique_ptr<Activation> ItemUpdater::createActivationObject(
@@ -355,32 +376,15 @@
void ItemUpdater::onPsuInventoryChangedMsg(sdbusplus::message_t& msg)
{
- using Interface = std::string;
- Interface interface;
- Properties properties;
- std::string psuPath = msg.get_path();
-
- msg.read(interface, properties);
- onPsuInventoryChanged(psuPath, properties);
-}
-
-void ItemUpdater::onPsuInventoryChanged(const std::string& psuPath,
- const Properties& properties)
-{
try
{
- if (psuStatusMap.contains(psuPath) && properties.contains(PRESENT))
- {
- psuStatusMap[psuPath].present =
- std::get<bool>(properties.at(PRESENT));
- handlePSUPresenceChanged(psuPath);
- if (psuStatusMap[psuPath].present)
- {
- // Check if there are new PSU images to update
- processStoredImage();
- syncToLatestImage();
- }
- }
+ using Interface = std::string;
+ Interface interface;
+ Properties properties;
+ std::string psuPath = msg.get_path();
+
+ msg.read(interface, properties);
+ onPsuInventoryChanged(psuPath, properties);
}
catch (const std::exception& e)
{
@@ -390,6 +394,22 @@
}
}
+void ItemUpdater::onPsuInventoryChanged(const std::string& psuPath,
+ const Properties& properties)
+{
+ if (psuStatusMap.contains(psuPath) && properties.contains(PRESENT))
+ {
+ psuStatusMap[psuPath].present = std::get<bool>(properties.at(PRESENT));
+ handlePSUPresenceChanged(psuPath);
+ if (psuStatusMap[psuPath].present)
+ {
+ // Check if there are new PSU images to update
+ processStoredImage();
+ syncToLatestImage();
+ }
+ }
+}
+
void ItemUpdater::processPSUImage()
{
try
@@ -419,102 +439,158 @@
void ItemUpdater::processStoredImage()
{
- scanDirectory(IMG_DIR_BUILTIN);
-
+ // Build list of directories to scan
+ std::vector<fs::path> paths;
+ paths.emplace_back(IMG_DIR_BUILTIN);
if (!ALWAYS_USE_BUILTIN_IMG_DIR)
{
- scanDirectory(IMG_DIR_PERSIST);
+ paths.emplace_back(IMG_DIR_PERSIST);
+ }
+
+ // Scan directories
+ auto logMsg = "Unable to find PSU firmware in directory {PATH}: {ERROR}";
+ for (const auto& path : paths)
+ {
+ try
+ {
+ scanDirectory(path);
+ }
+ catch (const RuntimeWarning& r)
+ {
+ lg2::warning(logMsg, "PATH", path, "ERROR", r);
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error(logMsg, "PATH", path, "ERROR", e);
+ }
}
}
void ItemUpdater::scanDirectory(const fs::path& dir)
{
- auto manifest = dir;
- auto path = dir;
- // The directory shall put PSU images in directories named with model
- if (!fs::exists(dir))
+ // Find the model subdirectory within the specified directory
+ auto modelDir = findModelDirectory(dir);
+ if (modelDir.empty())
{
- // Skip
- return;
- }
- if (!fs::is_directory(dir))
- {
- lg2::error("The path is not a directory: {PATH}", "PATH", dir);
return;
}
+ // Verify a manifest file exists within the model subdirectory
+ auto manifest = modelDir / MANIFEST_FILE;
+ if (!fs::exists(manifest))
+ {
+ throw std::runtime_error{
+ std::format("Manifest file does not exist: {}", manifest.c_str())};
+ }
+ if (!fs::is_regular_file(manifest))
+ {
+ throw std::runtime_error{
+ std::format("Path is not a file: {}", manifest.c_str())};
+ }
+
+ // Get version, extVersion, and model from manifest file
+ auto ret = Version::getValues(
+ manifest.string(), {MANIFEST_VERSION, MANIFEST_EXTENDED_VERSION});
+ auto version = ret[MANIFEST_VERSION];
+ auto extVersion = ret[MANIFEST_EXTENDED_VERSION];
+ auto info = Version::getExtVersionInfo(extVersion);
+ auto model = info["model"];
+
+ // Verify version and model are valid
+ if (version.empty() || model.empty())
+ {
+ throw std::runtime_error{std::format(
+ "Invalid information in manifest: path={}, version={}, model={}",
+ manifest.c_str(), version, model)};
+ }
+
+ // Verify model from manifest matches the subdirectory name
+ if (modelDir.stem() != model)
+ {
+ throw std::runtime_error{std::format(
+ "Model in manifest does not match path: model={}, path={}", model,
+ modelDir.c_str())};
+ }
+
+ // Found a valid PSU image directory; write path to journal
+ lg2::info("Found PSU firmware image directory: {PATH}", "PATH", modelDir);
+
+ // Calculate version ID and check if an Activation for it exists
+ auto versionId = utils::getVersionId(version);
+ auto it = activations.find(versionId);
+ if (it == activations.end())
+ {
+ // This is a version that is different than the running PSUs
+ auto activationState = Activation::Status::Ready;
+ auto purpose = VersionPurpose::PSU;
+ auto objPath = std::string(SOFTWARE_OBJPATH) + "/" + versionId;
+
+ auto activation = createActivationObject(objPath, versionId, extVersion,
+ activationState, {}, modelDir);
+ activations.emplace(versionId, std::move(activation));
+
+ auto versionPtr =
+ createVersionObject(objPath, versionId, version, purpose);
+ versions.emplace(versionId, std::move(versionPtr));
+ }
+ else
+ {
+ // This is a version that a running PSU is using, set the path
+ // on the version object
+ it->second->path(modelDir);
+ }
+}
+
+fs::path ItemUpdater::findModelDirectory(const fs::path& dir)
+{
+ fs::path modelDir;
+
+ // Verify directory path exists and is a directory
+ if (!fs::exists(dir))
+ {
+ // Warning condition. IMG_DIR_BUILTIN might not be used. IMG_DIR_PERSIST
+ // might not exist if an image from IMG_DIR has not been stored.
+ throw RuntimeWarning{
+ std::format("Directory does not exist: {}", dir.c_str())};
+ }
+ if (!fs::is_directory(dir))
+ {
+ throw std::runtime_error{
+ std::format("Path is not a directory: {}", dir.c_str())};
+ }
+
+ // Get the model name of the PSUs that have been found. Note that we
+ // might not have found the PSU information yet on D-Bus.
+ std::string model;
for (const auto& [key, item] : psuStatusMap)
{
if (!item.model.empty())
{
- path = path / item.model;
- manifest = dir / item.model / MANIFEST_FILE;
+ model = item.model;
break;
}
}
- if (path == dir)
+ if (!model.empty())
{
- lg2::error("Model directory not found");
- return;
- }
-
- if (!fs::is_directory(path))
- {
- lg2::error("The path is not a directory: {PATH}", "PATH", path);
- return;
- }
-
- if (!fs::exists(manifest))
- {
- lg2::error("No MANIFEST found at {PATH}", "PATH", manifest);
- return;
- }
- // If the model in manifest does not match the dir name
- // Log a warning
- if (fs::is_regular_file(manifest))
- {
- auto ret = Version::getValues(
- manifest.string(), {MANIFEST_VERSION, MANIFEST_EXTENDED_VERSION});
- auto version = ret[MANIFEST_VERSION];
- auto extVersion = ret[MANIFEST_EXTENDED_VERSION];
- auto info = Version::getExtVersionInfo(extVersion);
- auto model = info["model"];
- if (path.stem() != model)
+ // Verify model subdirectory path exists and is a directory
+ auto subDir = dir / model;
+ if (!fs::exists(subDir))
{
- lg2::error("Unmatched model: path={PATH}, model={MODEL}", "PATH",
- path, "MODEL", model);
+ // Warning condition. Subdirectory may not exist in IMG_DIR_PERSIST
+ // if no image has been stored there. May also not exist if
+ // firmware update is not supported for this PSU model.
+ throw RuntimeWarning{
+ std::format("Directory does not exist: {}", subDir.c_str())};
}
- else
+ if (!fs::is_directory(subDir))
{
- auto versionId = utils::getVersionId(version);
- auto it = activations.find(versionId);
- if (it == activations.end())
- {
- // This is a version that is different than the running PSUs
- auto activationState = Activation::Status::Ready;
- auto purpose = VersionPurpose::PSU;
- auto objPath = std::string(SOFTWARE_OBJPATH) + "/" + versionId;
-
- auto activation = createActivationObject(
- objPath, versionId, extVersion, activationState, {}, path);
- activations.emplace(versionId, std::move(activation));
-
- auto versionPtr =
- createVersionObject(objPath, versionId, version, purpose);
- versions.emplace(versionId, std::move(versionPtr));
- }
- else
- {
- // This is a version that a running PSU is using, set the path
- // on the version object
- it->second->path(path);
- }
+ throw std::runtime_error{
+ std::format("Path is not a directory: {}", subDir.c_str())};
}
+ modelDir = subDir;
}
- else
- {
- lg2::error("MANIFEST is not a file: {PATH}", "PATH", manifest);
- }
+
+ return modelDir;
}
std::optional<std::string> ItemUpdater::getLatestVersionId()
@@ -542,7 +618,11 @@
break;
}
}
- assert(versionId.has_value());
+ if (!versionId.has_value())
+ {
+ lg2::error("Unable to find versionId for latest version {VERSION}",
+ "VERSION", latestVersion);
+ }
return versionId;
}
@@ -554,7 +634,13 @@
return;
}
const auto& it = activations.find(*latestVersionId);
- assert(it != activations.end());
+ if (it == activations.end())
+
+ {
+ lg2::error("Unable to find Activation for versionId {VERSION_ID}",
+ "VERSION_ID", *latestVersionId);
+ return;
+ }
const auto& activation = it->second;
const auto& assocs = activation->associations();
@@ -568,7 +654,7 @@
{
if (!utils::isAssociated(p, assocs))
{
- lg2::info("Automatically update PSUs to version {VERSION_ID}",
+ lg2::info("Automatically update PSUs to versionId {VERSION_ID}",
"VERSION_ID", *latestVersionId);
invokeActivation(activation);
break;
@@ -583,7 +669,7 @@
activation->requestedActivation(Activation::RequestedActivations::Active);
}
-void ItemUpdater::onPSUInterfaceAdded(sdbusplus::message_t& msg)
+void ItemUpdater::onPSUInterfacesAdded(sdbusplus::message_t& msg)
{
// Maintain static set of valid PSU paths. This is needed if PSU interface
// comes in a separate InterfacesAdded message from Item interface.
@@ -592,9 +678,7 @@
try
{
sdbusplus::message::object_path objPath;
- std::map<std::string,
- std::map<std::string, std::variant<bool, std::string>>>
- interfaces;
+ InterfacesAddedMap interfaces;
msg.read(objPath, interfaces);
std::string path = objPath.str;
diff --git a/src/item_updater.hpp b/src/item_updater.hpp
index e8a489c..e4d7b99 100644
--- a/src/item_updater.hpp
+++ b/src/item_updater.hpp
@@ -14,6 +14,10 @@
#include <xyz/openbmc_project/Collection/DeleteAll/server.hpp>
#include <filesystem>
+#include <map>
+#include <string>
+#include <variant>
+#include <vector>
class TestItemUpdater;
@@ -54,14 +58,14 @@
versionMatch(
bus,
MatchRules::interfacesAdded() + MatchRules::path(SOFTWARE_OBJPATH),
- std::bind(std::mem_fn(&ItemUpdater::createActivation), this,
- std::placeholders::_1)),
+ std::bind(std::mem_fn(&ItemUpdater::onVersionInterfacesAddedMsg),
+ this, std::placeholders::_1)),
psuInterfaceMatch(
bus,
MatchRules::interfacesAdded() +
MatchRules::path("/xyz/openbmc_project/inventory") +
MatchRules::sender("xyz.openbmc_project.Inventory.Manager"),
- std::bind(std::mem_fn(&ItemUpdater::onPSUInterfaceAdded), this,
+ std::bind(std::mem_fn(&ItemUpdater::onPSUInterfacesAdded), this,
std::placeholders::_1))
{
processPSUImageAndSyncToLatest();
@@ -109,25 +113,37 @@
const std::string& psuInventoryPath) override;
private:
+ using Properties =
+ std::map<std::string, utils::UtilsInterface::PropertyType>;
+ using InterfacesAddedMap =
+ std::map<std::string,
+ std::map<std::string, std::variant<bool, std::string>>>;
+
/** @brief Callback function for Software.Version match.
- * @details Creates an Activation D-Bus object.
*
* @param[in] msg - Data associated with subscribed signal
*/
- void createActivation(sdbusplus::message_t& msg);
+ void onVersionInterfacesAddedMsg(sdbusplus::message_t& msg);
- using Properties =
- std::map<std::string, utils::UtilsInterface::PropertyType>;
+ /** @brief Called when new Software.Version interfaces are found
+ * @details Creates an Activation D-Bus object if appropriate
+ * Throws an exception if an error occurs.
+ *
+ * @param[in] path - D-Bus object path
+ * @param[in] interfaces - D-Bus interfaces that were added
+ */
+ void onVersionInterfacesAdded(const std::string& path,
+ const InterfacesAddedMap& interfaces);
/** @brief Callback function for PSU inventory match.
- * @details Update an Activation D-Bus object for PSU inventory.
*
* @param[in] msg - Data associated with subscribed signal
*/
void onPsuInventoryChangedMsg(sdbusplus::message_t& msg);
- /** @brief Callback function for PSU inventory match.
+ /** @brief Called when a PSU inventory object has changed
* @details Update an Activation D-Bus object for PSU inventory.
+ * Throws an exception if an error occurs.
*
* @param[in] psuPath - The PSU inventory path
* @param[in] properties - The updated properties
@@ -186,8 +202,21 @@
/** @brief Create PSU Version from stored images */
void processStoredImage();
- /** @brief Scan a directory and create PSU Version from stored images */
- void scanDirectory(const fs::path& p);
+ /** @brief Scan a directory and create PSU Version from stored images
+ * @details Throws an exception if an error occurs
+ *
+ * @param[in] dir Directory path to scan
+ */
+ void scanDirectory(const fs::path& dir);
+
+ /** @brief Find the PSU model subdirectory within the specified directory
+ * @details Throws an exception if an error occurs
+ *
+ * @param[in] dir Directory path to search
+ *
+ * @return Subdirectory path, or an empty path if none found
+ */
+ fs::path findModelDirectory(const fs::path& dir);
/** @brief Get the versionId of the latest PSU version */
std::optional<std::string> getLatestVersionId();
@@ -200,12 +229,12 @@
/** @brief Callback function for interfaces added signal.
*
- * This method is called when a new interface is added. It updates the
- * internal status map and process the new PSU if it's present.
+ * This method is called when new interfaces are added. It updates the
+ * internal status map and processes the new PSU if it's present.
*
* @param[in] msg - Data associated with subscribed signal
*/
- void onPSUInterfaceAdded(sdbusplus::message_t& msg);
+ void onPSUInterfacesAdded(sdbusplus::message_t& msg);
/**
* @brief Handles the processing of PSU images.
diff --git a/src/runtime_warning.hpp b/src/runtime_warning.hpp
new file mode 100644
index 0000000..78db37c
--- /dev/null
+++ b/src/runtime_warning.hpp
@@ -0,0 +1,45 @@
+#pragma once
+
+#include <exception>
+#include <string>
+
+namespace phosphor::software::updater
+{
+
+/**
+ * @class RuntimeWarning
+ *
+ * Exception class to report a runtime warning condition.
+ */
+class RuntimeWarning : public std::exception
+{
+ public:
+ // Specify which compiler-generated methods we want
+ RuntimeWarning() = delete;
+ RuntimeWarning(const RuntimeWarning&) = default;
+ RuntimeWarning(RuntimeWarning&&) = default;
+ RuntimeWarning& operator=(const RuntimeWarning&) = default;
+ RuntimeWarning& operator=(RuntimeWarning&&) = default;
+ ~RuntimeWarning() override = default;
+
+ /** @brief Constructor.
+ *
+ * @param error error message
+ */
+ explicit RuntimeWarning(const std::string& error) : error{error} {}
+
+ /** @brief Returns the description of this error.
+ *
+ * @return error description
+ */
+ const char* what() const noexcept override
+ {
+ return error.c_str();
+ }
+
+ private:
+ /** @brief Error message */
+ std::string error;
+};
+
+} // namespace phosphor::software::updater
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
diff --git a/src/utils.hpp b/src/utils.hpp
index 020d528..2ce64f4 100644
--- a/src/utils.hpp
+++ b/src/utils.hpp
@@ -24,12 +24,22 @@
const UtilsInterface& getUtils();
/**
- * @brief Get PSU inventory object path from DBus
+ * @brief Get PSU inventory object paths from DBus
+ *
+ * @details The returned vector will be empty if an error occurs or no paths are
+ * found.
+ *
+ * @param[in] bus - The Dbus bus object
+ *
+ * @return PSU inventory object paths that were found (if any)
*/
std::vector<std::string> getPSUInventoryPaths(sdbusplus::bus_t& bus);
/** @brief Get service name from object path and interface
*
+ * @details Throws an exception if an error occurs or no service name was
+ * found.
+ *
* @param[in] bus - The Dbus bus object
* @param[in] path - The Dbus object path
* @param[in] interface - The Dbus interface
@@ -39,19 +49,24 @@
std::string getService(sdbusplus::bus_t& bus, const char* path,
const char* interface);
-/** @brief Get all the service names from object path and interface
+/** @brief Get all service names from object path and interface
+ *
+ * @details The returned vector will be empty if no service names were found.
+ * Throws an exception if an error occurs.
*
* @param[in] bus - The Dbus bus object
* @param[in] path - The Dbus object path
* @param[in] interface - The Dbus interface
*
- * @return The name of the services
+ * @return The name of the services (if any)
*/
std::vector<std::string> getServices(sdbusplus::bus_t& bus, const char* path,
const char* interface);
/** @brief The template function to get property from the requested dbus path
*
+ * @details Throws an exception if an error occurs
+ *
* @param[in] bus - The Dbus bus object
* @param[in] service - The Dbus service name
* @param[in] path - The Dbus object path
@@ -96,7 +111,8 @@
*
* @param[in] versions - The list of the versions
*
- * @return The latest version string
+ * @return The latest version string, or empty string if it fails to get the
+ * latest version
*/
std::string getLatestVersion(const std::set<std::string>& versions);
diff --git a/test/test_item_updater.cpp b/test/test_item_updater.cpp
index 7523bfc..02a8409 100644
--- a/test/test_item_updater.cpp
+++ b/test/test_item_updater.cpp
@@ -478,7 +478,7 @@
// The valid image in test/psu-images-one-valid-one-invalid/model-1/
auto objPathValid = getObjPath("psu-test.v0.4");
auto objPathInvalid = getObjPath("psu-test.v0.5");
- // activation and version object will be added on scan dir
+ // No activation or version objects will be added on scan dir
EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPathValid)))
.Times(0);
EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPathInvalid)))
@@ -502,7 +502,7 @@
_, StrEq(PRESENT)))
.WillOnce(Return(any(PropertyType(true)))); // present
EXPECT_CALL(mockedUtils, getModel(StrEq(psuPath)))
- .WillOnce(Return(std::string("dummyModel")));
+ .WillOnce(Return(std::string("model-3")));
// The item updater itself
EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(dBusPath)))
@@ -522,7 +522,7 @@
_, StrEq(objPath),
StrEq("xyz.openbmc_project.Common.FilePath"),
Pointee(StrEq("Path"))))
- .Times(0);
+ .Times(1);
scanDirectory("./psu-images-valid-version0");
}