From f268c4679e391e213c36d2046f0d95b858f41054 Mon Sep 17 00:00:00 2001
From: Vernon Mauery <vernon.mauery@intel.com>
Date: Tue, 29 Sep 2020 13:38:35 -0700
Subject: [PATCH] Fix delete image by ID and inhibit removal of bmc_active

Delete image by ID was broken because when hitting the delete dbus
interface, it recalculated the ID from the parent version, which then
does not match because of the random number addition that was added
to the ID when the parent interface was created. This saves away the
parent interface ID and recalls it rather than recalculating it.

Also, there was a logic error in deleting images that would delete the
active BMC image. This fixes up that error.

Tested: run multiple back-to back updates and see that when the fwupd
        script calls delete on the seamless images, the interfaces are
        deleted and that the bmc_active interface is not deleted.

Signed-off-by: Vernon Mauery <vernon.mauery@intel.com>
Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com>
---
 item_updater.cpp      | 17 +++++++++++------
 pfr_image_manager.cpp |  2 +-
 version.cpp           |  2 +-
 version.hpp           | 19 +++++++++++++++----
 4 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/item_updater.cpp b/item_updater.cpp
index 8d7bb82..5bf8b87 100644
--- a/item_updater.cpp
+++ b/item_updater.cpp
@@ -132,7 +132,7 @@ void ItemUpdater::createActivation(sdbusplus::message::message& msg)
                                          activationState, associations)));
 
         auto versionPtr = std::make_unique<VersionClass>(
-            bus, path, version, purpose, filePath,
+            bus, path, versionId, version, purpose, filePath,
             std::bind(&ItemUpdater::erase, this, std::placeholders::_1));
         versionPtr->deleteObject =
             std::make_unique<phosphor::software::manager::Delete>(bus, path,
@@ -224,7 +224,7 @@ void ItemUpdater::processBMCImage()
 
             // Create Version instance for this version.
             auto versionPtr = std::make_unique<VersionClass>(
-                bus, path, version, purpose, "",
+                bus, path, id, version, purpose, "",
                 std::bind(&ItemUpdater::erase, this, std::placeholders::_1));
             auto isVersionFunctional = versionPtr->isFunctional();
             if (!isVersionFunctional)
@@ -298,11 +298,11 @@ void ItemUpdater::erase(std::string entryId)
     auto it = versions.find(entryId);
     if (it != versions.end())
     {
-        if (it->second->isFunctional() && ACTIVE_BMC_MAX_ALLOWED > 1)
+        if (it->second->isFunctional())
         {
-            log<level::ERR>("Error: Version is currently running on the BMC. "
-                            "Unable to remove.",
-                            entry("VERSIONID=%s", entryId.c_str()));
+            log<level::INFO>("Error: Version is currently running on the BMC. "
+                             "Unable to remove.",
+                             entry("VERSIONID=%s", entryId.c_str()));
             return;
         }
 
@@ -633,6 +633,11 @@ void ItemUpdater::freeSpace(Activation& caller)
     std::size_t count = 0;
     for (const auto& iter : activations)
     {
+        if (versions.find(iter.second->versionId)->second->isFunctional())
+        {
+            // don't bother with function versions
+            continue;
+        }
         if ((iter.second.get()->activation() ==
              server::Activation::Activations::Active) ||
             (iter.second.get()->activation() ==
diff --git a/pfr_image_manager.cpp b/pfr_image_manager.cpp
index ba73cc1..36f0a62 100644
--- a/pfr_image_manager.cpp
+++ b/pfr_image_manager.cpp
@@ -265,7 +265,7 @@ int Manager::processImage(const std::string& imgFilePath)
     std::string objPath = std::string{SOFTWARE_OBJPATH} + '/' + id;
 
     auto versionPtr = std::make_unique<Version>(
-        bus, objPath, ver, purpose, imageDirPath.string(),
+        bus, objPath, id, ver, purpose, imageDirPath.string(),
         std::bind(&Manager::erase, this, std::placeholders::_1));
     versionPtr->deleteObject =
         std::make_unique<phosphor::software::manager::Delete>(bus, objPath,
diff --git a/version.cpp b/version.cpp
index f50500a..1471233 100644
--- a/version.cpp
+++ b/version.cpp
@@ -181,7 +181,7 @@ void Delete::delete_()
 {
     if (parent.eraseCallback)
     {
-        parent.eraseCallback(parent.getId(parent.version()));
+        parent.eraseCallback(parent.getExtId());
     }
 }
 
diff --git a/version.hpp b/version.hpp
index 5827c8a..ad1170a 100644
--- a/version.hpp
+++ b/version.hpp
@@ -73,14 +73,15 @@ class Version : public VersionInherit
      * @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, eraseFunc callback) :
+            const std::string& extId, const std::string& versionString,
+            VersionPurpose versionPurpose, const std::string& filePath,
+            eraseFunc callback) :
         VersionInherit(bus, (objPath).c_str(), true),
-        eraseCallback(callback), versionStr(versionString)
+        eraseCallback(callback), versionStr(versionString), extId(extId)
     {
         // Set properties.
         purpose(versionPurpose);
-        version(versionString);
+        version(versionStr);
         path(filePath);
         // Emit deferred signal.
         emit_object_added();
@@ -133,6 +134,15 @@ class Version : public VersionInherit
      */
     bool isFunctional();
 
+    /* @brief Return the extended ID of this version object
+     *
+     * @ return - returns the extended ID string
+     */
+    std::string getExtId()
+    {
+        return extId;
+    }
+
     /** @brief Persistent Delete D-Bus object */
     std::unique_ptr<Delete> deleteObject;
 
@@ -142,6 +152,7 @@ class Version : public VersionInherit
   private:
     /** @brief This Version's version string */
     const std::string versionStr;
+    const std::string extId;
 };
 
 } // namespace manager
-- 
2.7.4