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>

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