BMC: Fix the delete implementation.
- Implement delete interface inside version class
so that both item_updater and image_manager can
share the same interface. This meant removing the
delete interface from inside the activation class.
- The delete is created as a separate object inside
version, only if the image is non-functional.
This helps remove the delete interface from a
running BMC/HOST image.
- As part of the activation process, the version from
inside the image_manager is deleted and so is the
version's tarfile from the image upload dir.
Partially resolves openbmc/openbmc#2490
Change-Id: Ib35bf188df85ebd2277d3d9ad04300e434965eea
Signed-off-by: Saqib Khan <khansa@us.ibm.com>
diff --git a/activation.cpp b/activation.cpp
index 9859cd6..d4ca885 100644
--- a/activation.cpp
+++ b/activation.cpp
@@ -4,7 +4,6 @@
#include "serialize.hpp"
#include <phosphor-logging/log.hpp>
-
namespace phosphor
{
namespace software
@@ -38,11 +37,6 @@
return;
}
-void Delete::delete_()
-{
- parent.parent.erase(parent.versionId);
-}
-
auto Activation::activation(Activations value) ->
Activations
{
@@ -113,6 +107,9 @@
roVolumeCreated = false;
Activation::unsubscribeFromSystemdSignals();
+ // Remove version object from image manager
+ Activation::deleteImageManagerObject();
+
// Create active association
parent.createActiveAssociation(path);
@@ -128,6 +125,47 @@
return softwareServer::Activation::activation(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;
+ }
+}
+
auto Activation::requestedActivation(RequestedActivations value) ->
RequestedActivations
{
diff --git a/activation.hpp b/activation.hpp
index a6e1111..9c1fde9 100644
--- a/activation.hpp
+++ b/activation.hpp
@@ -6,7 +6,6 @@
#include "xyz/openbmc_project/Software/RedundancyPriority/server.hpp"
#include "xyz/openbmc_project/Software/ActivationProgress/server.hpp"
#include "org/openbmc/Associations/server.hpp"
-#include "xyz/openbmc_project/Object/Delete/server.hpp"
namespace phosphor
{
@@ -26,8 +25,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;
@@ -186,55 +183,6 @@
std::string path;
};
-/** @class ActivationDelete
- * @brief OpenBMC Delete implementation.
- * @details A concrete implementation for xyz.openbmc_project.Object.Delete
- * DBus API.
- */
-class Delete : public DeleteInherit
-{
- public:
- /** @brief Constructs Delete.
- *
- * @param[in] bus - The Dbus bus object
- * @param[in] path - The Dbus object path
- * @param[in] parent - Parent object.
- */
- // Delete(sdbusplus::bus::bus& bus, const std::string& path) :
- 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:
- // 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 Activation
* @brief OpenBMC activation software management implementation.
* @details A concrete implementation for
@@ -332,6 +280,12 @@
*/
void unsubscribeFromSystemdSignals();
+ /**
+ * @brief Deletes the version from Image Manager and the
+ * untar image from image upload dir.
+ */
+ void deleteImageManagerObject();
+
/** @brief Persistent sdbusplus DBus bus connection */
sdbusplus::bus::bus& bus;
@@ -353,9 +307,6 @@
/** @brief Persistent ActivationProgress dbus object */
std::unique_ptr<ActivationProgress> activationProgress;
- /** @brief Persistent Delete dbus object */
- std::unique_ptr<Delete> deleteObject;
-
/** @brief Used to subscribe to dbus systemd signals **/
sdbusplus::bus::match_t systemdSignals;
diff --git a/image_manager.cpp b/image_manager.cpp
index 43edded..4a27260 100644
--- a/image_manager.cpp
+++ b/image_manager.cpp
@@ -162,21 +162,25 @@
if (versions.find(id) == versions.end())
{
- this->versions.insert(std::make_pair(
- id,
- std::make_unique<Version>(
- this->bus,
- objPath,
- version,
- purpose,
- imageDirPath.string())));
+ auto versionPtr = std::make_unique<Version>(
+ bus,
+ objPath,
+ version,
+ purpose,
+ imageDirPath.string(),
+ std::bind(&Manager::erase,
+ this,
+ std::placeholders::_1));
+ versionPtr->deleteObject =
+ std::make_unique<phosphor::software::manager::Delete>(
+ bus, objPath, *versionPtr);
+ versions.insert(std::make_pair(id, std::move(versionPtr)));
}
else
{
log<level::INFO>("Software Object with the same version already exists",
entry("VERSION_ID=%s", id));
}
-
return 0;
}
@@ -205,29 +209,6 @@
this->versions.erase(entryId);
}
-void Manager::removeVersion(sdbusplus::message::message& msg)
-{
- namespace mesg = sdbusplus::message;
-
- mesg::object_path objPath;
-
- msg.read(objPath);
- std::string path(std::move(objPath));
-
- // Version id is the last item in the path
- auto pos = path.rfind("/");
- if (pos == std::string::npos)
- {
- log<level::INFO>("No version id found in object path",
- entry("OBJPATH=%s", path));
- return;
- }
-
- auto versionId = path.substr(pos + 1);
-
- erase(versionId);
-}
-
int Manager::unTar(const std::string& tarFilePath,
const std::string& extractDirPath)
{
diff --git a/image_manager.hpp b/image_manager.hpp
index 5c69536..4938823 100644
--- a/image_manager.hpp
+++ b/image_manager.hpp
@@ -9,8 +9,6 @@
namespace manager
{
-namespace MatchRules = sdbusplus::bus::match::rules;
-
/** @class Manager
* @brief Contains a map of Version dbus objects.
* @details The software image manager class that contains the Version dbus
@@ -23,16 +21,7 @@
*
* @param[in] bus - The Dbus bus object
*/
- Manager(sdbusplus::bus::bus& bus) :
- bus(bus),
- versionMatch(
- bus,
- MatchRules::interfacesRemoved() +
- MatchRules::path(SOFTWARE_OBJPATH),
- std::bind(
- std::mem_fn(&Manager::removeVersion),
- this,
- std::placeholders::_1)){};
+ Manager(sdbusplus::bus::bus& bus) : bus(bus){};
/**
* @brief Verify and untar the tarball. Verify the manifest file.
@@ -52,13 +41,6 @@
void erase(std::string entryId);
private:
- /** @brief Callback function for Software.Version match.
- * @details Removes a version object.
- *
- * @param[in] msg - Data associated with subscribed signal
- */
- void removeVersion(sdbusplus::message::message& msg);
-
/** @brief Persistent map of Version dbus objects and their
* version id */
std::map<std::string, std::unique_ptr<Version>> versions;
@@ -66,9 +48,6 @@
/** @brief Persistent sdbusplus DBus bus connection. */
sdbusplus::bus::bus& bus;
- /** @brief sdbusplus signal match for Software.Version */
- sdbusplus::bus::match_t versionMatch;
-
/**
* @brief Untar the tarball.
*
diff --git a/item_updater.cpp b/item_updater.cpp
index 151986f..bbfdef4 100644
--- a/item_updater.cpp
+++ b/item_updater.cpp
@@ -120,27 +120,29 @@
bmcInventoryPath));
}
- auto activationPtr = std::make_unique<Activation>(
- bus,
- path,
- *this,
- versionId,
- activationState,
- associations);
+ activations.insert(std::make_pair(
+ versionId,
+ std::make_unique<Activation>(
+ bus,
+ path,
+ *this,
+ versionId,
+ activationState,
+ associations)));
- activationPtr->deleteObject =
- std::make_unique<Delete>(bus, path, *activationPtr);
-
- activations.insert(std::make_pair(versionId, std::move(activationPtr)));
-
- versions.insert(std::make_pair(
- versionId,
- std::make_unique<VersionClass>(
- bus,
- path,
- version,
- purpose,
- filePath)));
+ auto versionPtr = std::make_unique<VersionClass>(
+ bus,
+ path,
+ version,
+ purpose,
+ filePath,
+ std::bind(&ItemUpdater::erase,
+ this,
+ std::placeholders::_1));
+ versionPtr->deleteObject =
+ std::make_unique<phosphor::software::manager::Delete>(
+ bus, path, *versionPtr);
+ versions.insert(std::make_pair(versionId, std::move(versionPtr)));
}
return;
}
@@ -206,33 +208,35 @@
// Create Version instance for this version.
auto versionPtr = std::make_unique<VersionClass>(
- bus,
- path,
- version,
- purpose,
- "");
+ bus,
+ path,
+ version,
+ purpose,
+ "",
+ std::bind(&ItemUpdater::erase,
+ this,
+ std::placeholders::_1));
auto isVersionFunctional = versionPtr->isFunctional();
- versions.insert(std::make_pair(
- id,
- std::move(versionPtr)));
-
- // Create Activation instance for this version.
- auto activationPtr = std::make_unique<Activation>(
- bus,
- path,
- *this,
- id,
- activationState,
- associations);
-
- // Add Delete() if this isn't the functional version
if (!isVersionFunctional)
{
- activationPtr->deleteObject =
- std::make_unique<Delete>(bus, path, *activationPtr);
+ versionPtr->deleteObject =
+ std::make_unique<phosphor::software::manager::Delete>(
+ bus, path, *versionPtr);
}
+ versions.insert(std::make_pair(
+ id,
+ std::move(versionPtr)));
- activations.insert(std::make_pair(id, std::move(activationPtr)));
+ // Create Activation instance for this version.
+ activations.insert(std::make_pair(
+ id,
+ std::make_unique<Activation>(
+ bus,
+ path,
+ *this,
+ id,
+ activationState,
+ associations)));
// If Active, create RedundancyPriority instance for this version.
if (activationState == server::Activation::Activations::Active)
@@ -303,6 +307,9 @@
// Delete ReadOnly partitions if it's not active
removeReadOnlyPartition(entryId);
removeFile(entryId);
+
+ // Removing entry in versions map
+ this->versions.erase(entryId);
}
else
{
@@ -313,7 +320,6 @@
log<level::ERR>(("Error: Failed to find version " + entryId + \
" in item updater versions map." \
" Unable to remove.").c_str());
- return;
}
// Remove the priority environment variable.
@@ -326,9 +332,6 @@
method.append(serviceFile, "replace");
bus.call_noreply(method);
- // Removing entry in versions map
- this->versions.erase(entryId);
-
// Removing entry in activations map
auto ita = activations.find(entryId);
if (ita == activations.end())
@@ -336,30 +339,25 @@
log<level::ERR>(("Error: Failed to find version " + entryId + \
" in item updater activations map." \
" Unable to remove.").c_str());
- return;
}
-
- this->activations.erase(entryId);
+ else
+ {
+ this->activations.erase(entryId);
+ }
ItemUpdater::resetUbootEnvVars();
+ return;
}
void ItemUpdater::deleteAll()
{
- std::vector<std::string> deletableVersions;
-
for (const auto& versionIt : versions)
{
if (!versionIt.second->isFunctional())
{
- deletableVersions.push_back(versionIt.first);
+ ItemUpdater::erase(versionIt.first);
}
}
- for (const auto& deletableIt : deletableVersions)
- {
- ItemUpdater::erase(deletableIt);
- }
-
// Remove any volumes that do not match current versions.
auto method = bus.new_method_call(
SYSTEMD_BUSNAME,
diff --git a/version.cpp b/version.cpp
index 6377afb..724ffea 100644
--- a/version.cpp
+++ b/version.cpp
@@ -124,6 +124,14 @@
return versionStr == getBMCVersion(OS_RELEASE_FILE);
}
+void Delete::delete_()
+{
+ if (parent.eraseCallback)
+ {
+ parent.eraseCallback(parent.getId(parent.version()));
+ }
+}
+
} // namespace manager
} // namespace software
} // namepsace phosphor
diff --git a/version.hpp b/version.hpp
index 99c64c9..b7efe44 100644
--- a/version.hpp
+++ b/version.hpp
@@ -3,6 +3,7 @@
#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 <functional>
namespace phosphor
@@ -17,6 +18,58 @@
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>;
+
+class Version;
+class Delete;
+
+/** @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. */
+ 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.
@@ -33,15 +86,19 @@
* @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,
const std::string& versionString,
VersionPurpose versionPurpose,
- const std::string& filePath) : VersionInherit(
+ const std::string& filePath,
+ eraseFunc callback) : VersionInherit(
bus, (objPath).c_str(), true),
versionStr(versionString)
{
+ // Bind erase method
+ eraseCallback = callback;
// Set properties.
purpose(versionPurpose);
version(versionString);
@@ -87,7 +144,14 @@
*/
bool isFunctional();
+ /** @brief Persistent Delete D-Bus object */
+ std::unique_ptr<Delete> deleteObject;
+
+ /** @brief The parent's erase callback. */
+ eraseFunc eraseCallback;
+
private:
+
/** @brief This Version's version string */
const std::string versionStr;
};