bmc: add verify blob id only when ready
Originally, the verify blob id was always present. Now, it's only added
when the state transitions initially to verificationPending.
Signed-off-by: Patrick Venture <venture@google.com>
Change-Id: I706273519354a7a4a98d9fe4f600c6455a69cc3c
diff --git a/firmware_handler.cpp b/firmware_handler.cpp
index 6a317a7..ded5383 100644
--- a/firmware_handler.cpp
+++ b/firmware_handler.cpp
@@ -57,7 +57,6 @@
{
blobs.push_back(item.blobName);
}
- blobs.push_back(verifyBlobId); /* Add blob_id to always exist. */
if (0 == std::count(blobs.begin(), blobs.end(), hashBlobId))
{
@@ -442,6 +441,7 @@
lookup[session] = curr;
addBlobId(*active);
+ removeBlobId(verifyBlobId);
state = UpdateState::uploadInProgress;
fileOpen = true;
@@ -606,10 +606,8 @@
state = UpdateState::notYetStarted;
/* A this point, delete the active blob IDs from the blob_list. */
- blobIDs.erase(
- std::remove(blobIDs.begin(), blobIDs.end(), activeImageBlobId));
- blobIDs.erase(
- std::remove(blobIDs.begin(), blobIDs.end(), activeHashBlobId));
+ removeBlobId(activeImageBlobId);
+ removeBlobId(activeHashBlobId);
}
/* Must be verificationPending... not yet started, they may re-open and
* trigger verification.
@@ -619,6 +617,9 @@
{
/* They are closing a data pathway (image, tarball, hash). */
state = UpdateState::verificationPending;
+
+ /* Add verify blob ID now that we can expect it. */
+ addBlobId(verifyBlobId);
}
if (item->second->dataHandler)
diff --git a/firmware_handler.hpp b/firmware_handler.hpp
index 3f2d21d..0b58876 100644
--- a/firmware_handler.hpp
+++ b/firmware_handler.hpp
@@ -188,6 +188,12 @@
}
}
+ void removeBlobId(const std::string& blob)
+ {
+ blobIDs.erase(std::remove(blobIDs.begin(), blobIDs.end(), blob),
+ blobIDs.end());
+ }
+
/** List of handlers by type. */
std::vector<HandlerPack> handlers;
diff --git a/test/firmware_close_unittest.cpp b/test/firmware_close_unittest.cpp
index 6242d54..a46973f 100644
--- a/test/firmware_close_unittest.cpp
+++ b/test/firmware_close_unittest.cpp
@@ -13,6 +13,9 @@
namespace ipmi_flash
{
+namespace
+{
+
using ::testing::Eq;
using ::testing::Return;
using ::testing::StrEq;
@@ -35,7 +38,7 @@
/* The active hash blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeHashBlobId));
@@ -62,7 +65,7 @@
/* The active hash blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeHashBlobId));
@@ -71,4 +74,5 @@
EXPECT_TRUE(handler->close(0));
}
+} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_delete_unittest.cpp b/test/firmware_delete_unittest.cpp
index 7f36a56..ff2d7b3 100644
--- a/test/firmware_delete_unittest.cpp
+++ b/test/firmware_delete_unittest.cpp
@@ -13,6 +13,9 @@
namespace ipmi_flash
{
+namespace
+{
+
using ::testing::Eq;
using ::testing::Return;
using ::testing::StrEq;
@@ -32,7 +35,7 @@
/* The active hash blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeHashBlobId));
@@ -54,4 +57,5 @@
activeHashBlobId));
}
+} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_handler_unittest.cpp b/test/firmware_handler_unittest.cpp
index e1e3a78..e2ef7f4 100644
--- a/test/firmware_handler_unittest.cpp
+++ b/test/firmware_handler_unittest.cpp
@@ -11,6 +11,10 @@
namespace ipmi_flash
{
+namespace
+{
+
+using ::testing::UnorderedElementsAreArray;
TEST(FirmwareHandlerTest, CreateEmptyListVerifyFails)
{
@@ -56,9 +60,9 @@
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
blobs, data, CreateVerifyMock(), CreateUpdateMock());
auto result = handler->getBlobIds();
- EXPECT_EQ(3, result.size());
- EXPECT_EQ(3, std::count(result.begin(), result.end(), "asdf") +
- std::count(result.begin(), result.end(), hashBlobId) +
- std::count(result.begin(), result.end(), verifyBlobId));
+ std::vector<std::string> expectedBlobs = {"asdf", hashBlobId};
+ EXPECT_THAT(result, UnorderedElementsAreArray(expectedBlobs));
}
+
+} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_open_unittest.cpp b/test/firmware_open_unittest.cpp
index 4da41a1..8190a47 100644
--- a/test/firmware_open_unittest.cpp
+++ b/test/firmware_open_unittest.cpp
@@ -13,6 +13,9 @@
namespace ipmi_flash
{
+namespace
+{
+
using ::testing::Eq;
using ::testing::Return;
using ::testing::StrEq;
@@ -37,7 +40,7 @@
/* The active image blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeImageBlobId));
}
@@ -53,7 +56,7 @@
/* The active hash blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeHashBlobId));
}
@@ -72,7 +75,7 @@
/* The active hash blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeHashBlobId));
}
@@ -86,9 +89,9 @@
0, blobs::OpenFlags::write | FirmwareBlobHandler::UpdateFlags::lpc,
hashBlobId));
- /* The active hash blob_id was added. */
+ /* The active hash blob_id was not added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(3, currentBlobs.size());
+ EXPECT_EQ(2, currentBlobs.size());
}
TEST_F(FirmwareHandlerOpenTestIpmiOnly,
@@ -105,7 +108,7 @@
/* The active image blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeImageBlobId));
@@ -138,7 +141,7 @@
/* The active image blob_id was added. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(4, currentBlobs.size());
+ EXPECT_EQ(3, currentBlobs.size());
EXPECT_EQ(1, std::count(currentBlobs.begin(), currentBlobs.end(),
activeImageBlobId));
@@ -165,7 +168,7 @@
/* Verify blob_id list doesn't grow. */
auto currentBlobs = handler->getBlobIds();
- EXPECT_EQ(3, currentBlobs.size());
+ EXPECT_EQ(2, currentBlobs.size());
}
TEST_F(FirmwareHandlerOpenTestIpmiOnly, OpenWithoutWriteFails)
@@ -197,4 +200,5 @@
/* TODO: The client sends a request to open active image. */
/* TODO: The client sends a request to open active hash. */
+} // namespace
} // namespace ipmi_flash
diff --git a/test/firmware_state_notyetstarted_unittest.cpp b/test/firmware_state_notyetstarted_unittest.cpp
index a97a348..bdfffcb 100644
--- a/test/firmware_state_notyetstarted_unittest.cpp
+++ b/test/firmware_state_notyetstarted_unittest.cpp
@@ -50,8 +50,7 @@
/* TODO: Presently, /flash/verify is present from the beginning, however,
* this is going to change to simplify handling of open().
*/
- std::vector<std::string> expectedBlobs = {staticLayoutBlobId, hashBlobId,
- verifyBlobId};
+ std::vector<std::string> expectedBlobs = {staticLayoutBlobId, hashBlobId};
EXPECT_THAT(handler->getBlobIds(),
UnorderedElementsAreArray(expectedBlobs));
@@ -76,11 +75,8 @@
expected.blobState = FirmwareBlobHandler::UpdateFlags::ipmi;
expected.size = 0;
- /* TODO: note, once verifyblobid isn't present in this state we can use
- * getblobids()'s output as input
- */
- std::vector<std::string> testBlobs = {staticLayoutBlobId, hashBlobId};
- for (const auto& blob : testBlobs)
+ auto blobs = handler->getBlobIds();
+ for (const auto& blob : blobs)
{
blobs::BlobMeta meta = {};
EXPECT_TRUE(handler->stat(blob, &meta));
diff --git a/test/firmware_state_uploadinprogress_unittest.cpp b/test/firmware_state_uploadinprogress_unittest.cpp
index 691a8a6..237fc44 100644
--- a/test/firmware_state_uploadinprogress_unittest.cpp
+++ b/test/firmware_state_uploadinprogress_unittest.cpp
@@ -62,7 +62,7 @@
openToInProgress(staticLayoutBlobId);
std::vector<std::string> expectedAfterImage = {
- staticLayoutBlobId, hashBlobId, verifyBlobId, activeImageBlobId};
+ staticLayoutBlobId, hashBlobId, activeImageBlobId};
EXPECT_THAT(handler->getBlobIds(),
UnorderedElementsAreArray(expectedAfterImage));
}
@@ -73,7 +73,7 @@
openToInProgress(hashBlobId);
std::vector<std::string> expectedAfterImage = {
- staticLayoutBlobId, hashBlobId, verifyBlobId, activeHashBlobId};
+ staticLayoutBlobId, hashBlobId, activeHashBlobId};
EXPECT_THAT(handler->getBlobIds(),
UnorderedElementsAreArray(expectedAfterImage));
}
@@ -91,6 +91,7 @@
* verify blob.
*/
openToInProgress(staticLayoutBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeImageBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeImageBlobId, &meta));
@@ -102,6 +103,7 @@
* change from close.
*/
openToInProgress(hashBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeHashBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeHashBlobId, &meta));
@@ -127,11 +129,6 @@
}
}
-/* TODO: stat(verifyblobid) only should exist once both /image and /hash have
- * closed, so add this test when this is the case. NOTE: /image or /tarball
- * should have the same effect.
- */
-
/*
* stat(session)
*/
@@ -187,8 +184,6 @@
/*
* close(session) - closing the hash or image will trigger a state transition to
* verificationPending.
- * TODO: This state transition should add the verifyBlobId. This will test that
- * it's there, but this test doesn't verify that it only just now appeared.
*
* NOTE: Re-opening /flash/image will transition back to uploadInProgress, but
* that is verified in the verificationPending::open tests.
@@ -198,8 +193,7 @@
{
auto realHandler = dynamic_cast<FirmwareBlobHandler*>(handler.get());
- /* TODO: uncomment this when verify is properly added. */
- // EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
+ EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
EXPECT_CALL(imageMock, open(staticLayoutBlobId)).WillOnce(Return(true));
@@ -219,8 +213,7 @@
{
auto realHandler = dynamic_cast<FirmwareBlobHandler*>(handler.get());
- /* TODO: uncomment this when verify is properly added. */
- // EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
+ EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
EXPECT_CALL(imageMock, open(hashBlobId)).WillOnce(Return(true));
diff --git a/test/firmware_state_verificationpending_unittest.cpp b/test/firmware_state_verificationpending_unittest.cpp
index af777c7..92b6a2a 100644
--- a/test/firmware_state_verificationpending_unittest.cpp
+++ b/test/firmware_state_verificationpending_unittest.cpp
@@ -8,6 +8,7 @@
#include "status.hpp"
#include "util.hpp"
+#include <algorithm>
#include <cstdint>
#include <string>
#include <vector>
@@ -70,9 +71,10 @@
/* Only in the verificationPending state (and later), should the
* verifyBlobId be present. */
- /* TODO: Add this test in when the change is made. */
- // EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
+ EXPECT_FALSE(handler->canHandleBlob(verifyBlobId));
+
getToVerificationPending(staticLayoutBlobId);
+
EXPECT_TRUE(handler->canHandleBlob(verifyBlobId));
EXPECT_TRUE(handler->canHandleBlob(activeImageBlobId));
}
@@ -87,6 +89,7 @@
TEST_F(FirmwareHandlerVerificationPendingTest, StatOnActiveImageReturnsFailure)
{
getToVerificationPending(staticLayoutBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeImageBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeImageBlobId, &meta));
@@ -95,6 +98,7 @@
TEST_F(FirmwareHandlerVerificationPendingTest, StatOnActiveHashReturnsFailure)
{
getToVerificationPending(hashBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeHashBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeHashBlobId, &meta));
@@ -104,6 +108,7 @@
StatOnVerificationBlobReturnsFailure)
{
getToVerificationPending(hashBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeHashBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(verifyBlobId, &meta));
@@ -120,6 +125,7 @@
std::vector<std::string> testBlobs = {staticLayoutBlobId, hashBlobId};
for (const auto& blob : testBlobs)
{
+ ASSERT_TRUE(handler->canHandleBlob(blob));
blobs::BlobMeta meta = {};
EXPECT_TRUE(handler->stat(blob, &meta));
EXPECT_EQ(expected, meta);
@@ -168,7 +174,12 @@
EXPECT_EQ(FirmwareBlobHandler::UpdateState::uploadInProgress,
realHandler->getCurrentState());
- /* Verify the active blob ID was not added to the list twice. */
+ expectedBlobs.erase(
+ std::remove(expectedBlobs.begin(), expectedBlobs.end(), verifyBlobId));
+
+ /* Verify the active blob ID was not added to the list twice and
+ * verifyBlobId is removed
+ */
EXPECT_THAT(handler->getBlobIds(),
UnorderedElementsAreArray(expectedBlobs));
}
diff --git a/test/firmware_state_verificationstarted_unittest.cpp b/test/firmware_state_verificationstarted_unittest.cpp
index a1a4cce..e27ffc7 100644
--- a/test/firmware_state_verificationstarted_unittest.cpp
+++ b/test/firmware_state_verificationstarted_unittest.cpp
@@ -153,6 +153,7 @@
TEST_F(FirmwareHandlerVerificationStartedTest, StatOnActiveImageReturnsFailure)
{
getToVerificationStarted(staticLayoutBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeImageBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeImageBlobId, &meta));
@@ -160,7 +161,8 @@
TEST_F(FirmwareHandlerVerificationStartedTest, StatOnActiveHashReturnsFailure)
{
- getToVerificationStarted(staticLayoutBlobId);
+ getToVerificationStarted(hashBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(activeHashBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(activeHashBlobId, &meta));
@@ -170,6 +172,7 @@
{
/* the verifyBlobId is available starting at verificationPending. */
getToVerificationStarted(staticLayoutBlobId);
+ ASSERT_TRUE(handler->canHandleBlob(verifyBlobId));
blobs::BlobMeta meta;
EXPECT_FALSE(handler->stat(verifyBlobId, &meta));
@@ -186,6 +189,8 @@
std::vector<std::string> testBlobs = {staticLayoutBlobId, hashBlobId};
for (const auto& blob : testBlobs)
{
+ ASSERT_TRUE(handler->canHandleBlob(blob));
+
blobs::BlobMeta meta = {};
EXPECT_TRUE(handler->stat(blob, &meta));
EXPECT_EQ(expected, meta);