diff options
author | Saqib Khan <khansa@us.ibm.com> | 2017-10-22 11:29:07 -0500 |
---|---|---|
committer | Saqib Khan <khansa@us.ibm.com> | 2017-12-05 16:55:30 -0600 |
commit | 7f80e0b534b91b3a868f33934d92024427235e89 (patch) | |
tree | df60b8ca6f4f5faf8257c6fa49fe0d1257ff3908 | |
parent | 5b75651b350476455d3c2b6f948a273b1438a790 (diff) | |
download | openpower-pnor-code-mgmt-7f80e0b534b91b3a868f33934d92024427235e89.tar.gz openpower-pnor-code-mgmt-7f80e0b534b91b3a868f33934d92024427235e89.zip |
PNOR: Fix the delete implementation
- In order to remove the delete object from functional
image, the delete interface is moved inside the
version class so that both item_updater and image_manager
can make use of the same implementation.
- To avoid having two delete objects attached to the same
HOST version (item_updater and image_manager), we are now
deleting the image_manager object once the activation
is complete.
Partially resolves openbmc/openbmc#2490
Change-Id: Ie515cc01d5f154e6e55b9a3fb71d831730cd46f6
Signed-off-by: Saqib Khan <khansa@us.ibm.com>
-rwxr-xr-x | activation.cpp | 88 | ||||
-rwxr-xr-x | activation.hpp | 87 | ||||
-rw-r--r-- | item_updater.cpp | 76 | ||||
-rwxr-xr-x | test/Makefile.am | 9 | ||||
-rw-r--r-- | version.cpp | 40 | ||||
-rw-r--r-- | version.hpp | 126 |
6 files changed, 267 insertions, 159 deletions
diff --git a/activation.cpp b/activation.cpp index cd973b5c0..40acc894d 100755 --- a/activation.cpp +++ b/activation.cpp @@ -3,6 +3,7 @@ #include "config.h" #include "item_updater.hpp" #include "serialize.hpp" +#include <phosphor-logging/log.hpp> namespace openpower { @@ -14,6 +15,8 @@ namespace updater namespace fs = std::experimental::filesystem; namespace softwareServer = sdbusplus::xyz::openbmc_project::Software::server; +using namespace phosphor::logging; + constexpr auto SYSTEMD_SERVICE = "org.freedesktop.systemd1"; constexpr auto SYSTEMD_OBJ_PATH = "/org/freedesktop/systemd1"; @@ -89,6 +92,8 @@ void Activation::finishActivation() ubiVolumesCreated = false; Activation::unsubscribeFromSystemdSignals(); + // Remove version object from image manager + Activation::deleteImageManagerObject(); // Create active association parent.createActiveAssociation(path); } @@ -167,6 +172,49 @@ auto Activation::requestedActivation(RequestedActivations value) -> return softwareServer::Activation::requestedActivation(value); } +void Activation::deleteImageManagerObject() +{ + // Get the Delete object for <versionID> inside image_manager + auto method = this->bus.new_method_call(MAPPER_BUSNAME, + MAPPER_PATH, + MAPPER_INTERFACE, + "GetObject"); + + method.append(path); + method.append(std::vector<std::string>({ + "xyz.openbmc_project.Object.Delete"})); + auto mapperResponseMsg = bus.call(method); + if (mapperResponseMsg.is_method_error()) + { + log<level::ERR>("Error in Get Delete Object", + entry("VERSIONPATH=%s", path)); + return; + } + std::map<std::string, std::vector<std::string>> mapperResponse; + mapperResponseMsg.read(mapperResponse); + if (mapperResponse.begin() == mapperResponse.end()) + { + log<level::ERR>("ERROR in reading the mapper response", + entry("VERSIONPATH=%s", path)); + return; + } + + // Call the Delete object for <versionID> inside image_manager + method = this->bus.new_method_call((mapperResponse.begin()->first).c_str(), + path.c_str(), + "xyz.openbmc_project.Object.Delete", + "Delete"); + mapperResponseMsg = bus.call(method); + + //Check that the bus call didn't result in an error + if (mapperResponseMsg.is_method_error()) + { + log<level::ERR>("Error in Deleting image from image manager", + entry("VERSIONPATH=%s", path)); + return; + } +} + uint8_t RedundancyPriority::priority(uint8_t value) { parent.parent.freePriority(value, parent.versionId); @@ -208,46 +256,6 @@ void Activation::unitStateChange(sdbusplus::message::message& msg) return; } -void Activation::updateDeleteInterface(sdbusplus::message::message& msg) -{ - std::string interface, chassisState; - std::map<std::string, sdbusplus::message::variant<std::string>> properties; - - msg.read(interface, properties); - - for (const auto& p : properties) - { - if (p.first == "CurrentPowerState") - { - chassisState = p.second.get<std::string>(); - } - } - - if ((parent.isVersionFunctional(this->versionId)) && - (chassisState != CHASSIS_STATE_OFF)) - { - if (deleteObject) - { - deleteObject.reset(nullptr); - } - } - else - { - if (!deleteObject) - { - deleteObject = std::make_unique<Delete>(bus, path, *this); - } - } -} - -void Delete::delete_() -{ - // Remove active association - parent.parent.removeActiveAssociation(parent.path); - - parent.parent.erase(parent.versionId); -} - } // namespace updater } // namespace software } // namespace openpower diff --git a/activation.hpp b/activation.hpp index 286c85dda..ed077ae37 100755 --- a/activation.hpp +++ b/activation.hpp @@ -6,9 +6,7 @@ #include "xyz/openbmc_project/Software/ExtendedVersion/server.hpp" #include "xyz/openbmc_project/Software/RedundancyPriority/server.hpp" #include "xyz/openbmc_project/Software/ActivationProgress/server.hpp" -#include "xyz/openbmc_project/Object/Delete/server.hpp" #include "org/openbmc/Associations/server.hpp" -#include "config.h" namespace openpower { @@ -29,8 +27,6 @@ using RedundancyPriorityInherit = sdbusplus::server::object::object< sdbusplus::xyz::openbmc_project::Software::server::RedundancyPriority>; using ActivationProgressInherit = sdbusplus::server::object::object< sdbusplus::xyz::openbmc_project::Software::server::ActivationProgress>; -using DeleteInherit = sdbusplus::server::object::object< - sdbusplus::xyz::openbmc_project::Object::server::Delete>; namespace sdbusRule = sdbusplus::bus::match::rules; @@ -170,53 +166,6 @@ class ActivationProgress : public ActivationProgressInherit std::string path; }; -/** @class ActivationDelete - * @brief OpenBMC Delete implementation. - * @details A concrete implementation for xyz.openbmc_project.Object.Delete - * D-Bus API. - */ -class Delete : public DeleteInherit -{ - public: - /** @brief Constructs Delete. - * - * @param[in] bus - The D-Bus bus object - * @param[in] path - The D-Bus object path - * @param[in] parent - Parent object. - */ - Delete(sdbusplus::bus::bus& bus, - const std::string& path, - Activation& parent) : - DeleteInherit(bus, path.c_str(), true), - parent(parent), - bus(bus), - path(path) - { - std::vector<std::string> interfaces({interface}); - bus.emit_interfaces_added(path.c_str(), interfaces); - } - - ~Delete() - { - std::vector<std::string> interfaces({interface}); - bus.emit_interfaces_removed(path.c_str(), interfaces); - } - - /** - * @brief delete the D-Bus object. - */ - void delete_() override; - - /** @brief Parent Object. */ - Activation& parent; - - private: - static constexpr auto interface = - "xyz.openbmc_project.Object.Delete"; - sdbusplus::bus::bus& bus; - std::string path; -}; - /** @class Activation * @brief OpenBMC activation software management implementation. * @details A concrete implementation for @@ -255,17 +204,7 @@ class Activation : public ActivationInherit sdbusRule::interface( "org.freedesktop.systemd1.Manager"), std::bind(std::mem_fn(&Activation::unitStateChange), - this, std::placeholders::_1)), - chassisStateSignals( - bus, - sdbusRule::type::signal() + - sdbusRule::member("PropertiesChanged") + - sdbusRule::path(CHASSIS_STATE_PATH) + - sdbusRule::argN(0, CHASSIS_STATE_OBJ) + - sdbusRule::interface(SYSTEMD_PROPERTY_INTERFACE), - std::bind(std::mem_fn( - &Activation::updateDeleteInterface), this, - std::placeholders::_1)) + this, std::placeholders::_1)) { // Enable systemd signals subscribeToSystemdSignals(); @@ -311,17 +250,6 @@ class Activation : public ActivationInherit */ void unitStateChange(sdbusplus::message::message& msg); - /** @brief Update the Object.Delete interface for this activation - * - * Update the delete interface based on whether or not this activation - * is currently functional. A functional activation will have no - * Object.Delete, while a non-functional activation will have one. - * - * @param[in] msg - Data associated with subscribed signal - * - */ - void updateDeleteInterface(sdbusplus::message::message& msg); - /** * @brief subscribe to the systemd signals * @@ -361,15 +289,9 @@ class Activation : public ActivationInherit /** @brief Persistent RedundancyPriority dbus object */ std::unique_ptr<RedundancyPriority> redundancyPriority; - /** @brief Persistent Delete dbus object */ - std::unique_ptr<Delete> deleteObject; - /** @brief Used to subscribe to dbus systemd signals **/ sdbusplus::bus::match_t systemdSignals; - /** @brief Used to subscribe to chassis power state changes **/ - sdbusplus::bus::match_t chassisStateSignals; - /** @brief Tracks whether the read-only & read-write volumes have been *created as part of the activation process. **/ bool ubiVolumesCreated = false; @@ -381,6 +303,13 @@ class Activation : public ActivationInherit using ActivationInherit::activation; private: + + /** + * @brief Deletes the version from Image Manager and the + * untar image from image upload dir. + */ + void deleteImageManagerObject(); + /** @brief Member function for clarity & brevity at activation start */ void startActivation(); diff --git a/item_updater.cpp b/item_updater.cpp index 60ea7cea9..03ff20b92 100644 --- a/item_updater.cpp +++ b/item_updater.cpp @@ -131,19 +131,20 @@ void ItemUpdater::createActivation(sdbusplus::message::message& m) activationState, associations))); - activations.find(versionId)->second->deleteObject = - std::make_unique<Delete>(bus, - path, - *activations.find(versionId)->second); - - versions.insert(std::make_pair( - versionId, - std::make_unique<Version>( - bus, - path, - version, - purpose, - filePath))); + auto versionPtr = std::make_unique<Version>( + bus, + path, + *this, + versionId, + version, + purpose, + filePath, + std::bind(&ItemUpdater::erase, + this, + std::placeholders::_1)); + versionPtr->deleteObject = + std::make_unique<Delete>(bus, path, *versionPtr); + versions.insert(std::make_pair(versionId, std::move(versionPtr))); } return; } @@ -222,11 +223,6 @@ void ItemUpdater::processPNORImage() activationState, associations))); - activations.find(id)->second->deleteObject = - std::make_unique<Delete>(bus, - path, - *activations.find(id)->second); - // If Active, create RedundancyPriority instance for this version. if (activationState == server::Activation::Activations::Active) { @@ -245,14 +241,22 @@ void ItemUpdater::processPNORImage() } // Create Version instance for this version. + auto versionPtr = std::make_unique<Version>( + bus, + path, + *this, + id, + version, + purpose, + "", + std::bind(&ItemUpdater::erase, + this, + std::placeholders::_1)); + versionPtr->deleteObject = + std::make_unique<Delete>(bus, path, *versionPtr); versions.insert(std::make_pair( - id, - std::make_unique<Version>( - bus, - path, - version, - purpose, - ""))); + id, + std::move(versionPtr))); } else if (0 == iter.path().native().compare(0, PNOR_RW_PREFIX_LEN, PNOR_RW_PREFIX)) @@ -488,9 +492,11 @@ void ItemUpdater::erase(std::string entryId) log<level::ERR>(("Error: Failed to find version " + entryId + \ " in item updater versions map." \ " Unable to remove.").c_str()); - return; } - versions.erase(entryId); + else + { + versions.erase(entryId); + } // Removing entry in activations map auto ita = activations.find(entryId); @@ -499,28 +505,24 @@ void ItemUpdater::erase(std::string entryId) log<level::ERR>(("Error: Failed to find version " + entryId + \ " in item updater activations map." \ " Unable to remove.").c_str()); - return; } - activations.erase(entryId); + else + { + activations.erase(entryId); + } + return; } void ItemUpdater::deleteAll() { - std::vector<std::string> deletableActivations; - for (const auto& activationIt : activations) { if (!isVersionFunctional(activationIt.first)) { - deletableActivations.push_back(activationIt.first); + ItemUpdater::erase(activationIt.first); } } - for (const auto& deletableIt : deletableActivations) - { - ItemUpdater::erase(deletableIt); - } - // Remove any remaining pnor-ro- or pnor-rw- volumes that do not match // the current version. auto method = bus.new_method_call( diff --git a/test/Makefile.am b/test/Makefile.am index 4f78144c6..5b30065b2 100755 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -28,4 +28,11 @@ utest_LDFLAGS = \ -lcrypto utest_SOURCES = utest.cpp -utest_LDADD = $(top_builddir)/openpower_update_manager-version.o +utest_LDADD = \ + $(top_builddir)/openpower_update_manager-activation.o \ + $(top_builddir)/openpower_update_manager-version.o \ + $(top_builddir)/openpower_update_manager-serialize.o \ + $(top_builddir)/openpower_update_manager-watch.o \ + $(top_builddir)/openpower_update_manager-item_updater.o \ + $(top_builddir)/org/openbmc/Associations/openpower_update_manager-server.o \ + -lstdc++fs diff --git a/version.cpp b/version.cpp index 5dbab3508..c8fe7cf95 100644 --- a/version.cpp +++ b/version.cpp @@ -93,6 +93,46 @@ std::map<std::string, std::string> Version::getValue( return keys; } +void Delete::delete_() +{ + if (parent.eraseCallback) + { + parent.eraseCallback(parent.getId(parent.version())); + } +} + +void Version::updateDeleteInterface(sdbusplus::message::message& msg) +{ + std::string interface, chassisState; + std::map<std::string, sdbusplus::message::variant<std::string>> properties; + + msg.read(interface, properties); + + for (const auto& p : properties) + { + if (p.first == "CurrentPowerState") + { + chassisState = p.second.get<std::string>(); + } + } + + if ((parent.isVersionFunctional(this->versionId)) && + (chassisState != CHASSIS_STATE_OFF)) + { + if (deleteObject) + { + deleteObject.reset(nullptr); + } + } + else + { + if (!deleteObject) + { + deleteObject = std::make_unique<Delete>(bus, objPath, *this); + } + } +} + } // namespace updater } // namespace software } // namespace openpower diff --git a/version.hpp b/version.hpp index 8cb7e371d..13b42ea37 100644 --- a/version.hpp +++ b/version.hpp @@ -3,6 +3,8 @@ #include <sdbusplus/bus.hpp> #include "xyz/openbmc_project/Software/Version/server.hpp" #include "xyz/openbmc_project/Common/FilePath/server.hpp" +#include "xyz/openbmc_project/Object/Delete/server.hpp" +#include "config.h" namespace openpower { @@ -13,9 +15,69 @@ namespace updater class ItemUpdater; +typedef std::function<void(std::string)> eraseFunc; + using VersionInherit = sdbusplus::server::object::object< sdbusplus::xyz::openbmc_project::Software::server::Version, sdbusplus::xyz::openbmc_project::Common::server::FilePath>; +using DeleteInherit = sdbusplus::server::object::object< + sdbusplus::xyz::openbmc_project::Object::server::Delete>; + +namespace sdbusRule = sdbusplus::bus::match::rules; + +class Delete; +class Version; + +/** @class Delete + * @brief OpenBMC Delete implementation. + * @details A concrete implementation for xyz.openbmc_project.Object.Delete + * D-Bus API. +*/ +class Delete : public DeleteInherit +{ + public: + /** @brief Constructs Delete. + * + * @param[in] bus - The D-Bus bus object + * @param[in] path - The D-Bus object path + * @param[in] parent - Parent object. + */ + Delete(sdbusplus::bus::bus& bus, + const std::string& path, + Version& parent) : + DeleteInherit(bus, path.c_str(), true), + parent(parent), + bus(bus), + path(path) + { + std::vector<std::string> interfaces({interface}); + bus.emit_interfaces_added(path.c_str(), interfaces); + } + + ~Delete() + { + std::vector<std::string> interfaces({interface}); + bus.emit_interfaces_removed(path.c_str(), interfaces); + } + + /** + * @brief Delete the D-Bus object. + * Overrides the default delete function by calling + * Version class erase Method. + **/ + void delete_() override; + + private: + + /** @brief Parent Object. */ + Version& parent; + + // TODO Remove once openbmc/openbmc#1975 is resolved + static constexpr auto interface = + "xyz.openbmc_project.Object.Delete"; + sdbusplus::bus::bus& bus; + std::string path; +}; /** @class Version * @brief OpenBMC version software management implementation. @@ -29,17 +91,39 @@ class Version : public VersionInherit * * @param[in] bus - The D-Bus bus object * @param[in] objPath - The D-Bus object path + * @param[in] parent - Parent object. + * @param[in] versionId - The version Id * @param[in] versionString - The version string * @param[in] versionPurpose - The version purpose * @param[in] filePath - The image filesystem path + * @param[in] callback - The eraseFunc callback */ Version(sdbusplus::bus::bus& bus, const std::string& objPath, + ItemUpdater& parent, + const std::string& versionId, const std::string& versionString, VersionPurpose versionPurpose, - const std::string& filePath) : - VersionInherit(bus, (objPath).c_str(), true) + const std::string& filePath, eraseFunc callback) : + VersionInherit(bus, (objPath).c_str(), true), + bus(bus), + objPath(objPath), + parent(parent), + versionId(versionId), + versionStr(versionString), + chassisStateSignals( + bus, + sdbusRule::type::signal() + + sdbusRule::member("PropertiesChanged") + + sdbusRule::path(CHASSIS_STATE_PATH) + + sdbusRule::argN(0, CHASSIS_STATE_OBJ) + + sdbusRule::interface(SYSTEMD_PROPERTY_INTERFACE), + std::bind(std::mem_fn( + &Version::updateDeleteInterface), this, + std::placeholders::_1)) { + // Bind erase method + eraseCallback = callback; // Set properties. purpose(versionPurpose); version(versionString); @@ -50,6 +134,18 @@ class Version : public VersionInherit } /** + * @brief Update the Object.Delete interface for this activation + * + * Update the delete interface based on whether or not this + * activation is currently functional. A functional activation + * will have no Object.Delete, while a non-functional activation + * will have one. + * + * @param[in] msg - Data associated with subscribed signal + */ + void updateDeleteInterface(sdbusplus::message::message& msg); + + /** * @brief Read the manifest file to get the value of the key. * * @param[in] filePath - The path to the file which contains the value @@ -73,6 +169,32 @@ class Version : public VersionInherit * @return The id. */ static std::string getId(const std::string& version); + + /** @brief Persistent Delete D-Bus object */ + std::unique_ptr<Delete> deleteObject; + + /** @brief The parent's erase callback. */ + eraseFunc eraseCallback; + + private: + /** @brief Persistent sdbusplus DBus bus connection */ + sdbusplus::bus::bus& bus; + + /** @brief Persistent DBus object path */ + std::string objPath; + + /** @brief Parent Object. */ + ItemUpdater& parent; + + /** @brief This Version's version Id */ + const std::string versionId; + + /** @brief This Version's version string */ + const std::string versionStr; + + /** @brief Used to subscribe to chassis power state changes **/ + sdbusplus::bus::match_t chassisStateSignals; + }; } // namespace updater |