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>
diff --git a/activation.cpp b/activation.cpp
index cd973b5..40acc89 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 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 @@
ubiVolumesCreated = false;
Activation::unsubscribeFromSystemdSignals();
+ // Remove version object from image manager
+ Activation::deleteImageManagerObject();
// Create active association
parent.createActiveAssociation(path);
}
@@ -167,6 +172,49 @@
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 @@
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 286c85d..ed077ae 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 @@
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 @@
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 @@
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 @@
*/
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 @@
/** @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 @@
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 60ea7ce..03ff20b 100644
--- a/item_updater.cpp
+++ b/item_updater.cpp
@@ -131,19 +131,20 @@
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 @@
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 @@
}
// 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 @@
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 @@
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 4f78144..5b30065 100755
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -28,4 +28,11 @@
-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 5dbab35..c8fe7cf 100644
--- a/version.cpp
+++ b/version.cpp
@@ -93,6 +93,46 @@
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 8cb7e37..13b42ea 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 @@
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 @@
*
* @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 @@
}
/**
+ * @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 @@
* @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