version_handler: Don't store session information immediately
This can cause spurious trigger aborts when open's fail and cleanup is
called on the failed session.
Change-Id: I1c49f248b6f56bbecc0d51fafaabe871d23b1d7d
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/bmc/version-handler/test/version_close_unittest.cpp b/bmc/version-handler/test/version_close_unittest.cpp
index 83b4a43..c529206 100644
--- a/bmc/version-handler/test/version_close_unittest.cpp
+++ b/bmc/version-handler/test/version_close_unittest.cpp
@@ -31,9 +31,8 @@
TEST_F(VersionCloseExpireBlobTest, VerifyOpenThenClose)
{
EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
- EXPECT_CALL(*tm.at("blob0"), status())
- .WillOnce(Return(ActionStatus::success));
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->close(0));
}
@@ -45,9 +44,8 @@
TEST_F(VersionCloseExpireBlobTest, VerifyDoubleCloseFails)
{
EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
- EXPECT_CALL(*tm.at("blob0"), status())
- .WillOnce(Return(ActionStatus::success));
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->close(0));
EXPECT_FALSE(h->close(0));
}
@@ -55,20 +53,17 @@
TEST_F(VersionCloseExpireBlobTest, VerifyBadSessionNumberCloseFails)
{
EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
- EXPECT_CALL(*tm.at("blob0"), status())
- .WillOnce(Return(ActionStatus::success));
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
EXPECT_FALSE(h->close(1));
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->close(0));
}
TEST_F(VersionCloseExpireBlobTest, VerifyRunningActionIsAborted)
{
EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
- EXPECT_CALL(*tm.at("blob0"), status())
- .WillOnce(Return(ActionStatus::running));
- EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->close(0));
}
diff --git a/bmc/version-handler/test/version_open_unittest.cpp b/bmc/version-handler/test/version_open_unittest.cpp
index 0e80a94..033b2f4 100644
--- a/bmc/version-handler/test/version_open_unittest.cpp
+++ b/bmc/version-handler/test/version_open_unittest.cpp
@@ -21,11 +21,6 @@
{
h = std::make_unique<VersionBlobHandler>(
createMockVersionConfigs(blobNames, &im, &tm));
- for (const auto& blob : blobNames)
- {
- EXPECT_CALL(*tm.at(blob), status())
- .WillRepeatedly(Return(ActionStatus::unknown));
- }
}
std::unique_ptr<blobs::GenericBlobInterface> h;
@@ -43,10 +38,10 @@
TEST_F(VersionOpenBlobTest, VerifyMultipleBlobOpens)
{
- for (const auto& [key, val] : tm)
+ for (const auto& [_, val] : tm)
{
/* set the expectation that every onOpen will be triggered */
- EXPECT_CALL(*val, trigger()).Times(1).WillOnce(Return(true));
+ EXPECT_CALL(*val, trigger()).WillOnce(Return(true));
}
int i{defaultSessionNumber};
for (const auto& blob : blobNames)
@@ -57,37 +52,36 @@
TEST_F(VersionOpenBlobTest, VerifyOpenAfterClose)
{
- EXPECT_CALL(*tm.at("blob0"), trigger())
- .Times(2)
- .WillRepeatedly(Return(true));
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
+
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->close(defaultSessionNumber));
+
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
}
TEST_F(VersionOpenBlobTest, VerifyDoubleOpenFails)
{
- EXPECT_CALL(*tm.at("blob1"), trigger())
- .Times(1)
- .WillRepeatedly(Return(true));
+ EXPECT_CALL(*tm.at("blob1"), trigger()).WillOnce(Return(true));
EXPECT_TRUE(h->open(0, blobs::read, "blob1"));
EXPECT_FALSE(h->open(2, blobs::read, "blob1"));
+ EXPECT_FALSE(h->open(2, blobs::read, "blob1"));
}
TEST_F(VersionOpenBlobTest, VerifyFailedTriggerFails)
{
- EXPECT_CALL(*tm.at("blob1"), trigger())
- .Times(2)
- .WillOnce(Return(false))
- .WillOnce(Return(true));
+ EXPECT_CALL(*tm.at("blob1"), trigger()).WillOnce(Return(false));
EXPECT_FALSE(h->open(0, blobs::read, "blob1"));
+ EXPECT_CALL(*tm.at("blob1"), trigger()).WillOnce(Return(true));
EXPECT_TRUE(h->open(0, blobs::read, "blob1"));
}
TEST_F(VersionOpenBlobTest, VerifyUnsupportedOpenFlagsFails)
{
- EXPECT_CALL(*tm.at("blob1"), trigger()).Times(1).WillOnce(Return(true));
EXPECT_FALSE(h->open(0, blobs::write, "blob1"));
+ EXPECT_CALL(*tm.at("blob1"), trigger()).WillOnce(Return(true));
EXPECT_TRUE(h->open(0, blobs::read, "blob1"));
}
diff --git a/bmc/version-handler/version_handler.cpp b/bmc/version-handler/version_handler.cpp
index 5d66713..9918e33 100644
--- a/bmc/version-handler/version_handler.cpp
+++ b/bmc/version-handler/version_handler.cpp
@@ -70,22 +70,20 @@
}
auto& v = *blobInfoMap.at(path);
- sessionToBlob[session] = &v;
-
if (v.blobState == blobs::StateFlags::open_read)
{
fprintf(stderr, "open %s fail: blob already opened for read\n",
path.c_str());
- cleanup(session);
return false;
}
if (v.actions->onOpen->trigger() == false)
{
fprintf(stderr, "open %s fail: onOpen trigger failed\n", path.c_str());
- cleanup(session);
return false;
}
+
v.blobState = blobs::StateFlags::open_read;
+ sessionToBlob[session] = &v;
return true;
}
@@ -147,10 +145,7 @@
try
{
auto& pack = *sessionToBlob.at(session);
- if (pack.actions->onOpen->status() == ActionStatus::running)
- {
- pack.actions->onOpen->abort();
- }
+ pack.actions->onOpen->abort();
pack.blobState = static_cast<blobs::StateFlags>(0);
sessionToBlob.erase(session);
return true;