bmc: delete if open sessions bails
Handle case where one tries to delete any blob while a blob is open.
Signed-off-by: Patrick Venture <venture@google.com>
Change-Id: I4407d46f7a87c42f7d2738668e72e58a847b60af
diff --git a/firmware_handler.cpp b/firmware_handler.cpp
index 64279a2..a402327 100644
--- a/firmware_handler.cpp
+++ b/firmware_handler.cpp
@@ -97,51 +97,25 @@
/*
* Per the design, this mean abort, and this will trigger whatever
* appropriate actions are required to abort the process.
- *
- * You cannot delete a blob that has an open handle in the system, therefore
- * this is never called if there's an open session. Guaranteed by the blob
- * manager.
*/
bool FirmwareBlobHandler::deleteBlob(const std::string& path)
{
- const std::string* toDelete;
-
- /* You cannot delete the verify blob -- trying to delete it, currently has
- * no impact.
- * TODO: Should trying to delete this cause an abort?
+ /* This cannot be called if you have an open session to the path.
+ * You can have an open session to verify/update/hash/image, but not active*
+ *
+ * Therefore, if this is called, it's either on a blob that isn't presently
+ * open. However, there could be open blobs, so we need to close all open
+ * sessions. This closing on our is an invalid handler behavior. Therefore,
+ * we cannot close an active session. To enforce this, we only allow
+ * deleting if there isn't a file open.
*/
- if (path == verifyBlobId)
+ if (fileOpen)
{
return false;
}
- if (path == hashBlobId || path == activeHashBlobId)
- {
- /* They're deleting the hash. */
- toDelete = &activeHashBlobId;
- }
- else
- {
- /* They're deleting the image. */
- toDelete = &activeImageBlobId;
- }
-
- auto it = std::find_if(
- blobIDs.begin(), blobIDs.end(),
- [toDelete](const auto& iter) { return (iter == *toDelete); });
- if (it == blobIDs.end())
- {
- /* Somehow they've asked to delete something we didn't say we could
- * handle.
- */
- return false;
- }
-
- blobIDs.erase(it);
-
- /* TODO: Handle aborting the process and fixing up the state. */
-
- return true;
+ /* TODO: implement. */
+ return false;
}
/*
diff --git a/test/Makefile.am b/test/Makefile.am
index ca85bb9..bd628ec 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -29,7 +29,6 @@
firmware_write_unittest \
firmware_writemeta_unittest \
firmware_close_unittest \
- firmware_delete_unittest \
firmware_sessionstat_unittest \
firmware_commit_unittest \
file_handler_unittest \
@@ -76,9 +75,6 @@
firmware_close_unittest_SOURCES = firmware_close_unittest.cpp
firmware_close_unittest_LDADD = $(top_builddir)/libfirmwareblob_common.la
-firmware_delete_unittest_SOURCES = firmware_delete_unittest.cpp
-firmware_delete_unittest_LDADD = $(top_builddir)/libfirmwareblob_common.la
-
firmware_sessionstat_unittest_SOURCES = firmware_sessionstat_unittest.cpp
firmware_sessionstat_unittest_LDADD = $(top_builddir)/libfirmwareblob_common.la
diff --git a/test/firmware_delete_unittest.cpp b/test/firmware_delete_unittest.cpp
deleted file mode 100644
index 058295d..0000000
--- a/test/firmware_delete_unittest.cpp
+++ /dev/null
@@ -1,60 +0,0 @@
-#include "data_mock.hpp"
-#include "firmware_handler.hpp"
-#include "firmware_unittest.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
-
-#include <vector>
-
-#include <gmock/gmock.h>
-#include <gtest/gtest.h>
-
-namespace ipmi_flash
-{
-namespace
-{
-
-using ::testing::Eq;
-using ::testing::Return;
-using ::testing::StrEq;
-
-class FirmwareHandlerDeleteTest : public FakeLpcFirmwareTest
-{
-};
-
-TEST_F(FirmwareHandlerDeleteTest, DeleteActiveHashSucceeds)
-{
- /* Delete active image succeeds. */
- EXPECT_CALL(imageMock, open(StrEq(hashBlobId))).WillOnce(Return(true));
-
- EXPECT_TRUE(handler->open(
- 0, blobs::OpenFlags::write | FirmwareBlobHandler::UpdateFlags::ipmi,
- hashBlobId));
-
- /* The active hash blob_id was added. */
- auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(3, currentBlobs.size());
- EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
- activeHashBlobId));
-
- /* Set up close() expectations. */
- EXPECT_CALL(imageMock, close());
- EXPECT_TRUE(handler->close(0));
-
- currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
- EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
- activeHashBlobId));
-
- /* Delete the blob. */
- EXPECT_TRUE(handler->deleteBlob(activeHashBlobId));
-
- currentBlobs = handler->getBlobIds();
- EXPECT_EQ(3, currentBlobs.size());
- EXPECT_EQ(0, std::count(currentBlobs.begin(), currentBlobs.end(),
- activeHashBlobId));
-}
-
-} // namespace
-} // namespace ipmi_flash
diff --git a/test/firmware_state_updatecompleted_unittest.cpp b/test/firmware_state_updatecompleted_unittest.cpp
index 3424e89..923b6d0 100644
--- a/test/firmware_state_updatecompleted_unittest.cpp
+++ b/test/firmware_state_updatecompleted_unittest.cpp
@@ -232,9 +232,22 @@
}
/*
- * There are the following calls (parameters may vary):
- * TODO: deleteBlob(blob)
+ * deleteBlob(blob)
*/
+TEST_F(FirmwareHandlerUpdateCompletedTest, DeleteBlobReturnsFalse)
+{
+ /* Try deleting all blobs, it doesn't really matter which though because you
+ * cannot close out an open session, therefore you must fail to delete
+ * anything unless everything is closed.
+ */
+ getToUpdateCompleted(ActionStatus::success);
+
+ auto blobs = handler->getBlobIds();
+ for (const auto& b : blobs)
+ {
+ EXPECT_FALSE(handler->deleteBlob(b));
+ }
+}
} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_state_updatestarted_unittest.cpp b/test/firmware_state_updatestarted_unittest.cpp
index 51a3c20..e8bd103 100644
--- a/test/firmware_state_updatestarted_unittest.cpp
+++ b/test/firmware_state_updatestarted_unittest.cpp
@@ -69,8 +69,21 @@
}
/*
- * TODO: deleteBlob(blob)
+ * deleteBlob(blob)
*/
+TEST_F(FirmwareHandlerUpdateStartedTest, DeleteBlobReturnsFalse)
+{
+ /* Try deleting all blobs, it doesn't really matter which though because you
+ * cannot close out an open session, therefore you must fail to delete
+ * anything unless everything is closed.
+ */
+ getToUpdateStarted();
+ auto blobs = handler->getBlobIds();
+ for (const auto& b : blobs)
+ {
+ EXPECT_FALSE(handler->deleteBlob(b));
+ }
+}
/*
* stat(blob)
diff --git a/test/firmware_state_uploadinprogress_unittest.cpp b/test/firmware_state_uploadinprogress_unittest.cpp
index 5ed5491..c45861d 100644
--- a/test/firmware_state_uploadinprogress_unittest.cpp
+++ b/test/firmware_state_uploadinprogress_unittest.cpp
@@ -269,5 +269,22 @@
EXPECT_FALSE(handler->commit(session, {}));
}
+/*
+ * deleteBlob(blob)
+ */
+TEST_F(FirmwareHandlerUploadInProgressTest, DeleteBlobReturnsFalse)
+{
+ /* Try deleting all blobs, it doesn't really matter which though because you
+ * cannot close out an open session, therefore you must fail to delete
+ * anything unless everything is closed.
+ */
+ openToInProgress(staticLayoutBlobId);
+ auto blobs = handler->getBlobIds();
+ for (const auto& b : blobs)
+ {
+ EXPECT_FALSE(handler->deleteBlob(b));
+ }
+}
+
} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_state_verificationcompleted_unittest.cpp b/test/firmware_state_verificationcompleted_unittest.cpp
index d9b4e70..ac3539d 100644
--- a/test/firmware_state_verificationcompleted_unittest.cpp
+++ b/test/firmware_state_verificationcompleted_unittest.cpp
@@ -47,7 +47,22 @@
{
};
-/* TODO: deleteBlob(blob) */
+/*
+ * deleteBlob(blob)
+ */
+TEST_F(FirmwareHandlerVerificationCompletedTest, DeleteBlobReturnsFalse)
+{
+ /* Try deleting all blobs, it doesn't really matter which though because you
+ * cannot close out an open session, therefore you must fail to delete
+ * anything unless everything is closed.
+ */
+ getToVerificationCompleted(ActionStatus::success);
+ auto blobs = handler->getBlobIds();
+ for (const auto& b : blobs)
+ {
+ EXPECT_FALSE(handler->deleteBlob(b));
+ }
+}
/*
* canHandleBlob
diff --git a/test/firmware_state_verificationstarted_unittest.cpp b/test/firmware_state_verificationstarted_unittest.cpp
index ba76503..e14bee2 100644
--- a/test/firmware_state_verificationstarted_unittest.cpp
+++ b/test/firmware_state_verificationstarted_unittest.cpp
@@ -146,7 +146,22 @@
* updateBlobId.
*/
-/* TODO: deleteBlob(blob) */
+/*
+ * deleteBlob(blob)
+ */
+TEST_F(FirmwareHandlerVerificationStartedTest, DeleteBlobReturnsFalse)
+{
+ /* Try deleting all blobs, it doesn't really matter which though because you
+ * cannot close out an open session, therefore you must fail to delete
+ * anything unless everything is closed.
+ */
+ getToVerificationStarted(staticLayoutBlobId);
+ auto blobs = handler->getBlobIds();
+ for (const auto& b : blobs)
+ {
+ EXPECT_FALSE(handler->deleteBlob(b));
+ }
+}
/*
* stat(blob)