version_handler: Support multiple sessions
We want to be able to support multiple concurrent readers of version
information. Otherwise, upstream version checks might fail if they end
up being sequenced concurrently.
Change-Id: I5420ad667622b7906e633562a5373e0be042c0c1
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 c529206..efdff5d 100644
--- a/bmc/version-handler/test/version_close_unittest.cpp
+++ b/bmc/version-handler/test/version_close_unittest.cpp
@@ -36,6 +36,16 @@
EXPECT_TRUE(h->close(0));
}
+TEST_F(VersionCloseExpireBlobTest, VerifySingleAbort)
+{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
+ EXPECT_TRUE(h->open(1, blobs::read, "blob0"));
+ EXPECT_TRUE(h->close(0));
+ EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
+ EXPECT_TRUE(h->close(1));
+}
+
TEST_F(VersionCloseExpireBlobTest, VerifyUnopenedBlobCloseFails)
{
EXPECT_FALSE(h->close(0));
diff --git a/bmc/version-handler/test/version_open_unittest.cpp b/bmc/version-handler/test/version_open_unittest.cpp
index 033b2f4..3d6f559 100644
--- a/bmc/version-handler/test/version_open_unittest.cpp
+++ b/bmc/version-handler/test/version_open_unittest.cpp
@@ -62,12 +62,12 @@
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
}
-TEST_F(VersionOpenBlobTest, VerifyDoubleOpenFails)
+TEST_F(VersionOpenBlobTest, VerifyMultiOpenWorks)
{
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"));
+ EXPECT_TRUE(h->open(1, blobs::read, "blob1"));
+ EXPECT_TRUE(h->open(2, blobs::read, "blob1"));
}
TEST_F(VersionOpenBlobTest, VerifyFailedTriggerFails)
diff --git a/bmc/version-handler/test/version_read_unittest.cpp b/bmc/version-handler/test/version_read_unittest.cpp
index bee3d11..e621df8 100644
--- a/bmc/version-handler/test/version_read_unittest.cpp
+++ b/bmc/version-handler/test/version_read_unittest.cpp
@@ -3,12 +3,18 @@
#include <memory>
#include <string>
+#include <string_view>
#include <vector>
#include <gtest/gtest.h>
+
using ::testing::_;
+using ::testing::DoAll;
+using ::testing::ElementsAreArray;
+using ::testing::Ge;
using ::testing::IsEmpty;
using ::testing::Return;
+
namespace ipmi_flash
{
@@ -27,64 +33,104 @@
const std::uint16_t defaultSessionNumber{200};
std::vector<uint8_t> vector1{0xDE, 0xAD, 0xBE, 0xEF,
0xBA, 0xDF, 0xEE, 0x0D};
+ std::vector<uint8_t> vector2{0xCE, 0xAD, 0xDE, 0xFF};
};
TEST_F(VersionReadBlobTest, VerifyValidRead)
{
- EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ testing::InSequence seq;
+ EXPECT_CALL(*tm.at("blob0"), trigger())
+ .WillOnce(DoAll([&]() { tm.at("blob0")->cb(*tm.at("blob0")); },
+ Return(true)));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(2)
- .WillRepeatedly(Return(ActionStatus::success));
+ .WillOnce(Return(ActionStatus::success));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
+ EXPECT_CALL(*im.at("blob0"), read(0, Ge(vector1.size())))
+ .WillOnce(Return(vector1));
+ EXPECT_CALL(*im.at("blob0"), close()).Times(1);
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
- /* file path gets bound to file_handler on creation so path parameter
- * doesn't actually matter
- */
- EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in))
- .Times(2)
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*im.at("blob0"), read(0, 10)).WillOnce(Return(vector1));
- EXPECT_CALL(*im.at("blob0"), read(2, 10)).WillOnce(Return(vector1));
- EXPECT_CALL(*im.at("blob0"), close()).Times(2);
- EXPECT_EQ(h->read(defaultSessionNumber, 0, 10), vector1);
- EXPECT_EQ(h->read(defaultSessionNumber, 2, 10), vector1);
+ std::basic_string_view<uint8_t> vectorS(vector1.data(), vector1.size());
+ EXPECT_THAT(h->read(defaultSessionNumber, 0, 7),
+ ElementsAreArray(vectorS.substr(0, 7)));
+ EXPECT_THAT(h->read(defaultSessionNumber, 2, 10),
+ ElementsAreArray(vectorS.substr(2, 6)));
+ EXPECT_THAT(h->read(defaultSessionNumber, 10, 0), IsEmpty());
+}
+
+TEST_F(VersionReadBlobTest, VerifyMultipleSession)
+{
+ testing::InSequence seq;
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
+ EXPECT_TRUE(h->open(1, blobs::read, "blob0"));
+
+ EXPECT_CALL(*tm.at("blob0"), status())
+ .WillOnce(Return(ActionStatus::success));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
+ EXPECT_CALL(*im.at("blob0"), read(0, Ge(vector1.size())))
+ .WillOnce(Return(vector1));
+ EXPECT_CALL(*im.at("blob0"), close()).Times(1);
+ tm.at("blob0")->cb(*tm.at("blob0"));
+
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ EXPECT_TRUE(h->open(2, blobs::read, "blob0"));
+
+ EXPECT_CALL(*tm.at("blob0"), status())
+ .WillOnce(Return(ActionStatus::success));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
+ EXPECT_CALL(*im.at("blob0"), read(0, Ge(vector2.size())))
+ .WillOnce(Return(vector2));
+ EXPECT_CALL(*im.at("blob0"), close()).Times(1);
+ tm.at("blob0")->cb(*tm.at("blob0"));
+
+ EXPECT_THAT(h->read(0, 0, 10), ElementsAreArray(vector1));
+ EXPECT_THAT(h->read(1, 0, 10), ElementsAreArray(vector1));
+ EXPECT_THAT(h->read(2, 0, 10), ElementsAreArray(vector2));
+}
+
+TEST_F(VersionReadBlobTest, VerifyReadEarlyFails)
+{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+
+ EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
+ EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
}
TEST_F(VersionReadBlobTest, VerifyTriggerFailureReadFails)
{
- EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ EXPECT_CALL(*tm.at("blob0"), trigger())
+ .WillOnce(DoAll([&]() { tm.at("blob0")->cb(*tm.at("blob0")); },
+ Return(true)));
EXPECT_CALL(*tm.at("blob0"), status())
.WillOnce(Return(ActionStatus::failed));
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
}
-TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileReadFailure)
+TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileOpenFailure)
{
- EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
+ EXPECT_CALL(*tm.at("blob0"), trigger())
+ .WillOnce(DoAll([&]() { tm.at("blob0")->cb(*tm.at("blob0")); },
+ Return(true)));
EXPECT_CALL(*tm.at("blob0"), status())
.WillOnce(Return(ActionStatus::success));
- /* file path gets bound to file_handler on creation so path parameter
- * doesn't actually matter
- */
- EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
- EXPECT_CALL(*im.at("blob0"), read(_, _)).WillOnce(Return(std::nullopt));
- EXPECT_CALL(*im.at("blob0"), close()).Times(1);
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(false));
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
}
-TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileOpenFailure)
+TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileReadFailure)
{
- EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
- /* first call to trigger status fails, second succeeds */
+ EXPECT_CALL(*tm.at("blob0"), trigger())
+ .WillOnce(DoAll([&]() { tm.at("blob0")->cb(*tm.at("blob0")); },
+ Return(true)));
EXPECT_CALL(*tm.at("blob0"), status())
.WillOnce(Return(ActionStatus::success));
- /* file path gets bound to file_handler on creation so path parameter
- * doesn't actually matter
- */
- EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(false));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
+ EXPECT_CALL(*im.at("blob0"), read(_, _)).WillOnce(Return(std::nullopt));
+ EXPECT_CALL(*im.at("blob0"), close()).Times(1);
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
diff --git a/bmc/version-handler/version_handler.cpp b/bmc/version-handler/version_handler.cpp
index a5a357b..45d9b02 100644
--- a/bmc/version-handler/version_handler.cpp
+++ b/bmc/version-handler/version_handler.cpp
@@ -15,6 +15,41 @@
info->blobId = std::move(config.blobId);
info->actions = std::move(config.actions);
info->handler = std::move(config.handler);
+ info->actions->onOpen->setCallback(
+ [infoP = info.get()](TriggerableActionInterface& tai) {
+ auto data =
+ std::make_shared<std::optional<std::vector<uint8_t>>>();
+ do
+ {
+ if (tai.status() != ActionStatus::success)
+ {
+ fprintf(stderr, "Version file unit failed for %s\n",
+ infoP->blobId.c_str());
+ continue;
+ }
+ if (!infoP->handler->open("", std::ios::in))
+ {
+ fprintf(stderr, "Opening version file failed for %s\n",
+ infoP->blobId.c_str());
+ continue;
+ }
+ auto d = infoP->handler->read(
+ 0, std::numeric_limits<uint32_t>::max());
+ infoP->handler->close();
+ if (!d)
+ {
+ fprintf(stderr, "Reading version file failed for %s\n",
+ infoP->blobId.c_str());
+ continue;
+ }
+ *data = std::move(d);
+ } while (false);
+ for (auto sessionP : infoP->sessionsToUpdate)
+ {
+ sessionP->data = data;
+ }
+ infoP->sessionsToUpdate.clear();
+ });
if (!blobInfoMap.try_emplace(info->blobId, std::move(info)).second)
{
fprintf(stderr, "Ignoring duplicate config for %s\n",
@@ -68,69 +103,50 @@
return false;
}
- auto& v = *blobInfoMap.at(path);
- if (v.blobState == blobs::StateFlags::open_read)
- {
- fprintf(stderr, "open %s fail: blob already opened for read\n",
- path.c_str());
- return false;
- }
- if (v.actions->onOpen->trigger() == false)
+ auto info = std::make_unique<SessionInfo>();
+ info->blob = blobInfoMap.at(path).get();
+ info->blob->sessionsToUpdate.emplace(info.get());
+ if (info->blob->sessionsToUpdate.size() == 1 &&
+ !info->blob->actions->onOpen->trigger())
{
fprintf(stderr, "open %s fail: onOpen trigger failed\n", path.c_str());
+ info->blob->sessionsToUpdate.erase(info.get());
return false;
}
- v.blobState = blobs::StateFlags::open_read;
- sessionToBlob[session] = &v;
+ sessionInfoMap[session] = std::move(info);
return true;
}
std::vector<uint8_t> VersionBlobHandler::read(uint16_t session, uint32_t offset,
uint32_t requestedSize)
{
- auto pack = sessionToBlob.at(session);
-
- /* onOpen trigger must be successful, otherwise potential
- * for stale data to be read
- */
- if (pack->actions->onOpen->status() != ActionStatus::success)
+ auto& data = sessionInfoMap.at(session)->data;
+ if (data == nullptr || !*data || (*data)->size() < offset)
{
- fprintf(stderr, "read failed: onOpen trigger not successful\n");
return {};
}
- if (!pack->handler->open("don't care", std::ios::in))
- {
- fprintf(stderr, "read failed: file open unsuccessful blob=%s\n",
- pack->blobId.c_str());
- return {};
- }
- auto d = pack->handler->read(offset, requestedSize);
- if (!d)
- {
- fprintf(stderr, "read failed: unable to read file for blob %s\n",
- pack->blobId.c_str());
- pack->handler->close();
- return {};
- }
- pack->handler->close();
- return *d;
+ std::vector<uint8_t> ret(
+ std::min<size_t>(requestedSize, (*data)->size() - offset));
+ std::memcpy(&ret[0], &(**data)[offset], ret.size());
+ return ret;
}
bool VersionBlobHandler::close(uint16_t session)
{
- try
- {
- auto& pack = *sessionToBlob.at(session);
- pack.actions->onOpen->abort();
- pack.blobState = static_cast<blobs::StateFlags>(0);
- sessionToBlob.erase(session);
- return true;
- }
- catch (const std::out_of_range& e)
+ auto it = sessionInfoMap.find(session);
+ if (it == sessionInfoMap.end())
{
return false;
}
+ auto& info = *it->second;
+ info.blob->sessionsToUpdate.erase(&info);
+ if (info.blob->sessionsToUpdate.empty())
+ {
+ info.blob->actions->onOpen->abort();
+ }
+ sessionInfoMap.erase(it);
+ return true;
}
bool VersionBlobHandler::stat(uint16_t session, blobs::BlobMeta* meta)
diff --git a/bmc/version-handler/version_handler.hpp b/bmc/version-handler/version_handler.hpp
index 24b0d3a..128ec09 100644
--- a/bmc/version-handler/version_handler.hpp
+++ b/bmc/version-handler/version_handler.hpp
@@ -9,6 +9,8 @@
#include <cstdint>
#include <map>
#include <memory>
+#include <optional>
+#include <set>
#include <string>
#include <string_view>
#include <unordered_map>
@@ -66,15 +68,30 @@
bool expire(uint16_t session) override;
private:
+ struct SessionInfo;
+
struct BlobInfo
{
Pinned<std::string> blobId;
std::unique_ptr<ActionPack> actions;
std::unique_ptr<ImageHandlerInterface> handler;
- blobs::StateFlags blobState = static_cast<blobs::StateFlags>(0);
+ std::set<SessionInfo*> sessionsToUpdate;
+ };
+
+ struct SessionInfo
+ {
+ BlobInfo* blob;
+
+ // A cached copy of the version data shared by all clients for a single
+ // execution of the version retrieval action. This is is null until the
+ // TriggerableAction has completed. If the action is an error, the
+ // shared object is nullopt. Otherwise, contains a vector of the version
+ // data when successfully read.
+ std::shared_ptr<const std::optional<std::vector<uint8_t>>> data;
};
std::unordered_map<std::string_view, std::unique_ptr<BlobInfo>> blobInfoMap;
- std::unordered_map<uint16_t, BlobInfo*> sessionToBlob;
+ std::unordered_map<uint16_t, std::unique_ptr<SessionInfo>> sessionInfoMap;
};
+
} // namespace ipmi_flash