Fix Short read/write issue
It was possible for "short" reads and writes to cause the pattern and
zero steps to not work correctly. This change adds logic to deal with
the short reads.
Tested: unit test and machine test
root@bmc# time busctl call xyz.openbmc_project.eStoraged.mmcblk0 \
> /xyz/openbmc_project/inventory/storage/mmcblk0 \
> xyz.openbmc_project.Inventory.Item.Volume Erase s \
> xyz.openbmc_project.Inventory.Item.Volume.EraseMethod.ZeroOverWrite \
> --timeout=1200
real 6m0.815s
user 0m0.010s
sys 0m0.010s
Change-Id: If8df9bdba159a3bcfa77104a4c17b8d352794db2
Signed-off-by: John Edward Broadbent <jebr@google.com>
diff --git a/src/erase/pattern.cpp b/src/erase/pattern.cpp
index 5563661..7d7990b 100644
--- a/src/erase/pattern.cpp
+++ b/src/erase/pattern.cpp
@@ -10,22 +10,20 @@
#include <xyz/openbmc_project/Common/error.hpp>
#include <array>
+#include <chrono>
#include <iostream>
#include <random>
#include <span>
#include <string>
+#include <thread>
namespace estoraged
{
-constexpr uint32_t seed = 0x6a656272;
-constexpr size_t blockSize = 4096;
-constexpr size_t blockSizeUsing32 = blockSize / sizeof(uint32_t);
-
using sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
-using stdplus::fd::ManagedFd;
+using stdplus::fd::Fd;
-void Pattern::writePattern(const uint64_t driveSize)
+void Pattern::writePattern(const uint64_t driveSize, Fd& fd)
{
// static seed defines a fixed prng sequnce so it can be verified later,
// and validated for entropy
@@ -36,9 +34,6 @@
std::minstd_rand0 generator(seed);
std::array<std::byte, blockSize> randArr{};
- ManagedFd fd =
- stdplus::fd::open(devPath, stdplus::fd::OpenAccess::WriteOnly);
-
while (currentIndex < driveSize)
{
// generate a 4k block of prng
@@ -52,19 +47,34 @@
size_t writeSize = currentIndex + blockSize < driveSize
? blockSize
: driveSize - currentIndex;
- if (fd.write({randArr.data(), writeSize}).size() != writeSize)
+ size_t written = 0;
+ size_t retry = 0;
+ while (written < writeSize)
{
- lg2::error("Estoraged erase pattern unable to write sizeof(long)",
- "REDFISH_MESSAGE_ID",
- std::string("eStorageD.1.0.EraseFailure"),
- "REDFISH_MESSAGE_ARGS", std::to_string(fd.get()));
- throw InternalFailure();
+ written += fd.write({randArr.data() + written, writeSize - written})
+ .size();
+ if (written == writeSize)
+ {
+ break;
+ }
+ if (written > writeSize)
+ {
+ throw InternalFailure();
+ }
+ retry++;
+ if (retry > maxRetry)
+ {
+ lg2::error("Unable to do full write", "REDFISH_MESSAGE_ID",
+ std::string("eStorageD.1.0.EraseFailure"));
+ throw InternalFailure();
+ }
+ std::this_thread::sleep_for(delay);
}
currentIndex = currentIndex + writeSize;
}
}
-void Pattern::verifyPattern(const uint64_t driveSize)
+void Pattern::verifyPattern(const uint64_t driveSize, Fd& fd)
{
uint64_t currentIndex = 0;
@@ -74,9 +84,6 @@
std::array<std::byte, blockSize> randArr{};
std::array<std::byte, blockSize> readArr{};
- ManagedFd fd =
- stdplus::fd::open(devPath, stdplus::fd::OpenAccess::ReadOnly);
-
while (currentIndex < driveSize)
{
size_t readSize = currentIndex + blockSize < driveSize
@@ -91,14 +98,35 @@
{
(*randArrFill)[i] = generator();
}
- fd.read({readArr.data(), readSize});
+ size_t read = 0;
+ size_t retry = 0;
+ while (read < readSize)
+ {
+ read +=
+ fd.read({readArr.data() + read, readSize - read}).size();
+ if (read == readSize)
+ {
+ break;
+ }
+ if (read > readSize)
+ {
+ throw InternalFailure();
+ }
+ retry++;
+ if (retry > maxRetry)
+ {
+ lg2::error("Unable to do full read", "REDFISH_MESSAGE_ID",
+ std::string("eStorageD.1.0.EraseFailure"));
+ throw InternalFailure();
+ }
+ std::this_thread::sleep_for(delay);
+ }
}
catch (...)
{
lg2::error("Estoraged erase pattern unable to read",
"REDFISH_MESSAGE_ID",
- std::string("eStorageD.1.0.EraseFailure"),
- "REDFISH_MESSAGE_ARGS", std::to_string(fd.get()));
+ std::string("eStorageD.1.0.EraseFailure"));
throw InternalFailure();
}
diff --git a/src/erase/zero.cpp b/src/erase/zero.cpp
index 2c5a6f5..d26e1b9 100644
--- a/src/erase/zero.cpp
+++ b/src/erase/zero.cpp
@@ -2,26 +2,26 @@
#include "erase.hpp"
+#include <unistd.h>
+
#include <phosphor-logging/lg2.hpp>
#include <stdplus/fd/create.hpp>
#include <stdplus/fd/managed.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
#include <array>
+#include <chrono>
#include <span>
+#include <thread>
namespace estoraged
{
using sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
-using stdplus::fd::ManagedFd;
+using stdplus::fd::Fd;
-void Zero::writeZero(const uint64_t driveSize)
+void Zero::writeZero(const uint64_t driveSize, Fd& fd)
{
-
- ManagedFd fd =
- stdplus::fd::open(devPath, stdplus::fd::OpenAccess::WriteOnly);
-
uint64_t currentIndex = 0;
const std::array<const std::byte, blockSize> blockOfZeros{};
@@ -32,7 +32,31 @@
: driveSize - currentIndex;
try
{
- fd.write({blockOfZeros.data(), writeSize});
+ size_t written = 0;
+ size_t retry = 0;
+ while (written < writeSize)
+ {
+ written += fd.write({blockOfZeros.data() + written,
+ writeSize - written})
+ .size();
+ if (written == writeSize)
+ {
+ break;
+ }
+ if (written > writeSize)
+ {
+ throw InternalFailure();
+ }
+ retry++;
+ if (retry > maxRetry)
+ {
+ lg2::error("Unable to make full write",
+ "REDFISH_MESSAGE_ID",
+ std::string("eStorageD.1.0.EraseFailure"));
+ throw InternalFailure();
+ }
+ std::this_thread::sleep_for(delay);
+ }
}
catch (...)
{
@@ -45,11 +69,8 @@
}
}
-void Zero::verifyZero(uint64_t driveSize)
+void Zero::verifyZero(uint64_t driveSize, Fd& fd)
{
- ManagedFd fd =
- stdplus::fd::open(devPath, stdplus::fd::OpenAccess::ReadOnly);
-
uint64_t currentIndex = 0;
std::array<std::byte, blockSize> readArr{};
const std::array<const std::byte, blockSize> blockOfZeros{};
@@ -61,7 +82,29 @@
: driveSize - currentIndex;
try
{
- fd.read({readArr.data(), readSize});
+ size_t read = 0;
+ size_t retry = 0;
+ while (read < readSize)
+ {
+ read +=
+ fd.read({readArr.data() + read, readSize - read}).size();
+ if (read == readSize)
+ {
+ break;
+ }
+ if (read > readSize)
+ {
+ throw InternalFailure();
+ }
+ retry++;
+ if (retry > maxRetry)
+ {
+ lg2::error("Unable to make full read", "REDFISH_MESSAGE_ID",
+ std::string("eStorageD.1.0.EraseFailure"));
+ throw InternalFailure();
+ }
+ std::this_thread::sleep_for(delay);
+ }
}
catch (...)
{
diff --git a/src/test/erase/pattern_test.cpp b/src/test/erase/pattern_test.cpp
index e0192a0..2e2ac81 100644
--- a/src/test/erase/pattern_test.cpp
+++ b/src/test/erase/pattern_test.cpp
@@ -5,6 +5,7 @@
#include <unistd.h>
#include <stdplus/fd/create.hpp>
+#include <stdplus/fd/gmock.hpp>
#include <stdplus/fd/managed.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
@@ -20,6 +21,9 @@
using estoraged::Pattern;
using sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
+using testing::_;
+using testing::Invoke;
+using testing::Return;
TEST(pattern, patternPass)
{
@@ -30,14 +34,22 @@
std::ios::out | std::ios::binary | std::ios::trunc);
testFile.close();
+ stdplus::fd::Fd&& writeFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::WriteOnly);
+
Pattern pass(testFileName);
- EXPECT_NO_THROW(pass.writePattern(size));
- EXPECT_NO_THROW(pass.verifyPattern(size));
+ EXPECT_NO_THROW(pass.writePattern(size, writeFd));
+
+ stdplus::fd::Fd&& readFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadOnly);
+
+ EXPECT_NO_THROW(pass.verifyPattern(size, readFd));
}
/* This test that pattern writes the correct number of bytes even if
* size of the drive is not divisable by the block size
*/
+
TEST(pattern, patternNotDivisible)
{
std::string testFileName = "notDivisible";
@@ -48,9 +60,14 @@
std::ios::out | std::ios::binary | std::ios::trunc);
testFile.close();
+ stdplus::fd::Fd&& writeFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::WriteOnly);
Pattern pass(testFileName);
- EXPECT_NO_THROW(pass.writePattern(size));
- EXPECT_NO_THROW(pass.verifyPattern(size));
+ EXPECT_NO_THROW(pass.writePattern(size, writeFd));
+
+ stdplus::fd::Fd&& readFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadOnly);
+ EXPECT_NO_THROW(pass.verifyPattern(size, readFd));
}
TEST(pattern, patternsDontMatch)
@@ -67,8 +84,140 @@
sizeof(dummyValue));
testFile.close();
- EXPECT_NO_THROW(pass.writePattern(size - sizeof(dummyValue)));
- EXPECT_THROW(pass.verifyPattern(size), InternalFailure);
+ stdplus::fd::Fd&& writeFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::WriteOnly);
+
+ EXPECT_NO_THROW(pass.writePattern(size - sizeof(dummyValue), writeFd));
+
+ stdplus::fd::Fd&& readFd =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadOnly);
+
+ EXPECT_THROW(pass.verifyPattern(size, readFd), InternalFailure);
+}
+
+uint64_t size = 4096;
+size_t shortSize = 128;
+static auto shortData1 = std::vector<std::byte>(shortSize);
+static auto shortData2 = std::vector<std::byte>(shortSize);
+static auto restOfData = std::vector<std::byte>(size - shortSize * 2);
+
+TEST(pattern, shortReadWritePass)
+{
+ std::string testFileName = "testfile_shortRead";
+
+ uint64_t size = 4096;
+ Pattern pass(testFileName);
+ stdplus::fd::FdMock mock;
+
+ testing::Sequence s;
+
+ // test write pattern with short blocks
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Invoke([](std::span<const std::byte> x) {
+ std::copy_n(x.begin(), shortData1.size(), shortData1.begin());
+ return shortData1;
+ }))
+ .RetiresOnSaturation();
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Invoke([](std::span<const std::byte> x) {
+ std::copy_n(x.begin(), restOfData.size(), restOfData.begin());
+ return restOfData;
+ }))
+ .RetiresOnSaturation();
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Invoke([](std::span<const std::byte> x) {
+ std::copy_n(x.begin(), shortData2.size(), shortData2.begin());
+ return shortData2;
+ }))
+ .RetiresOnSaturation();
+
+ // test read pattern
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Invoke([](nonstd::span<std::byte> x) {
+ std::copy_n(shortData1.begin(), shortData1.size(), x.data());
+ nonstd::span ret(shortData1);
+ return ret;
+ }))
+ .RetiresOnSaturation();
+
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Invoke([](nonstd::span<std::byte> x) {
+ std::copy_n(restOfData.begin(), restOfData.size(), x.data());
+ nonstd::span ret(restOfData);
+ return ret;
+ }))
+ .RetiresOnSaturation();
+
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Invoke([](nonstd::span<std::byte> x) {
+ std::copy_n(shortData2.begin(), shortData2.size(), x.data());
+ nonstd::span ret(shortData2);
+ return ret;
+ }))
+ .RetiresOnSaturation();
+
+ EXPECT_NO_THROW(pass.writePattern(size, mock));
+ EXPECT_NO_THROW(pass.verifyPattern(size, mock));
+}
+
+TEST(Zeros, shortReadWriteFail)
+{
+ std::string testFileName = "testfile_shortRead";
+
+ uint64_t size = 4096;
+ size_t shortSize = 128;
+ Pattern tryPattern(testFileName);
+ auto shortData = std::vector<std::byte>(shortSize, std::byte{0});
+ auto restOfData =
+ std::vector<std::byte>(size - shortSize * 3, std::byte{0});
+ // open the file and write none to it
+
+ stdplus::fd::FdMock mock;
+
+ // test writes
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(restOfData)); // return too much data!
+
+ EXPECT_THROW(tryPattern.writePattern(size, mock), InternalFailure);
+
+ // test reads
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(restOfData)); // return too much data!
+
+ EXPECT_THROW(tryPattern.verifyPattern(size, mock), InternalFailure);
+}
+
+TEST(pattern, driveIsSmaller)
+{
+ std::string testFileName = "testfile_driveIsSmaller";
+
+ uint64_t size = 4096;
+ Pattern tryPattern(testFileName);
+
+ stdplus::fd::FdMock mocks;
+ testing::InSequence s;
+
+ // test writes
+ EXPECT_CALL(mocks, write(_))
+ .Times(33)
+ .WillRepeatedly(Return(std::vector<std::byte>{}))
+ .RetiresOnSaturation();
+
+ EXPECT_THROW(tryPattern.writePattern(size, mocks), InternalFailure);
+
+ // test reads
+ EXPECT_CALL(mocks, read(_))
+ .Times(33)
+ .WillRepeatedly(Return(std::vector<std::byte>{}))
+ .RetiresOnSaturation();
+
+ EXPECT_THROW(tryPattern.verifyPattern(size, mocks), InternalFailure);
}
} // namespace estoraged_test
diff --git a/src/test/erase/zero_test.cpp b/src/test/erase/zero_test.cpp
index 846059d..b14d546 100644
--- a/src/test/erase/zero_test.cpp
+++ b/src/test/erase/zero_test.cpp
@@ -5,6 +5,7 @@
#include <unistd.h>
#include <stdplus/fd/create.hpp>
+#include <stdplus/fd/gmock.hpp>
#include <stdplus/fd/managed.hpp>
#include <xyz/openbmc_project/Common/error.hpp>
@@ -15,12 +16,14 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
-using estoraged::Zero;
-using sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
-
namespace estoraged_test
{
+using estoraged::Zero;
+using sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
+using testing::_;
+using testing::Return;
+
TEST(Zeros, zeroPass)
{
std::string testFileName = "testfile_pass";
@@ -31,8 +34,12 @@
testFile.close();
uint64_t size = 4096;
Zero pass(testFileName);
- EXPECT_NO_THROW(pass.writeZero(size));
- EXPECT_NO_THROW(pass.verifyZero(size));
+ stdplus::fd::Fd&& write =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+ EXPECT_NO_THROW(pass.writeZero(size, write));
+ stdplus::fd::Fd&& read =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+ EXPECT_NO_THROW(pass.verifyZero(size, read));
}
/* This test that pattern writes the correct number of bytes even if
@@ -49,9 +56,14 @@
uint64_t size = 4097;
// 4097 is not divisible by the block size, and we expect no errors
+
+ stdplus::fd::Fd&& write =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
Zero pass(testFileName);
- EXPECT_NO_THROW(pass.writeZero(size));
- EXPECT_NO_THROW(pass.verifyZero(size));
+ EXPECT_NO_THROW(pass.writeZero(size, write));
+ stdplus::fd::Fd&& read =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+ EXPECT_NO_THROW(pass.verifyZero(size, read));
}
TEST(Zeros, notZeroStart)
@@ -66,15 +78,18 @@
sizeof(dummyValue));
testFile.close();
uint64_t size = 4096;
+ stdplus::fd::Fd&& readWrite =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+
Zero pass(testFileName);
- EXPECT_NO_THROW(pass.writeZero(size - sizeof(dummyValue)));
+ EXPECT_NO_THROW(pass.writeZero(size - sizeof(dummyValue), readWrite));
// force flush, needed for unit testing
std::ofstream file;
file.open(testFileName);
file.close();
- EXPECT_THROW(pass.verifyZero(size), InternalFailure);
+ EXPECT_THROW(pass.verifyZero(size, readWrite), InternalFailure);
}
TEST(Zeros, notZeroEnd)
@@ -89,14 +104,106 @@
uint64_t size = 4096;
Zero pass(testFileName);
uint32_t dummyValue = 88;
- EXPECT_NO_THROW(pass.writeZero(size - sizeof(dummyValue)));
+ stdplus::fd::Fd&& write =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+ EXPECT_NO_THROW(pass.writeZero(size - sizeof(dummyValue), write));
// open the file and write none zero to it
testFile.open(testFileName, std::ios::out);
testFile << dummyValue;
testFile.close();
- EXPECT_THROW(pass.verifyZero(size), InternalFailure);
+ stdplus::fd::Fd&& read =
+ stdplus::fd::open(testFileName, stdplus::fd::OpenAccess::ReadWrite);
+ EXPECT_THROW(pass.verifyZero(size, read), InternalFailure);
+}
+
+TEST(Zeros, shortReadWritePass)
+{
+ std::string testFileName = "testfile_shortRead";
+
+ uint64_t size = 4096;
+ size_t shortSize = 128;
+ Zero pass(testFileName);
+ auto shortData = std::vector<std::byte>(shortSize, std::byte{0});
+ auto restOfData =
+ std::vector<std::byte>(size - shortSize * 3, std::byte{0});
+ stdplus::fd::FdMock mock;
+
+ // test write zeros with short blocks
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(shortData));
+
+ EXPECT_NO_THROW(pass.writeZero(size, mock));
+
+ // test read zeros with short blocks
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(shortData));
+
+ EXPECT_NO_THROW(pass.verifyZero(size, mock));
+}
+
+TEST(Zeros, shortReadWriteFail)
+{
+ std::string testFileName = "testfile_shortRead";
+
+ uint64_t size = 4096;
+ size_t shortSize = 128;
+ Zero tryZero(testFileName);
+ auto shortData = std::vector<std::byte>(shortSize, std::byte{0});
+ auto restOfData =
+ std::vector<std::byte>(size - shortSize * 3, std::byte{0});
+ // open the file and write none zero to it
+
+ stdplus::fd::FdMock mock;
+
+ // test writes
+ EXPECT_CALL(mock, write(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(restOfData)); // return too much data!
+
+ EXPECT_THROW(tryZero.writeZero(size, mock), InternalFailure);
+
+ // test reads
+ EXPECT_CALL(mock, read(_))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(shortData))
+ .WillOnce(Return(restOfData))
+ .WillOnce(Return(restOfData)); // return too much data!
+
+ EXPECT_THROW(tryZero.verifyZero(size, mock), InternalFailure);
+}
+
+TEST(Zeros, driveIsSmaller)
+{
+ std::string testFileName = "testfile_driveIsSmaller";
+
+ uint64_t size = 4096;
+ Zero tryZero(testFileName);
+
+ stdplus::fd::FdMock mocks;
+ testing::InSequence s;
+
+ // test writes
+ EXPECT_CALL(mocks, write(_))
+ .Times(33)
+ .WillRepeatedly(Return(std::vector<std::byte>{}))
+ .RetiresOnSaturation();
+ EXPECT_THROW(tryZero.writeZero(size, mocks), InternalFailure);
+ // test reads
+ EXPECT_CALL(mocks, read(_))
+ .Times(33)
+ .WillRepeatedly(Return(std::vector<std::byte>{}))
+ .RetiresOnSaturation();
+ EXPECT_THROW(tryZero.verifyZero(size, mocks), InternalFailure);
}
} // namespace estoraged_test