extend file_handler to support reads
Problem: the upcomming version handler will need to read from files.
Currently file hander (image handler) does not support reads.
Solution: Add read support by providing an optional mode parameter.
Tests added:
FileHanderTest added to
- Test out open for reads
- Test out reading out bytes and verifying size and content
- Test out trying to read beyond EOF
- Test out offset reads that go beyond EOF
Tested:
Existing unit tests pass
New unit tests for reading all pass
Signed-off-by: Jason Ling <jasonling@google.com>
Change-Id: Ie416a6b4b452d8d04fa158bd55989d07a891896f
diff --git a/bmc/file_handler.cpp b/bmc/file_handler.cpp
index f7878ea..dacca19 100644
--- a/bmc/file_handler.cpp
+++ b/bmc/file_handler.cpp
@@ -18,6 +18,7 @@
#include <cstdint>
#include <filesystem>
+#include <ios>
#include <memory>
#include <string>
#include <vector>
@@ -26,8 +27,10 @@
{
namespace fs = std::filesystem;
-bool FileHandler::open(const std::string& path)
+bool FileHandler::open(const std::string& path, std::ios_base::openmode mode)
{
+ /* force binary mode */
+ mode |= std::ios::binary;
this->path = path;
if (file.is_open())
@@ -38,9 +41,8 @@
return false;
}
- /* using ofstream no need to set out */
- file.open(filename, std::ios::binary);
- if (file.bad())
+ file.open(filename, mode);
+ if (!file.good()) /* on success goodbit is set */
{
/* TODO: Oh no! Care about this. */
return false;
@@ -90,6 +92,46 @@
return true;
}
+std::optional<std::vector<uint8_t>> FileHandler::read(std::uint32_t offset,
+ std::uint32_t size)
+{
+ if (!file.is_open())
+ {
+ return std::nullopt;
+ }
+
+ /* determine size of file */
+ file.seekg(0, std::ios_base::end);
+ uint32_t filesize = file.tellg();
+ uint32_t bytesToRead = size;
+
+ /* make sure to not read past the end of file */
+ if (offset + size > filesize)
+ {
+ bytesToRead = filesize - offset;
+ }
+
+ /* if no bytes can be read, fail */
+ if (0 == bytesToRead)
+ {
+ return std::nullopt;
+ }
+
+ /* seek to offset then read */
+ file.seekg(offset);
+ std::vector<uint8_t> fileData(bytesToRead);
+ file.read(reinterpret_cast<char*>(fileData.data()), bytesToRead);
+
+ /* if any sort of failure happened during all the seeks
+ * and reads then fail the entire operation
+ */
+ if (!file.good())
+ {
+ return std::nullopt;
+ }
+ return fileData;
+}
+
int FileHandler::getSize()
{
try
diff --git a/bmc/file_handler.hpp b/bmc/file_handler.hpp
index 0c00d2d..bf31f40 100644
--- a/bmc/file_handler.hpp
+++ b/bmc/file_handler.hpp
@@ -23,10 +23,13 @@
explicit FileHandler(const std::string& filename) : filename(filename)
{}
- bool open(const std::string& path) override;
+ bool open(const std::string& path,
+ std::ios_base::openmode mode = std::ios::out) override;
void close() override;
bool write(std::uint32_t offset,
const std::vector<std::uint8_t>& data) override;
+ virtual std::optional<std::vector<uint8_t>>
+ read(std::uint32_t offset, std::uint32_t size) override;
int getSize() override;
private:
@@ -34,7 +37,7 @@
std::string path;
/** The file handle. */
- std::ofstream file;
+ std::fstream file;
/** The filename (including path) to use to write bytes. */
std::string filename;
diff --git a/bmc/firmware-handler/firmware_handler.cpp b/bmc/firmware-handler/firmware_handler.cpp
index d71cc9f..f5f1023 100644
--- a/bmc/firmware-handler/firmware_handler.cpp
+++ b/bmc/firmware-handler/firmware_handler.cpp
@@ -458,7 +458,7 @@
}
/* Ok, so we found a handler that matched, so call open() */
- if (!h->handler->open(path))
+ if (!h->handler->open(path, std::ios::out))
{
return false;
}
diff --git a/bmc/firmware-handler/test/file_handler_unittest.cpp b/bmc/firmware-handler/test/file_handler_unittest.cpp
index ab12125..b579830 100644
--- a/bmc/firmware-handler/test/file_handler_unittest.cpp
+++ b/bmc/firmware-handler/test/file_handler_unittest.cpp
@@ -21,7 +21,7 @@
}
};
-TEST_F(FileHandlerOpenTest, VerifyItIsHappy)
+TEST_F(FileHandlerOpenTest, VerifyDefaultOpenIsHappy)
{
/* Opening a fail may create it? */
@@ -32,6 +32,21 @@
EXPECT_FALSE(handler.open(""));
}
+TEST_F(FileHandlerOpenTest, VerifyOpenForReadFailsWithNoFile)
+{
+ FileHandler handler(TESTPATH);
+ EXPECT_FALSE(handler.open("", std::ios::in));
+}
+
+TEST_F(FileHandlerOpenTest, VerifyOpenForReadSucceedsWithFile)
+{
+ std::ofstream testfile;
+ testfile.open(TESTPATH, std::ios::out);
+ testfile << "Hello world";
+ FileHandler handler(TESTPATH);
+ EXPECT_TRUE(handler.open("", std::ios::in));
+}
+
TEST_F(FileHandlerOpenTest, VerifyWriteDataWrites)
{
/* Verify writing bytes writes them... flushing data can be an issue here,
@@ -55,4 +70,55 @@
/* annoyingly the memcmp was failing... but it's the same data. */
}
+TEST_F(FileHandlerOpenTest, VerifySimpleRead)
+{
+ std::ofstream testfile;
+ testfile.open(TESTPATH, std::ios::out);
+ std::vector<std::uint8_t> testPattern = {0x0, 0x1, 0x2, 0x3, 0x4,
+ 0x5, 0x6, 0x7, 0x8, 0x9};
+ testfile.write(reinterpret_cast<const char*>(testPattern.data()),
+ testPattern.size());
+ testfile.close();
+ FileHandler handler(TESTPATH);
+ EXPECT_TRUE(handler.open("", std::ios::in));
+ auto result = handler.read(0, 10);
+ ASSERT_TRUE(result);
+ EXPECT_EQ(result->size(), 10);
+ EXPECT_EQ(*result, testPattern);
+}
+
+TEST_F(FileHandlerOpenTest, VerifyTruncatedAndOffsetReads)
+{
+ std::ofstream testfile;
+ testfile.open(TESTPATH, std::ios::out);
+ std::vector<std::uint8_t> testPattern = {0x0, 0x1, 0x2, 0x3, 0x4,
+ 0x5, 0x6, 0x7, 0x8, 0x9};
+ std::vector<std::uint8_t> expectedResult(testPattern.begin() + 3,
+ testPattern.end());
+
+ testfile.write(reinterpret_cast<const char*>(testPattern.data()),
+ testPattern.size());
+ testfile.close();
+ FileHandler handler(TESTPATH);
+ EXPECT_TRUE(handler.open("", std::ios::in));
+ auto result = handler.read(3, 10);
+ ASSERT_TRUE(result);
+ EXPECT_EQ(*result, expectedResult);
+}
+
+TEST_F(FileHandlerOpenTest, VerifyBadOffsetReadsFail)
+{
+ std::ofstream testfile;
+ testfile.open(TESTPATH, std::ios::out);
+ std::vector<std::uint8_t> testPattern = {0x0, 0x1, 0x2, 0x3, 0x4,
+ 0x5, 0x6, 0x7, 0x8, 0x9};
+ testfile.write(reinterpret_cast<const char*>(testPattern.data()),
+ testPattern.size());
+ testfile.close();
+ FileHandler handler(TESTPATH);
+ EXPECT_TRUE(handler.open("", std::ios::in));
+ auto result = handler.read(11, 10);
+ EXPECT_FALSE(result);
+}
+
} // namespace ipmi_flash
diff --git a/bmc/firmware-handler/test/firmware_close_unittest.cpp b/bmc/firmware-handler/test/firmware_close_unittest.cpp
index 6d1fe55..2a722b6 100644
--- a/bmc/firmware-handler/test/firmware_close_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_close_unittest.cpp
@@ -28,7 +28,8 @@
* everything looks right.
*/
EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
- EXPECT_CALL(*hashImageMock, open(StrEq(hashBlobId))).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(StrEq(hashBlobId), std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::lpc,
@@ -55,7 +56,8 @@
/* Boring test where you open a blob_id using ipmi, so there's no data
* handler, and it's closed and everything looks right.
*/
- EXPECT_CALL(*hashImageMock, open(StrEq(hashBlobId))).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(StrEq(hashBlobId), std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi,
diff --git a/bmc/firmware-handler/test/firmware_commit_unittest.cpp b/bmc/firmware-handler/test/firmware_commit_unittest.cpp
index cb50b8e..d564de6 100644
--- a/bmc/firmware-handler/test/firmware_commit_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_commit_unittest.cpp
@@ -48,7 +48,8 @@
auto handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), CreateActionMap("asdf"));
- EXPECT_CALL(*imageMock2, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock2, open("asdf", std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi, "asdf"));
@@ -64,7 +65,8 @@
auto handler = FirmwareBlobHandler::CreateFirmwareBlobHandler(
std::move(blobs), std::move(data), CreateActionMap("asdf"));
- EXPECT_CALL(*imageMock1, open(StrEq(hashBlobId))).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock1, open(StrEq(hashBlobId), std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi,
diff --git a/bmc/firmware-handler/test/firmware_multiplebundle_unittest.cpp b/bmc/firmware-handler/test/firmware_multiplebundle_unittest.cpp
index 20c63a9..58f07c7 100644
--- a/bmc/firmware-handler/test/firmware_multiplebundle_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_multiplebundle_unittest.cpp
@@ -118,7 +118,7 @@
/* You can only have one file open at a time, and the first firmware file
* you open locks it down
*/
- EXPECT_CALL(*staticImageMock, open(staticLayoutBlobId))
+ EXPECT_CALL(*staticImageMock, open(staticLayoutBlobId, std::ios::out))
.WillOnce(Return(true));
EXPECT_CALL(*bmcPrepareMockPtr, trigger()).WillOnce(Return(true));
@@ -130,14 +130,15 @@
expectedState(FirmwareBlobHandler::UpdateState::verificationPending);
- EXPECT_CALL(*biosImageMock, open(biosBlobId)).Times(0);
+ EXPECT_CALL(*biosImageMock, open(biosBlobId, std::ios::out)).Times(0);
EXPECT_FALSE(handler->open(session, flags, biosBlobId));
}
TEST_F(IpmiOnlyTwoFirmwaresTest, OpeningHashBeforeBiosSucceeds)
{
/* Opening the hash blob does nothing special in this regard. */
- EXPECT_CALL(*hashImageMock, open(hashBlobId)).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(hashBlobId, std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(session, flags, hashBlobId));
expectedState(FirmwareBlobHandler::UpdateState::uploadInProgress);
@@ -148,7 +149,8 @@
expectedState(FirmwareBlobHandler::UpdateState::verificationPending);
ASSERT_FALSE(handler->canHandleBlob(verifyBlobId));
- EXPECT_CALL(*biosImageMock, open(biosBlobId)).WillOnce(Return(true));
+ EXPECT_CALL(*biosImageMock, open(biosBlobId, std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(session, flags, biosBlobId));
expectedState(FirmwareBlobHandler::UpdateState::uploadInProgress);
diff --git a/bmc/firmware-handler/test/firmware_sessionstat_unittest.cpp b/bmc/firmware-handler/test/firmware_sessionstat_unittest.cpp
index 0736293..3753118 100644
--- a/bmc/firmware-handler/test/firmware_sessionstat_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_sessionstat_unittest.cpp
@@ -25,7 +25,7 @@
/* Verifying running stat if the type of data session is IPMI returns no
* metadata.
*/
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi, "asdf"));
@@ -48,7 +48,7 @@
* simply implementing read().
*/
EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::lpc, "asdf"));
diff --git a/bmc/firmware-handler/test/firmware_state_notyetstarted_tarball_unittest.cpp b/bmc/firmware-handler/test/firmware_state_notyetstarted_tarball_unittest.cpp
index e666395..6a39d9d 100644
--- a/bmc/firmware-handler/test/firmware_state_notyetstarted_tarball_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_state_notyetstarted_tarball_unittest.cpp
@@ -65,11 +65,13 @@
{
if (blobId == hashBlobId)
{
- EXPECT_CALL(*hashImageMock, open(blobId)).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(blobId, std::ios::out))
+ .WillOnce(Return(true));
}
else
{
- EXPECT_CALL(*imageMock, open(blobId)).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open(blobId, std::ios::out))
+ .WillOnce(Return(true));
}
EXPECT_TRUE(handler->open(session, flags, blobId));
expectedState(FirmwareBlobHandler::UpdateState::uploadInProgress);
diff --git a/bmc/firmware-handler/test/firmware_state_notyetstarted_unittest.cpp b/bmc/firmware-handler/test/firmware_state_notyetstarted_unittest.cpp
index 970ae2f..0193e91 100644
--- a/bmc/firmware-handler/test/firmware_state_notyetstarted_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_state_notyetstarted_unittest.cpp
@@ -88,7 +88,8 @@
*/
TEST_F(FirmwareHandlerNotYetStartedTest, OpenStaticImageFileVerifyStateChange)
{
- EXPECT_CALL(*imageMock2, open(staticLayoutBlobId)).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock2, open(staticLayoutBlobId, std::ios::out))
+ .WillOnce(Return(true));
EXPECT_CALL(*prepareMockPtr, trigger()).WillOnce(Return(true));
EXPECT_TRUE(handler->open(session, flags, staticLayoutBlobId));
@@ -100,7 +101,8 @@
TEST_F(FirmwareHandlerNotYetStartedTest, OpenHashFileVerifyStateChange)
{
- EXPECT_CALL(*hashImageMock, open(hashBlobId)).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(hashBlobId, std::ios::out))
+ .WillOnce(Return(true));
/* Opening the hash blob id doesn't trigger a preparation, only a firmware
* blob.
*/
diff --git a/bmc/firmware-handler/test/firmware_state_verificationpending_unittest.cpp b/bmc/firmware-handler/test/firmware_state_verificationpending_unittest.cpp
index dc6f3d3..6188890 100644
--- a/bmc/firmware-handler/test/firmware_state_verificationpending_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_state_verificationpending_unittest.cpp
@@ -240,7 +240,8 @@
/* Verifies it isn't triggered again. */
EXPECT_CALL(*prepareMockPtr, trigger()).Times(0);
- EXPECT_CALL(*imageMock2, open(staticLayoutBlobId)).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock2, open(staticLayoutBlobId, std::ios::out))
+ .WillOnce(Return(true));
EXPECT_TRUE(handler->open(session, flags, staticLayoutBlobId));
expectedState(FirmwareBlobHandler::UpdateState::uploadInProgress);
diff --git a/bmc/firmware-handler/test/firmware_unittest.hpp b/bmc/firmware-handler/test/firmware_unittest.hpp
index e9db343..da2e8f7 100644
--- a/bmc/firmware-handler/test/firmware_unittest.hpp
+++ b/bmc/firmware-handler/test/firmware_unittest.hpp
@@ -76,11 +76,13 @@
{
if (blobId == hashBlobId)
{
- EXPECT_CALL(*hashImageMock, open(blobId)).WillOnce(Return(true));
+ EXPECT_CALL(*hashImageMock, open(blobId, std::ios::out))
+ .WillOnce(Return(true));
}
else
{
- EXPECT_CALL(*imageMock2, open(blobId)).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock2, open(blobId, std::ios::out))
+ .WillOnce(Return(true));
}
if (blobId != hashBlobId)
diff --git a/bmc/firmware-handler/test/firmware_write_unittest.cpp b/bmc/firmware-handler/test/firmware_write_unittest.cpp
index e9b70ba..27899db 100644
--- a/bmc/firmware-handler/test/firmware_write_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_write_unittest.cpp
@@ -29,7 +29,7 @@
TEST_F(FirmwareHandlerWriteTestIpmiOnly, DataTypeIpmiWriteSuccess)
{
/* Verify if data type ipmi, it calls write with the bytes. */
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi, "asdf"));
@@ -44,7 +44,7 @@
{
/* Verify if data type non-ipmi, it calls write with the length. */
EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::lpc, "asdf"));
@@ -67,7 +67,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(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::lpc, "asdf"));
diff --git a/bmc/firmware-handler/test/firmware_writemeta_unittest.cpp b/bmc/firmware-handler/test/firmware_writemeta_unittest.cpp
index 512754a..3d86c0c 100644
--- a/bmc/firmware-handler/test/firmware_writemeta_unittest.cpp
+++ b/bmc/firmware-handler/test/firmware_writemeta_unittest.cpp
@@ -23,7 +23,7 @@
TEST_F(FirmwareHandlerWriteMetaTest, WriteConfigParametersFailIfOverIPMI)
{
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::ipmi, "asdf"));
@@ -36,7 +36,7 @@
TEST_F(FirmwareHandlerWriteMetaTest, WriteConfigParametersPassedThrough)
{
EXPECT_CALL(*dataMock, open()).WillOnce(Return(true));
- EXPECT_CALL(*imageMock, open("asdf")).WillOnce(Return(true));
+ EXPECT_CALL(*imageMock, open("asdf", std::ios::out)).WillOnce(Return(true));
EXPECT_TRUE(handler->open(
0, blobs::OpenFlags::write | FirmwareFlags::UpdateFlags::lpc, "asdf"));
diff --git a/bmc/firmware-handler/test/image_mock.hpp b/bmc/firmware-handler/test/image_mock.hpp
index fb10c39..2a967ac 100644
--- a/bmc/firmware-handler/test/image_mock.hpp
+++ b/bmc/firmware-handler/test/image_mock.hpp
@@ -11,10 +11,11 @@
{
public:
virtual ~ImageHandlerMock() = default;
-
- MOCK_METHOD1(open, bool(const std::string&));
+ MOCK_METHOD2(open, bool(const std::string&, std::ios_base::openmode));
MOCK_METHOD0(close, void());
MOCK_METHOD2(write, bool(std::uint32_t, const std::vector<std::uint8_t>&));
+ MOCK_METHOD2(read, std::optional<std::vector<std::uint8_t>>(std::uint32_t,
+ std::uint32_t));
MOCK_METHOD0(getSize, int());
};
diff --git a/bmc/image_handler.hpp b/bmc/image_handler.hpp
index c024ec9..4d5b475 100644
--- a/bmc/image_handler.hpp
+++ b/bmc/image_handler.hpp
@@ -3,6 +3,7 @@
#include <cstdint>
#include <fstream>
#include <memory>
+#include <optional>
#include <string>
#include <vector>
@@ -23,7 +24,8 @@
* @param[in] path - the path passed to the handler (the blob_id).
* @return bool - returns true on success.
*/
- virtual bool open(const std::string& path) = 0;
+ virtual bool open(const std::string& path,
+ std::ios_base::openmode mode) = 0;
/**
* close the image.
@@ -39,6 +41,17 @@
*/
virtual bool write(std::uint32_t offset,
const std::vector<std::uint8_t>& data) = 0;
+ /**
+ * read data from a file.
+ *
+ * @param[in] offset - 0-based offset into the file.
+ * @param[in] size - the number of bytes
+ * @return std::optional<std::vector<std::uint8_t>> - returns std::nullopt
+ * on failure otherwise returns a vector filled with the bytes read.
+ *
+ */
+ virtual std::optional<std::vector<std::uint8_t>>
+ read(std::uint32_t offset, std::uint32_t size) = 0;
/**
* return the size of the file (if that notion makes sense).