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>

7 files changed, 193 insertions, 174 deletions

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 @@ void Activation::unsubscribeFromSystemdSignals()
     return;
 }
 
-void Delete::delete_()
-{
-    parent.parent.erase(parent.versionId);
-}
-
 auto Activation::activation(Activations value) ->
         Activations
 {
@@ -113,6 +107,9 @@ auto Activation::activation(Activations value) ->
             roVolumeCreated = false;
             Activation::unsubscribeFromSystemdSignals();
 
+            // Remove version object from image manager
+            Activation::deleteImageManagerObject();
+
             // Create active association
             parent.createActiveAssociation(path);
 
@@ -128,6 +125,47 @@ auto Activation::activation(Activations value) ->
     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 @@ 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;
 
@@ -186,55 +183,6 @@ class ActivationProgress : public ActivationProgressInherit
         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 @@ class Activation : public ActivationInherit
          */
         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 @@ class Activation : public ActivationInherit
         /** @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 @@ int Manager::processImage(const std::string& tarFilePath)
 
     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 @@ void Manager::erase(std::string entryId)
     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 software
 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 @@ class Manager
          *
          * @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 @@ class Manager
         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 @@ class Manager
         /** @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 @@ void ItemUpdater::createActivation(sdbusplus::message::message& msg)
                                               bmcInventoryPath));
         }
 
-        auto activationPtr = 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)));
+        activations.insert(std::make_pair(
+                                   versionId,
+                                   std::make_unique<Activation>(
+                                           bus,
+                                           path,
+                                           *this,
+                                           versionId,
+                                           activationState,
+                                           associations)));
+
+        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 @@ void ItemUpdater::processBMCImage()
 
             // 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 @@ void ItemUpdater::erase(std::string entryId)
         // 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 @@ 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;
     }
 
     // Remove the priority environment variable.
@@ -326,9 +332,6 @@ void ItemUpdater::erase(std::string entryId)
     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 @@ 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;
     }
-
-    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 @@ bool Version::isFunctional()
     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 @@ 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>;
+
+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 @@ class Version : public VersionInherit
          * @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 @@ class Version : public VersionInherit
          */
         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;
 };