buffer: Fix a corner case with header wraparound
Update the unit test so that we cover this corner case + update the API
so that we no longer pass in the offset manually.
Tested: Unit test coverage + integration test with our downstream
platform that found this issue
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: I6b0a7974f902e0248bfb4932237176df20fdaf61
diff --git a/include/buffer.hpp b/include/buffer.hpp
index a44e504..9dba34c 100644
--- a/include/buffer.hpp
+++ b/include/buffer.hpp
@@ -134,22 +134,18 @@
virtual std::vector<uint8_t> wraparoundRead(const uint32_t relativeOffset,
const uint32_t length) = 0;
/**
- * Read the entry header from shared buffer
+ * Read the entry header from shared buffer from the read pointer
*
- * @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 relativeOffset) = 0;
+ virtual struct QueueEntryHeader readEntryHeader() = 0;
/**
- * Read the queue entry from the error log queue
+ * Read the queue entry from the error log queue from the read pointer
*
- * @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 relativeOffset) = 0;
+ virtual EntryPair readEntry() = 0;
/**
* Read the buffer - this API should be used instead of individual functions
@@ -179,8 +175,8 @@
void updateBmcFlags(const uint32_t newBmcFlag) 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;
+ struct QueueEntryHeader readEntryHeader() override;
+ EntryPair readEntry() override;
std::vector<EntryPair> readErrorLogs() override;
private:
diff --git a/src/buffer.cpp b/src/buffer.cpp
index b0922d7..275a8c6 100644
--- a/src/buffer.cpp
+++ b/src/buffer.cpp
@@ -218,25 +218,28 @@
return bytesRead;
}
-struct QueueEntryHeader BufferImpl::readEntryHeader(size_t relativeOffset)
+struct QueueEntryHeader BufferImpl::readEntryHeader()
{
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(relativeOffset, headerSize);
+ std::vector<uint8_t> bytesRead = wraparoundRead(
+ boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ headerSize);
return *reinterpret_cast<struct QueueEntryHeader*>(bytesRead.data());
}
-EntryPair BufferImpl::readEntry(size_t relativeOffset)
+EntryPair BufferImpl::readEntry()
{
- struct QueueEntryHeader entryHeader = readEntryHeader(relativeOffset);
+ struct QueueEntryHeader entryHeader = readEntryHeader();
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
std::vector<uint8_t> entry = wraparoundRead(
- relativeOffset + sizeof(struct QueueEntryHeader), entrySize);
+ boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
+ entrySize);
// Calculate the checksum
uint8_t* entryHeaderPtr = reinterpret_cast<uint8_t*>(&entryHeader);
@@ -304,7 +307,7 @@
std::vector<EntryPair> entryPairs;
while (byteRead < bytesToRead)
{
- EntryPair entryPair = readEntry(currentReadPtr);
+ EntryPair entryPair = readEntry();
byteRead += sizeof(struct QueueEntryHeader) + entryPair.second.size();
entryPairs.push_back(entryPair);
diff --git a/test/buffer_test.cpp b/test/buffer_test.cpp
index fce322b..0fff5ee 100644
--- a/test/buffer_test.cpp
+++ b/test/buffer_test.cpp
@@ -503,7 +503,7 @@
// Calculated checksum for the header
static constexpr uint8_t testChecksum =
(testSequenceId ^ testEntrySize ^ testRdeCommandType);
- size_t testOffset = 0x20;
+ size_t testOffset = 0x0;
struct QueueEntryHeader testEntryHeader
{};
@@ -515,7 +515,7 @@
std::vector<uint8_t> testEntryHeaderVector(
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
wraparoundReadMock(testOffset, testEntryHeaderVector);
- EXPECT_EQ(bufferImpl->readEntryHeader(testOffset), testEntryHeader);
+ EXPECT_EQ(bufferImpl->readEntryHeader(), testEntryHeader);
// Check the bmcReadPtr
struct CircularBufferHeader cachedBufferHeader =
bufferImpl->getCachedBufferHeader();
@@ -535,9 +535,7 @@
wraparoundReadMock(testOffset, testEntryHeaderVector);
wraparoundReadMock(testOffset + entryHeaderSize, testEntryVector);
EXPECT_THROW(
- try {
- bufferImpl->readEntry(testOffset);
- } catch (const std::runtime_error& e) {
+ try { bufferImpl->readEntry(); } catch (const std::runtime_error& e) {
// Calculation: testChecksum (0x21) XOR (0x22) = 3
EXPECT_STREQ(e.what(),
"[readEntry] Checksum was '3', expected '0'");
@@ -546,7 +544,7 @@
std::runtime_error);
}
-TEST_F(BufferEntryTest, ReadEntryPass)
+TEST_F(BufferEntryTest, ReadEntryPassWraparound)
{
InSequence s;
// We expect this will bump checksum up by "testEntrySize" = 0xff ^ 0xff ...
@@ -555,20 +553,42 @@
uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
std::vector<uint8_t> testEntryHeaderVector(
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
- // 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;
+ // Set testOffset so that we can test the wraparound here at the header and
+ // update the readPtr
+ testOffset = testQueueSize - 1;
+ EXPECT_CALL(*dataInterfaceMockPtr, write(expectedBmcReadPtrOffset, _))
+ .WillOnce(Return(expectedWriteSize));
+ EXPECT_NO_THROW(bufferImpl->updateReadPtr(testOffset));
+
wraparoundReadMock(testOffset, testEntryHeaderVector);
wraparoundReadMock(testOffset + entryHeaderSize, testEntryVector);
EntryPair testedEntryPair;
- EXPECT_NO_THROW(testedEntryPair = bufferImpl->readEntry(testOffset));
+ EXPECT_NO_THROW(testedEntryPair = bufferImpl->readEntry());
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),
+ entryHeaderSize + testEntrySize - 1);
+
+ // 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;
+ EXPECT_CALL(*dataInterfaceMockPtr, write(expectedBmcReadPtrOffset, _))
+ .WillOnce(Return(expectedWriteSize));
+ EXPECT_NO_THROW(bufferImpl->updateReadPtr(testOffset));
+
+ wraparoundReadMock(testOffset, testEntryHeaderVector);
+ wraparoundReadMock(testOffset + entryHeaderSize, testEntryVector);
+
+ EXPECT_NO_THROW(testedEntryPair = bufferImpl->readEntry());
+ EXPECT_EQ(testedEntryPair.first, testEntryHeader);
+ EXPECT_THAT(testedEntryPair.second, ElementsAreArray(testEntryVector));
+ cachedBufferHeader = bufferImpl->getCachedBufferHeader();
+ // The bmcReadPtr should have been updated to reflect the wraparound
+ EXPECT_EQ(boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr),
testEntrySize - 1);
}