buffer: bugfix (v2) consolidate the offset and readptr
- It didn't make sense to make the user keep count of the different
types of read pointers, instead do the relative calculation at the
lowest level (wraparoundRead)
- Added further tests to prove out that readPtr is being updated
properly in wraparoundRead)
- Fixed wraparoundRead, as it was relying on the dataInterface->read to
read until the "end". The "end" was fixed in the bugfix #1 to be the
end of the queue instead of the mmaped buffer.
- took out addtionalBoundaryCheck from wraparoundRead, as it was no
longer needed
Tested: Unit tested
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: I101360223597d197362dc1dbe4a27601da09933f
diff --git a/include/buffer.hpp b/include/buffer.hpp
index 7738339..d86f204 100644
--- a/include/buffer.hpp
+++ b/include/buffer.hpp
@@ -105,39 +105,37 @@
/**
* Write to the bufferHeader and update the read pointer
- * @param[in] newReadPtr - read pointer to update to.
- * "readPtr"s (including this one) are offset relative to the "Error Log
- * Queue region" = (sizeof(CircularBufferHeader) + UE reserved region)
+ * @param[in] newReadPtr - read pointer to update to
*/
virtual void updateReadPtr(const uint32_t newReadPtr) = 0;
/**
* Wrapper for the dataInterface->read, performs wraparound read
*
- * @param[in] offset - offset relative to the beginning of MMIO space
+ * @param[in] relativeOffset - offset relative the "Error Log
+ * Queue region" = (sizeof(CircularBufferHeader) + UE reserved region)
* @param[in] length - bytes to read
- * @param[in] additionalBoundaryCheck - bytes to add to the boundary check
- * for added restriction
* @return the bytes read
*/
- virtual std::vector<uint8_t>
- wraparoundRead(const uint32_t offset, const uint32_t length,
- const uint32_t additionalBoundaryCheck) = 0;
+ virtual std::vector<uint8_t> wraparoundRead(const uint32_t relativeOffset,
+ const uint32_t length) = 0;
/**
* Read the entry header from shared buffer
*
- * @param[in] offset - offset relative to the beginning of MMIO space
+ * @param[in] relativeOffset - offset relative the "Error Log
+ * Queue region" = (sizeof(CircularBufferHeader) + UE reserved region)
* @return the entry header
*/
- virtual struct QueueEntryHeader readEntryHeader(size_t offset) = 0;
+ virtual struct QueueEntryHeader readEntryHeader(size_t relativeOffset) = 0;
/**
* Read the queue entry from the error log queue
*
- * @param[in] offset - offset relative to the beginning of MMIO space
- * @return entry header and entry pair read from buffer
+ * @param[in] relativeOffset - offset relative the "Error Log
+ * Queue region" = (sizeof(CircularBufferHeader) + UE reserved region)
+ * * @return entry header and entry pair read from buffer
*/
- virtual EntryPair readEntry(size_t offset) = 0;
+ virtual EntryPair readEntry(size_t relativeOffset) = 0;
};
/**
@@ -156,11 +154,10 @@
void readBufferHeader() override;
struct CircularBufferHeader getCachedBufferHeader() const override;
void updateReadPtr(const uint32_t newReadPtr) override;
- std::vector<uint8_t>
- wraparoundRead(const uint32_t offset, const uint32_t length,
- const uint32_t additionalBoundaryCheck = 0) override;
- struct QueueEntryHeader readEntryHeader(size_t offset) override;
- EntryPair readEntry(size_t offset) override;
+ std::vector<uint8_t> wraparoundRead(const uint32_t relativeOffset,
+ const uint32_t length) override;
+ struct QueueEntryHeader readEntryHeader(size_t relativeOffset) override;
+ EntryPair readEntry(size_t relativeOffset) override;
private:
/** @brief The Error log queue starts after the UE region, which is where
diff --git a/include/data_interface.hpp b/include/data_interface.hpp
index bf91817..8292143 100644
--- a/include/data_interface.hpp
+++ b/include/data_interface.hpp
@@ -18,7 +18,7 @@
/**
* Read bytes from shared buffer (blocking call).
*
- * @param[in] offset - offset to read from
+ * @param[in] offset - offset to read from relative to MMIO space
* @param[in] length - number of bytes to read
* @return the bytes read
*/
@@ -28,7 +28,7 @@
/**
* Write bytes to shared buffer.
*
- * @param[in] offset - offset to write to
+ * @param[in] offset - offset to write to relative to MMIO space
* @param[in] bytes - byte vector of data.
* @return return the byte length written
*/
diff --git a/src/buffer.cpp b/src/buffer.cpp
index 04e9861..35a6f77 100644
--- a/src/buffer.cpp
+++ b/src/buffer.cpp
@@ -125,74 +125,89 @@
boost::endian::little_to_native(cachedBufferHeader.ueRegionSize);
}
-std::vector<uint8_t>
- BufferImpl::wraparoundRead(const uint32_t offset, const uint32_t length,
- const uint32_t additionalBoundaryCheck)
+std::vector<uint8_t> BufferImpl::wraparoundRead(const uint32_t relativeOffset,
+ const uint32_t length)
{
const size_t queueSize =
boost::endian::little_to_native(cachedBufferHeader.queueSize);
- if (length + additionalBoundaryCheck > queueSize)
+ if (relativeOffset > queueSize)
+ {
+ throw std::runtime_error(
+ fmt::format("[wraparoundRead] relativeOffset '{}' was bigger "
+ "than queueSize '{}'",
+ relativeOffset, queueSize));
+ }
+ if (length > queueSize)
{
throw std::runtime_error(fmt::format(
- "[wraparoundRead] length '{}' + additionalBoundaryCheck '{}' "
- "was bigger than queueSize '{}'",
- length, additionalBoundaryCheck, queueSize));
+ "[wraparoundRead] length '{}' was bigger than queueSize '{}'",
+ length, queueSize));
}
- // Do a first read up to the end of the buffer (dataInerface->read should
- // only read up to the end of the buffer)
- std::vector<uint8_t> bytesRead = dataInterface->read(offset, length);
- size_t updatedReadOffset = offset + bytesRead.size();
- size_t bytesRemaining = length - bytesRead.size();
+ // Do a calculation to see if the read will wraparound
+ const size_t queueOffset = getQueueOffset();
+ const size_t queueSizeToQueueEnd = queueSize - relativeOffset;
+ size_t numWraparoundBytesToRead = 0;
+ if (length > queueSizeToQueueEnd)
+ {
+ // This means we will wrap, count the bytes that are left to read
+ numWraparoundBytesToRead = length - queueSizeToQueueEnd;
+ }
+ const size_t numBytesToReadTillQueueEnd = length - numWraparoundBytesToRead;
+
+ std::vector<uint8_t> bytesRead = dataInterface->read(
+ queueOffset + relativeOffset, numBytesToReadTillQueueEnd);
+ if (bytesRead.size() != numBytesToReadTillQueueEnd)
+ {
+ throw std::runtime_error(
+ fmt::format("[wraparoundRead] Read '{}' which was not "
+ "the requested length of '{}'",
+ bytesRead.size(), numBytesToReadTillQueueEnd));
+ }
+ size_t updatedReadPtr = relativeOffset + numBytesToReadTillQueueEnd;
// If there are any more bytes to be read beyond the buffer, wrap around and
// read from the beginning of the buffer (offset by the queueOffset)
- size_t queueOffset = getQueueOffset();
- if (bytesRemaining > 0)
+ if (numWraparoundBytesToRead > 0)
{
std::vector<uint8_t> wrappedBytesRead =
- dataInterface->read(queueOffset, bytesRemaining);
- bytesRemaining -= wrappedBytesRead.size();
- if (bytesRemaining != 0)
+ dataInterface->read(queueOffset, numWraparoundBytesToRead);
+ if (numWraparoundBytesToRead != wrappedBytesRead.size())
{
throw std::runtime_error(fmt::format(
- "[wraparoundRead] Buffer wrapped around but was not able to read "
- "all of the requested info. Bytes remaining to read '{}' of '{}'",
- bytesRemaining, length));
+ "[wraparoundRead] Buffer wrapped around but read '{}' which "
+ "was not the requested lenght of '{}'",
+ wrappedBytesRead.size(), numWraparoundBytesToRead));
}
bytesRead.insert(bytesRead.end(), wrappedBytesRead.begin(),
wrappedBytesRead.end());
- updatedReadOffset = wrappedBytesRead.size();
+ updatedReadPtr = numWraparoundBytesToRead;
}
- updateReadPtr(updatedReadOffset);
+ updateReadPtr(updatedReadPtr);
return bytesRead;
}
-struct QueueEntryHeader BufferImpl::readEntryHeader(size_t offset)
+struct QueueEntryHeader BufferImpl::readEntryHeader(size_t relativeOffset)
{
size_t headerSize = sizeof(struct QueueEntryHeader);
// wraparonudRead will throw if it did not read all the bytes, let it
// propagate up the stack
- std::vector<uint8_t> bytesRead = wraparoundRead(offset, headerSize);
+ std::vector<uint8_t> bytesRead = wraparoundRead(relativeOffset, headerSize);
return *reinterpret_cast<struct QueueEntryHeader*>(bytesRead.data());
}
-EntryPair BufferImpl::readEntry(size_t offset)
+EntryPair BufferImpl::readEntry(size_t relativeOffset)
{
- struct QueueEntryHeader entryHeader = readEntryHeader(offset);
-
- size_t entrySize = entryHeader.entrySize;
+ struct QueueEntryHeader entryHeader = readEntryHeader(relativeOffset);
+ size_t entrySize = boost::endian::little_to_native(entryHeader.entrySize);
// wraparonudRead may throw if entrySize was bigger than the buffer or if it
// was not able to read all the bytes, let it propagate up the stack
- // - Use additionalBoundaryCheck parameter to add QueueEntryHeader size to
- // boundary condition calculation
- std::vector<uint8_t> entry =
- wraparoundRead(offset + sizeof(struct QueueEntryHeader), entrySize,
- sizeof(struct QueueEntryHeader));
+ std::vector<uint8_t> entry = wraparoundRead(
+ relativeOffset + sizeof(struct QueueEntryHeader), entrySize);
// Calculate the checksum
uint8_t* entryHeaderPtr = reinterpret_cast<uint8_t*>(&entryHeader);
diff --git a/test/buffer_test.cpp b/test/buffer_test.cpp
index 8649c27..2c928d8 100644
--- a/test/buffer_test.cpp
+++ b/test/buffer_test.cpp
@@ -236,38 +236,53 @@
static constexpr size_t expectedqueueOffset = 0x30 + testUeRegionSize;
};
-TEST_F(BufferWraparoundReadTest, TooBigReadFail)
+TEST_F(BufferWraparoundReadTest, ParamsTooBigFail)
{
InSequence s;
- size_t tooBigLength = testQueueSize + 1;
+
+ size_t tooBigOffset = testQueueSize + 1;
EXPECT_THROW(
try {
- bufferImpl->wraparoundRead(/* offset */ 0, tooBigLength);
+ bufferImpl->wraparoundRead(tooBigOffset, /* length */ 1);
} catch (const std::runtime_error& e) {
EXPECT_STREQ(
e.what(),
- "[wraparoundRead] length '257' + additionalBoundaryCheck '0' "
- "was bigger than queueSize '256'");
+ "[wraparoundRead] relativeOffset '257' was bigger than queueSize '256'");
+ throw;
+ },
+ std::runtime_error);
+
+ size_t tooBigLength = testQueueSize + 1;
+ EXPECT_THROW(
+ try {
+ bufferImpl->wraparoundRead(/* relativeOffset */ 0, tooBigLength);
+ } catch (const std::runtime_error& e) {
+ EXPECT_STREQ(e.what(), "[wraparoundRead] length '257' was bigger "
+ "than queueSize '256'");
throw;
},
std::runtime_error);
}
-TEST_F(BufferWraparoundReadTest, TooBigReadWithAdditionalBoundaryCheckFail)
+TEST_F(BufferWraparoundReadTest, NoWrapAroundReadFails)
{
InSequence s;
- // Use additionalBoundaryCheck to still go over the queueSize by 1
- size_t additionalBoundaryCheck = 10;
- size_t tooBigLength = testQueueSize - additionalBoundaryCheck + 1;
+ size_t testLength = 0x10;
+ size_t testOffset = 0x20;
+
+ // Fail the first read
+ std::vector<std::uint8_t> shortTestBytesRead(testLength - 1);
+ EXPECT_CALL(*dataInterfaceMockPtr,
+ read(testOffset + expectedqueueOffset, testLength))
+ .WillOnce(Return(shortTestBytesRead));
+
EXPECT_THROW(
try {
- bufferImpl->wraparoundRead(/* offset */ 0, tooBigLength,
- additionalBoundaryCheck);
+ bufferImpl->wraparoundRead(testOffset, testLength);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(
- e.what(),
- "[wraparoundRead] length '247' + additionalBoundaryCheck '10' "
- "was bigger than queueSize '256'");
+ EXPECT_STREQ(e.what(),
+ "[wraparoundRead] Read '15' which was not the "
+ "requested length of '16'");
throw;
},
std::runtime_error);
@@ -277,11 +292,12 @@
{
InSequence s;
size_t testLength = 0x10;
- size_t testOffset = 0x50;
+ size_t testOffset = 0x20;
// Successfully read all the requested length without a wrap around
std::vector<std::uint8_t> testBytesRead(testLength);
- EXPECT_CALL(*dataInterfaceMockPtr, read(testOffset, testLength))
+ EXPECT_CALL(*dataInterfaceMockPtr,
+ read(testOffset + expectedqueueOffset, testLength))
.WillOnce(Return(testBytesRead));
// Call to updateReadPtr is triggered
@@ -293,6 +309,11 @@
EXPECT_THAT(bufferImpl->wraparoundRead(testOffset, testLength),
ElementsAreArray(testBytesRead));
+ struct CircularBufferHeader cachedBufferHeader =
+ bufferImpl->getCachedBufferHeader();
+ // The bmcReadPtr should have been updated
+ EXPECT_EQ(boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ testOffset + testLength);
}
TEST_F(BufferWraparoundReadTest, WrapAroundReadFails)
@@ -302,9 +323,10 @@
size_t testLength = 0x10;
size_t testOffset = testQueueSize - (testLength - testBytesLeft);
- // Read 3 bytes short
+ // Read until the end of the queue
std::vector<std::uint8_t> testBytesReadShort(testLength - testBytesLeft);
- EXPECT_CALL(*dataInterfaceMockPtr, read(testOffset, testLength))
+ EXPECT_CALL(*dataInterfaceMockPtr, read(testOffset + expectedqueueOffset,
+ testLength - testBytesLeft))
.WillOnce(Return(testBytesReadShort));
// Read 1 byte short after wraparound
@@ -316,10 +338,10 @@
try {
bufferImpl->wraparoundRead(testOffset, testLength);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(),
- "[wraparoundRead] Buffer wrapped around but was not "
- "able to read all of the requested info. "
- "Bytes remaining to read '1' of '16'");
+ EXPECT_STREQ(
+ e.what(),
+ "[wraparoundRead] Buffer wrapped around but read '2' which was "
+ "not the requested lenght of '3'");
throw;
},
std::runtime_error);
@@ -332,10 +354,11 @@
size_t testLength = 0x10;
size_t testOffset = testQueueSize - (testLength - testBytesLeft);
- // Read 3 bytes short
+ // Read to the end of the queue
std::vector<std::uint8_t> testBytesReadFirst{16, 15, 14, 13, 12, 11, 10,
9, 8, 7, 6, 5, 4};
- EXPECT_CALL(*dataInterfaceMockPtr, read(testOffset, testLength))
+ EXPECT_CALL(*dataInterfaceMockPtr, read(testOffset + expectedqueueOffset,
+ testLength - testBytesLeft))
.WillOnce(Return(testBytesReadFirst));
std::vector<std::uint8_t> testBytesReadSecond{3, 2, 1};
@@ -353,6 +376,11 @@
8, 7, 6, 5, 4, 3, 2, 1};
EXPECT_THAT(bufferImpl->wraparoundRead(testOffset, testLength),
ElementsAreArray(expectedBytes));
+ struct CircularBufferHeader cachedBufferHeader =
+ bufferImpl->getCachedBufferHeader();
+ // The bmcReadPtr should have been updated to reflect the wraparound
+ EXPECT_EQ(boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ testBytesLeft);
}
class BufferEntryTest : public BufferWraparoundReadTest
@@ -367,11 +395,30 @@
}
~BufferEntryTest() override = default;
- void wraparoundReadMock(std::span<std::uint8_t> expetedBytesOutput)
+ void wraparoundReadMock(const uint32_t relativeOffset,
+ std::span<std::uint8_t> expetedBytesOutput)
{
- EXPECT_CALL(*dataInterfaceMockPtr, read(_, _))
- .WillOnce(Return(std::vector<std::uint8_t>(
- expetedBytesOutput.begin(), expetedBytesOutput.end())));
+ InSequence s;
+ const uint32_t queueSizeToQueueEnd = testQueueSize - relativeOffset;
+
+ // This will wrap, split the read mocks in 2
+ if (expetedBytesOutput.size() > queueSizeToQueueEnd)
+ {
+ EXPECT_CALL(*dataInterfaceMockPtr, read(_, _))
+ .WillOnce(Return(std::vector<std::uint8_t>(
+ expetedBytesOutput.begin(),
+ expetedBytesOutput.begin() + queueSizeToQueueEnd)));
+ EXPECT_CALL(*dataInterfaceMockPtr, read(_, _))
+ .WillOnce(Return(std::vector<std::uint8_t>(
+ expetedBytesOutput.begin() + queueSizeToQueueEnd,
+ expetedBytesOutput.end())));
+ }
+ else
+ {
+ EXPECT_CALL(*dataInterfaceMockPtr, read(_, _))
+ .WillOnce(Return(std::vector<std::uint8_t>(
+ expetedBytesOutput.begin(), expetedBytesOutput.end())));
+ }
EXPECT_CALL(*dataInterfaceMockPtr, write(_, _))
.WillOnce(Return(expectedWriteSize));
@@ -383,7 +430,7 @@
// Calculated checksum for the header (0x100 - 0 - 0x20 - 0x01) & 0xff
static constexpr uint8_t testChecksum = 0xdf;
static constexpr uint8_t testRdeCommandType = 0x01;
- size_t testOffset = 0x50;
+ size_t testOffset = 0x20;
struct QueueEntryHeader testEntryHeader
{};
@@ -394,8 +441,13 @@
uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
std::vector<uint8_t> testEntryHeaderVector(
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
- wraparoundReadMock(testEntryHeaderVector);
+ wraparoundReadMock(testOffset, testEntryHeaderVector);
EXPECT_EQ(bufferImpl->readEntryHeader(testOffset), testEntryHeader);
+ // Check the bmcReadPtr
+ struct CircularBufferHeader cachedBufferHeader =
+ bufferImpl->getCachedBufferHeader();
+ EXPECT_EQ(boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ testOffset + testEntryHeaderVector.size());
}
TEST_F(BufferEntryTest, ReadEntryChecksumFail)
@@ -408,9 +460,8 @@
uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
std::vector<uint8_t> testEntryHeaderVector(
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
- wraparoundReadMock(testEntryHeaderVector);
-
- wraparoundReadMock(testEntryVector);
+ wraparoundReadMock(testOffset, testEntryHeaderVector);
+ wraparoundReadMock(testOffset + entryHeaderSize, testEntryVector);
EXPECT_THROW(
try {
bufferImpl->readEntry(testOffset);
@@ -425,19 +476,28 @@
TEST_F(BufferEntryTest, ReadEntryPass)
{
InSequence s;
- // We expect this will bump checksum up by "testEntrySize" = 0x40
- std::vector<uint8_t> testEntryVector(testEntrySize, 2);
- testEntryHeader.checksum -= (0x40);
+ // We expect this will bump checksum up by "testEntrySize" = 0xff * 0x20 =
+ // 0x1fe0 -> 0x1fe0 & 0xff = 0xe0.
+ std::vector<uint8_t> testEntryVector(testEntrySize, 0xff);
+ testEntryHeader.checksum -= (0xe0);
uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
std::vector<uint8_t> testEntryHeaderVector(
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
- wraparoundReadMock(testEntryHeaderVector);
- wraparoundReadMock(testEntryVector);
+ // Set testOffset so that we can test the wraparound here as well on our
+ // second read for the entry (by 1 byte)
+ testOffset = testQueueSize - entryHeaderSize - 1;
+ wraparoundReadMock(testOffset, testEntryHeaderVector);
+ wraparoundReadMock(testOffset + entryHeaderSize, testEntryVector);
EntryPair testedEntryPair;
EXPECT_NO_THROW(testedEntryPair = bufferImpl->readEntry(testOffset));
EXPECT_EQ(testedEntryPair.first, testEntryHeader);
EXPECT_THAT(testedEntryPair.second, ElementsAreArray(testEntryVector));
+ struct CircularBufferHeader cachedBufferHeader =
+ bufferImpl->getCachedBufferHeader();
+ // The bmcReadPtr should have been updated to reflect the wraparound
+ EXPECT_EQ(boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ testEntrySize - 1);
}
} // namespace