bmc: move data handler to owned object
To handle the case where one cannot have complex static or global
objects, the object must be owned. In this case, it is reasonable to
pass ownership to the only object that will use the data handler.
Alternatively, singletons can be used to get around this, but just using
ownership is more appropriate.
Signed-off-by: Patrick Venture <venture@google.com>
Change-Id: I47b291d1cd01af9d4b7287c1411ed9ba37aa7a4c
diff --git a/bmc/data_handler.hpp b/bmc/data_handler.hpp
index bf17934..162347c 100644
--- a/bmc/data_handler.hpp
+++ b/bmc/data_handler.hpp
@@ -1,6 +1,7 @@
#pragma once
#include <cstdint>
+#include <memory>
#include <vector>
namespace ipmi_flash
@@ -57,7 +58,19 @@
struct DataHandlerPack
{
std::uint16_t bitmask;
- DataInterface* handler;
+ std::unique_ptr<DataInterface> handler;
+
+ DataHandlerPack(std::uint16_t bitmask,
+ std::unique_ptr<DataInterface> handler) :
+ bitmask(bitmask),
+ handler(std::move(handler))
+ {}
+
+ /* Don't allow copying, assignment or move assignment, only moving. */
+ DataHandlerPack(const DataHandlerPack&) = delete;
+ DataHandlerPack& operator=(const DataHandlerPack&) = delete;
+ DataHandlerPack(DataHandlerPack&&) = default;
+ DataHandlerPack& operator=(DataHandlerPack&&) = delete;
};
} // namespace ipmi_flash
diff --git a/bmc/firmware_handler.cpp b/bmc/firmware_handler.cpp
index bc9ff84..ccd76d3 100644
--- a/bmc/firmware_handler.cpp
+++ b/bmc/firmware_handler.cpp
@@ -479,7 +479,7 @@
}
curr->flags = flags;
- curr->dataHandler = d->handler;
+ curr->dataHandler = d->handler.get();
curr->imageHandler = h->handler.get();
lookup[session] = curr;
diff --git a/bmc/main.cpp b/bmc/main.cpp
index 97e18e1..5f1538d 100644
--- a/bmc/main.cpp
+++ b/bmc/main.cpp
@@ -44,38 +44,9 @@
namespace
{
-#ifdef NUVOTON_P2A_MBOX
-static constexpr std::size_t memoryRegionSize = 16 * 1024UL;
-#elif defined NUVOTON_P2A_VGA
-static constexpr std::size_t memoryRegionSize = 4 * 1024 * 1024UL;
-#else
-/* The maximum external buffer size we expect is 64KB. */
-static constexpr std::size_t memoryRegionSize = 64 * 1024UL;
-#endif
-
static constexpr const char* jsonConfigurationPath =
"/usr/share/phosphor-ipmi-flash/";
-#ifdef ENABLE_LPC_BRIDGE
-#if defined(ASPEED_LPC)
-LpcDataHandler lpcDataHandler(
- LpcMapperAspeed::createAspeedMapper(MAPPED_ADDRESS, memoryRegionSize));
-#elif defined(NUVOTON_LPC)
-LpcDataHandler lpcDataHandler(
- LpcMapperNuvoton::createNuvotonMapper(MAPPED_ADDRESS, memoryRegionSize));
-#else
-#error "You must specify a hardware implementation."
-#endif
-#endif
-
-#ifdef ENABLE_PCI_BRIDGE
-PciDataHandler pciDataHandler(MAPPED_ADDRESS, memoryRegionSize);
-#endif
-
-#ifdef ENABLE_NET_BRIDGE
-NetDataHandler netDataHandler;
-#endif
-
/**
* Given a name and path, create a HandlerPack.
*
@@ -101,18 +72,45 @@
{
using namespace ipmi_flash;
- std::vector<DataHandlerPack> supportedTransports = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
+#ifdef NUVOTON_P2A_MBOX
+ constexpr std::size_t memoryRegionSize = 16 * 1024UL;
+#elif defined NUVOTON_P2A_VGA
+ constexpr std::size_t memoryRegionSize = 4 * 1024 * 1024UL;
+#else
+ /* The maximum external buffer size we expect is 64KB. */
+ constexpr std::size_t memoryRegionSize = 64 * 1024UL;
+#endif
+
+ std::vector<DataHandlerPack> supportedTransports;
+
+ supportedTransports.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
#ifdef ENABLE_PCI_BRIDGE
- {FirmwareFlags::UpdateFlags::p2a, &pciDataHandler},
+ supportedTransports.emplace_back(
+ FirmwareFlags::UpdateFlags::p2a,
+ std::make_unique<PciDataHandler>(MAPPED_ADDRESS, memoryRegionSize));
#endif
+
#ifdef ENABLE_LPC_BRIDGE
- {FirmwareFlags::UpdateFlags::lpc, &lpcDataHandler},
+#if defined(ASPEED_LPC)
+ supportedTransports.emplace_back(
+ FirmwareFlags::UpdateFlags::lpc,
+ std::make_unique<LpcDataHandler>(LpcMapperAspeed::createAspeedMapper(
+ MAPPED_ADDRESS, memoryRegionSize)));
+#elif defined(NUVOTON_LPC)
+ supportedTransports.emplace_back(
+ FirmwareFlags::UpdateFlags::lpc,
+ std::make_unique<LpcDataHandler>(LpcMapperNuvoton::createNuvotonMapper(
+ MAPPED_ADDRESS, memoryRegionSize)));
+#else
+#error "You must specify a hardware implementation."
#endif
+#endif
+
#ifdef ENABLE_NET_BRIDGE
- {FirmwareFlags::UpdateFlags::net, &netDataHandler},
+ supportedTransports.emplace_back(FirmwareFlags::UpdateFlags::net,
+ std::make_unique<NetDataHandler>());
#endif
- };
ActionMap actionPacks = {};
diff --git a/bmc/test/firmware_canhandle_unittest.cpp b/bmc/test/firmware_canhandle_unittest.cpp
index b18b525..861503c 100644
--- a/bmc/test/firmware_canhandle_unittest.cpp
+++ b/bmc/test/firmware_canhandle_unittest.cpp
@@ -32,9 +32,8 @@
blobs.push_back(
std::move(HandlerPack("bcdf", std::make_unique<ImageHandlerMock>())));
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
auto handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), std::move(CreateActionMap("asdf")));
diff --git a/bmc/test/firmware_close_unittest.cpp b/bmc/test/firmware_close_unittest.cpp
index 580fcf4..6ab2b8d 100644
--- a/bmc/test/firmware_close_unittest.cpp
+++ b/bmc/test/firmware_close_unittest.cpp
@@ -28,7 +28,7 @@
/* Boring test where you open a blob_id, then verify that when it's closed
* everything looks right.
*/
- EXPECT_CALL(dataMock, open()).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
EXPECT_CALL(*hashImageMock, open(StrEq(hashBlobId))).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
@@ -42,7 +42,7 @@
activeHashBlobId));
/* Set up close() expectations. */
- EXPECT_CALL(dataMock, close());
+ EXPECT_CALL(*dataMock, close());
EXPECT_CALL(*hashImageMock, close());
EXPECT_TRUE(handler->close(0));
diff --git a/bmc/test/firmware_commit_unittest.cpp b/bmc/test/firmware_commit_unittest.cpp
index eb11211..846aa4f 100644
--- a/bmc/test/firmware_commit_unittest.cpp
+++ b/bmc/test/firmware_commit_unittest.cpp
@@ -39,9 +39,7 @@
imageMock2 = reinterpret_cast<ImageHandlerMock*>(image.get());
blobs.push_back(std::move(HandlerPack("asdf", std::move(image))));
- data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
}
};
diff --git a/bmc/test/firmware_handler_unittest.cpp b/bmc/test/firmware_handler_unittest.cpp
index bbbd15a..b886a01 100644
--- a/bmc/test/firmware_handler_unittest.cpp
+++ b/bmc/test/firmware_handler_unittest.cpp
@@ -19,9 +19,8 @@
TEST(FirmwareHandlerTest, CreateEmptyHandlerListVerifyFails)
{
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
auto handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
{}, std::move(data), std::move(CreateActionMap("abcd")));
@@ -47,9 +46,8 @@
/* The ActionPack map corresponds to the firmware list passed in, but
* they're not checked against each other yet.
*/
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
std::vector<HandlerPack> blobs;
blobs.push_back(
@@ -68,9 +66,8 @@
/* The hashblob handler must be one of the entries, but it cannot be the
* only entry.
*/
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
/* Provide a firmware list that has the hash blob, which is the required one
* -- tested in a different test.
@@ -90,11 +87,12 @@
blobs2.push_back(
std::move(HandlerPack("asdf", std::make_unique<ImageHandlerMock>())));
- data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data2;
+ data2.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
- std::move(blobs2), std::move(data), std::move(CreateActionMap("asdf")));
+ std::move(blobs2), std::move(data2),
+ std::move(CreateActionMap("asdf")));
auto result = handler->getBlobIds();
std::vector<std::string> expectedBlobs = {"asdf", hashBlobId};
@@ -102,9 +100,8 @@
}
TEST(FirmwareHandlerTest, VerifyHashRequiredForHappiness)
{
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
/* This works fine only if you also pass in the hash handler. */
std::vector<HandlerPack> blobs;
@@ -121,11 +118,12 @@
blobs2.push_back(std::move(
HandlerPack(hashBlobId, std::make_unique<ImageHandlerMock>())));
- data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data2;
+ data2.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
- std::move(blobs2), std::move(data), std::move(CreateActionMap("asdf")));
+ std::move(blobs2), std::move(data2),
+ std::move(CreateActionMap("asdf")));
auto result = handler->getBlobIds();
std::vector<std::string> expectedBlobs = {"asdf", hashBlobId};
diff --git a/bmc/test/firmware_multiplebundle_unittest.cpp b/bmc/test/firmware_multiplebundle_unittest.cpp
index a2984b7..ae7924f 100644
--- a/bmc/test/firmware_multiplebundle_unittest.cpp
+++ b/bmc/test/firmware_multiplebundle_unittest.cpp
@@ -87,6 +87,9 @@
packs[staticLayoutBlobId] = std::move(bmcPack);
packs[biosBlobId] = std::move(biosPack);
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), std::move(packs));
}
@@ -100,8 +103,6 @@
ImageHandlerMock *hashImageMock, *staticImageMock, *biosImageMock;
std::vector<HandlerPack> blobs;
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr}};
std::unique_ptr<blobs::GenericBlobInterface> handler;
diff --git a/bmc/test/firmware_open_unittest.cpp b/bmc/test/firmware_open_unittest.cpp
index a2e3d95..ba0ac87 100644
--- a/bmc/test/firmware_open_unittest.cpp
+++ b/bmc/test/firmware_open_unittest.cpp
@@ -20,11 +20,10 @@
TEST_P(FirmwareOpenFailTest, WithFlags)
{
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- {FirmwareFlags::UpdateFlags::p2a, nullptr},
- {FirmwareFlags::UpdateFlags::lpc, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+ data.emplace_back(FirmwareFlags::UpdateFlags::p2a, nullptr);
+ data.emplace_back(FirmwareFlags::UpdateFlags::lpc, nullptr);
std::vector<HandlerPack> blobs;
blobs.push_back(std::move(
diff --git a/bmc/test/firmware_sessionstat_unittest.cpp b/bmc/test/firmware_sessionstat_unittest.cpp
index 0e2796c..896dd70 100644
--- a/bmc/test/firmware_sessionstat_unittest.cpp
+++ b/bmc/test/firmware_sessionstat_unittest.cpp
@@ -47,7 +47,7 @@
* P2A to for now. Later, LPC may have reason to provide data, and can by
* simply implementing read().
*/
- EXPECT_CALL(dataMock, open()).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
@@ -56,7 +56,7 @@
int size = 512;
EXPECT_CALL(*imageMock, getSize()).WillOnce(Return(size));
std::vector<std::uint8_t> mBytes = {0x01, 0x02};
- EXPECT_CALL(dataMock, readMeta()).WillOnce(Return(mBytes));
+ EXPECT_CALL(*dataMock, readMeta()).WillOnce(Return(mBytes));
blobs::BlobMeta meta;
EXPECT_TRUE(handler->stat(0, &meta));
diff --git a/bmc/test/firmware_stat_unittest.cpp b/bmc/test/firmware_stat_unittest.cpp
index 1313c5a..86075bb 100644
--- a/bmc/test/firmware_stat_unittest.cpp
+++ b/bmc/test/firmware_stat_unittest.cpp
@@ -31,9 +31,8 @@
blobs.push_back(
std::move(HandlerPack("asdf", std::make_unique<ImageHandlerMock>())));
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- };
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
auto handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), std::move(CreateActionMap("asdf")));
diff --git a/bmc/test/firmware_state_notyetstarted_tarball_unittest.cpp b/bmc/test/firmware_state_notyetstarted_tarball_unittest.cpp
index 6ce6102..32e05ef 100644
--- a/bmc/test/firmware_state_notyetstarted_tarball_unittest.cpp
+++ b/bmc/test/firmware_state_notyetstarted_tarball_unittest.cpp
@@ -49,6 +49,9 @@
ActionMap packs;
packs[ubiTarballBlobId] = std::move(actionPack);
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), std::move(packs));
}
@@ -75,8 +78,6 @@
ImageHandlerMock *hashImageMock, *imageMock;
std::vector<HandlerPack> blobs;
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr}};
std::unique_ptr<blobs::GenericBlobInterface> handler;
TriggerMock* verifyMockPtr;
TriggerMock* updateMockPtr;
diff --git a/bmc/test/firmware_unittest.hpp b/bmc/test/firmware_unittest.hpp
index 0b4dc53..2b97d01 100644
--- a/bmc/test/firmware_unittest.hpp
+++ b/bmc/test/firmware_unittest.hpp
@@ -59,6 +59,9 @@
ActionMap packs;
packs[staticLayoutBlobId] = std::move(actionPack);
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), std::move(packs));
}
@@ -174,8 +177,6 @@
ImageHandlerMock *hashImageMock, *imageMock2;
std::vector<HandlerPack> blobs;
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr}};
std::unique_ptr<blobs::GenericBlobInterface> handler;
@@ -197,8 +198,6 @@
protected:
ImageHandlerMock *hashImageMock, *imageMock;
std::vector<HandlerPack> blobs;
- std::vector<DataHandlerPack> data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr}};
std::unique_ptr<blobs::GenericBlobInterface> handler;
void SetUp() override
@@ -212,6 +211,9 @@
imageMock = reinterpret_cast<ImageHandlerMock*>(image.get());
blobs.push_back(std::move(HandlerPack("asdf", std::move(image))));
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data),
std::move(CreateActionMap("asdf")));
@@ -221,10 +223,9 @@
class FakeLpcFirmwareTest : public ::testing::Test
{
protected:
- DataHandlerMock dataMock;
+ DataHandlerMock* dataMock;
ImageHandlerMock *hashImageMock, *imageMock;
std::vector<HandlerPack> blobs;
- std::vector<DataHandlerPack> data;
std::unique_ptr<blobs::GenericBlobInterface> handler;
void SetUp() override
@@ -238,10 +239,13 @@
imageMock = reinterpret_cast<ImageHandlerMock*>(image.get());
blobs.push_back(std::move(HandlerPack("asdf", std::move(image))));
- data = {
- {FirmwareFlags::UpdateFlags::ipmi, nullptr},
- {FirmwareFlags::UpdateFlags::lpc, &dataMock},
- };
+ auto dataMockInstance = std::make_unique<DataHandlerMock>();
+ dataMock = dataMockInstance.get();
+
+ std::vector<DataHandlerPack> data;
+ data.emplace_back(FirmwareFlags::UpdateFlags::ipmi, nullptr);
+ data.emplace_back(FirmwareFlags::UpdateFlags::lpc,
+ std::move(dataMockInstance));
handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data),
std::move(CreateActionMap("asdf")));
diff --git a/bmc/test/firmware_write_unittest.cpp b/bmc/test/firmware_write_unittest.cpp
index a47134f..e9b70ba 100644
--- a/bmc/test/firmware_write_unittest.cpp
+++ b/bmc/test/firmware_write_unittest.cpp
@@ -43,7 +43,7 @@
TEST_F(FirmwareHandlerWriteTestLpc, DataTypeNonIpmiWriteSuccess)
{
/* Verify if data type non-ipmi, it calls write with the length. */
- EXPECT_CALL(dataMock, open()).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
@@ -57,7 +57,7 @@
std::vector<std::uint8_t> bytes = {0x01, 0x02, 0x03, 0x04};
- EXPECT_CALL(dataMock, copyFrom(request.length)).WillOnce(Return(bytes));
+ EXPECT_CALL(*dataMock, copyFrom(request.length)).WillOnce(Return(bytes));
EXPECT_CALL(*imageMock, write(0, Eq(bytes))).WillOnce(Return(true));
EXPECT_TRUE(handler->write(0, 0, ipmiRequest));
}
@@ -66,7 +66,7 @@
{
/* Verify the data type non-ipmi, if the request's structure doesn't match,
* return failure. */
- EXPECT_CALL(dataMock, open()).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
diff --git a/bmc/test/firmware_writemeta_unittest.cpp b/bmc/test/firmware_writemeta_unittest.cpp
index 22f778d..512754a 100644
--- a/bmc/test/firmware_writemeta_unittest.cpp
+++ b/bmc/test/firmware_writemeta_unittest.cpp
@@ -35,7 +35,7 @@
TEST_F(FirmwareHandlerWriteMetaTest, WriteConfigParametersPassedThrough)
{
- EXPECT_CALL(dataMock, open()).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
@@ -43,7 +43,7 @@
std::vector<std::uint8_t> bytes = {0x01, 0x02, 0x03, 0x04};
- EXPECT_CALL(dataMock, writeMeta(Eq(bytes))).WillOnce(Return(true));
+ EXPECT_CALL(*dataMock, writeMeta(Eq(bytes))).WillOnce(Return(true));
EXPECT_TRUE(handler->writeMeta(0, 0, bytes));
}