pw_kvs: Fix AlignedWrite issues; expand tests
- Update AlignedWriter to prevent writing after an error occurs.
- Ensure that the number of written bytes is always returned for
AlignedWriter errors.
- Remove FlashError::Mode, since the number of bytes affected should
always be set.
- Add tests to alignment_test.cc and key_value_store.cc to cover
failures during AlignedWrite.
- Update some comments.
Change-Id: Id62f1564b3641be0aeb7f799f0fa0538a1b5b09e
diff --git a/pw_kvs/alignment.cc b/pw_kvs/alignment.cc
index cba0e40..6f58788 100644
--- a/pw_kvs/alignment.cc
+++ b/pw_kvs/alignment.cc
@@ -31,11 +31,11 @@
// Always use write_size_ for the bytes written. If there was an error
// assume the space was written or at least disturbed.
bytes_written_ += write_size_;
+ bytes_in_buffer_ = 0;
+
if (!result.ok()) {
return StatusWithSize(result.status(), bytes_written_);
}
-
- bytes_in_buffer_ = 0;
}
}
@@ -45,6 +45,8 @@
StatusWithSize AlignedWriter::Flush() {
static constexpr std::byte kPadByte = std::byte{0};
+ Status status;
+
// If data remains in the buffer, pad it to the alignment size and flush the
// remaining data.
if (bytes_in_buffer_ != 0u) {
@@ -52,17 +54,14 @@
std::memset(&buffer_[bytes_in_buffer_],
int(kPadByte),
remaining_bytes - bytes_in_buffer_);
-
- if (auto result = output_.Write(buffer_, remaining_bytes); !result.ok()) {
- return StatusWithSize(result.status(), bytes_written_);
- }
+ status = output_.Write(buffer_, remaining_bytes).status();
bytes_written_ += remaining_bytes; // Include padding in the total.
+ bytes_in_buffer_ = 0;
}
- const StatusWithSize result(bytes_written_);
+ const StatusWithSize result(status, bytes_written_);
bytes_written_ = 0;
- bytes_in_buffer_ = 0;
return result;
}
diff --git a/pw_kvs/alignment_test.cc b/pw_kvs/alignment_test.cc
index 542428d..e82fee9 100644
--- a/pw_kvs/alignment_test.cc
+++ b/pw_kvs/alignment_test.cc
@@ -17,10 +17,12 @@
#include <string_view>
#include "gtest/gtest.h"
+#include "pw_status/status_with_size.h"
namespace pw::kvs {
namespace {
+using namespace std::string_view_literals;
using std::byte;
TEST(AlignUp, Zero) {
@@ -163,7 +165,6 @@
TEST(AlignedWriter, DestructorFlushes) {
static size_t called_with_bytes;
-
called_with_bytes = 0;
OutputToFunction output([](span<const byte> data) {
@@ -180,5 +181,87 @@
EXPECT_EQ(called_with_bytes, AlignUp(sizeof("What is this?"), 3));
}
+TEST(AlignedWriter, Write_NoFurtherWritesOnFailure) {
+ struct BreakableOutput final : public Output {
+ public:
+ enum { kKeepGoing, kBreakOnNext, kBroken } state = kKeepGoing;
+
+ StatusWithSize Write(span<const byte> data) override {
+ switch (state) {
+ case kKeepGoing:
+ return StatusWithSize(data.size());
+ case kBreakOnNext:
+ state = kBroken;
+ break;
+ case kBroken:
+ ADD_FAILURE();
+ break;
+ }
+ return StatusWithSize(Status::UNKNOWN, data.size());
+ }
+ } output;
+
+ {
+ AlignedWriterBuffer<4> writer(3, output);
+ writer.Write(as_bytes(span("Everything is fine.")));
+ output.state = BreakableOutput::kBreakOnNext;
+ EXPECT_EQ(Status::UNKNOWN,
+ writer.Write(as_bytes(span("No more writes, okay?"))).status());
+ writer.Flush();
+ }
+}
+
+TEST(AlignedWriter, Write_ReturnsTotalBytesWritten) {
+ static Status return_status;
+ return_status = Status::OK;
+
+ OutputToFunction output([](span<const byte> data) {
+ return StatusWithSize(return_status, data.size());
+ });
+
+ AlignedWriterBuffer<22> writer(10, output);
+
+ StatusWithSize result = writer.Write(as_bytes(span("12345678901"sv)));
+ EXPECT_EQ(Status::OK, result.status());
+ EXPECT_EQ(0u, result.size()); // No writes; haven't filled buffer.
+
+ result = writer.Write(as_bytes(span("2345678901"sv)));
+ EXPECT_EQ(Status::OK, result.status());
+ EXPECT_EQ(20u, result.size());
+
+ return_status = Status::PERMISSION_DENIED;
+
+ result = writer.Write(as_bytes(span("2345678901234567890"sv)));
+ EXPECT_EQ(Status::PERMISSION_DENIED, result.status());
+ EXPECT_EQ(40u, result.size());
+}
+
+TEST(AlignedWriter, Flush_Ok_ReturnsTotalBytesWritten) {
+ OutputToFunction output(
+ [](span<const byte> data) { return StatusWithSize(data.size()); });
+
+ AlignedWriterBuffer<4> writer(2, output);
+
+ EXPECT_EQ(Status::OK, writer.Write(as_bytes(span("12345678901"sv))).status());
+
+ StatusWithSize result = writer.Flush();
+ EXPECT_EQ(Status::OK, result.status());
+ EXPECT_EQ(12u, result.size());
+}
+
+TEST(AlignedWriter, Flush_Error_ReturnsTotalBytesWritten) {
+ OutputToFunction output([](span<const byte> data) {
+ return StatusWithSize(Status::ABORTED, data.size());
+ });
+
+ AlignedWriterBuffer<20> writer(10, output);
+
+ EXPECT_EQ(0u, writer.Write(as_bytes(span("12345678901"sv))).size());
+
+ StatusWithSize result = writer.Flush();
+ EXPECT_EQ(Status::ABORTED, result.status());
+ EXPECT_EQ(20u, result.size());
+}
+
} // namespace
} // namespace pw::kvs
diff --git a/pw_kvs/in_memory_fake_flash.cc b/pw_kvs/in_memory_fake_flash.cc
index 1c9b7bc..245e329 100644
--- a/pw_kvs/in_memory_fake_flash.cc
+++ b/pw_kvs/in_memory_fake_flash.cc
@@ -18,40 +18,39 @@
namespace pw::kvs {
-FlashError::Result FlashError::Check(span<FlashError> errors,
- FlashMemory::Address address,
- size_t size) {
+Status FlashError::Check(span<FlashError> errors,
+ FlashMemory::Address address,
+ size_t size) {
for (auto& error : errors) {
- if (Result result = error.Check(address, size); !result.status.ok()) {
- return result;
+ if (Status status = error.Check(address, size); !status.ok()) {
+ return status;
}
}
- return {Status::OK, true};
+ return Status::OK;
}
-FlashError::Result FlashError::Check(FlashMemory::Address start_address,
- size_t size) {
+Status FlashError::Check(FlashMemory::Address start_address, size_t size) {
// Check if the event overlaps with this address range.
if (begin_ != kAnyAddress &&
(start_address >= end_ || (start_address + size) < begin_)) {
- return {Status::OK, true};
+ return Status::OK;
}
if (delay_ > 0u) {
delay_ -= 1;
- return {Status::OK, true};
+ return Status::OK;
}
if (remaining_ == 0u) {
- return {Status::OK, true};
+ return Status::OK;
}
if (remaining_ != kAlways) {
remaining_ -= 1;
}
- return {status_, mode_ != kAbort};
+ return status_;
}
Status InMemoryFakeFlash::Erase(Address address, size_t num_sectors) {
@@ -83,11 +82,8 @@
}
// Check for injected read errors
- auto [status, finish_operation] =
- FlashError::Check(read_errors_, address, output.size());
- if (finish_operation) {
- std::memcpy(output.data(), &buffer_[address], output.size());
- }
+ Status status = FlashError::Check(read_errors_, address, output.size());
+ std::memcpy(output.data(), &buffer_[address], output.size());
return StatusWithSize(status, output.size());
}
@@ -128,11 +124,8 @@
}
// Check for any injected write errors
- auto [status, finish_operation] =
- FlashError::Check(write_errors_, address, data.size());
- if (finish_operation) {
- std::memcpy(&buffer_[address], data.data(), data.size());
- }
+ Status status = FlashError::Check(write_errors_, address, data.size());
+ std::memcpy(&buffer_[address], data.data(), data.size());
return StatusWithSize(status, data.size());
}
diff --git a/pw_kvs/key_value_store_error_handling_test.cc b/pw_kvs/key_value_store_error_handling_test.cc
index 69074d3..0fb59c4 100644
--- a/pw_kvs/key_value_store_error_handling_test.cc
+++ b/pw_kvs/key_value_store_error_handling_test.cc
@@ -242,15 +242,28 @@
EXPECT_EQ(32u, kvs_.GetStorageStats().in_use_bytes);
}
-TEST_F(KvsErrorHandling, Put_WriteFailure_EntryNotAdded) {
+TEST_F(KvsErrorHandling, Put_WriteFailure_EntryNotAddedButBytesMarkedWritten) {
ASSERT_EQ(Status::OK, kvs_.Init());
- flash_.InjectWriteError(FlashError::Unconditional(Status::UNAVAILABLE));
+ flash_.InjectWriteError(FlashError::Unconditional(Status::UNAVAILABLE, 1));
EXPECT_EQ(Status::UNAVAILABLE, kvs_.Put("key1", ByteStr("value1")));
- byte buffer[64];
- EXPECT_EQ(Status::NOT_FOUND, kvs_.Get("key1", buffer).status());
+ EXPECT_EQ(Status::NOT_FOUND, kvs_.Get("key1", span<byte>()).status());
ASSERT_TRUE(kvs_.empty());
+
+ auto stats = kvs_.GetStorageStats();
+ EXPECT_EQ(stats.in_use_bytes, 0u);
+ EXPECT_EQ(stats.reclaimable_bytes, 32u);
+ EXPECT_EQ(stats.writable_bytes, 512u * 3 - 32);
+
+ // The bytes were marked used, so a new key should not overlap with the bytes
+ // from the failed Put.
+ EXPECT_EQ(Status::OK, kvs_.Put("key1", ByteStr("value1")));
+
+ stats = kvs_.GetStorageStats();
+ EXPECT_EQ(stats.in_use_bytes, 32u);
+ EXPECT_EQ(stats.reclaimable_bytes, 32u);
+ EXPECT_EQ(stats.writable_bytes, 512u * 3 - 32 * 2);
}
} // namespace
diff --git a/pw_kvs/public/pw_kvs/alignment.h b/pw_kvs/public/pw_kvs/alignment.h
index 61b0b8e..0d734d5 100644
--- a/pw_kvs/public/pw_kvs/alignment.h
+++ b/pw_kvs/public/pw_kvs/alignment.h
@@ -107,8 +107,8 @@
AlignedWriterBuffer<kBufferSize> buffer(alignment_bytes, output);
for (const span<const std::byte>& chunk : data) {
- if (StatusWithSize status = buffer.Write(chunk); !status.ok()) {
- return status;
+ if (StatusWithSize result = buffer.Write(chunk); !result.ok()) {
+ return result;
}
}
diff --git a/pw_kvs/public/pw_kvs/in_memory_fake_flash.h b/pw_kvs/public/pw_kvs/in_memory_fake_flash.h
index 2bb385d..5dcb1f9 100644
--- a/pw_kvs/public/pw_kvs/in_memory_fake_flash.h
+++ b/pw_kvs/public/pw_kvs/in_memory_fake_flash.h
@@ -27,42 +27,30 @@
class FlashError {
public:
- enum Mode {
- kAbort,
- kContinueButReportError,
- };
-
static constexpr FlashMemory::Address kAnyAddress = FlashMemory::Address(-1);
static constexpr size_t kAlways = size_t(-1);
// Creates a FlashError that always triggers on the next operation.
static constexpr FlashError Unconditional(Status status,
- Mode mode = kAbort,
size_t times = kAlways,
size_t delay = 0) {
- return FlashError(status, kAnyAddress, 0, mode, times, delay);
+ return FlashError(status, kAnyAddress, 0, times, delay);
}
// Creates a FlashError that triggers for particular addresses.
static constexpr FlashError InRange(Status status,
FlashMemory::Address address,
size_t size = 1,
- Mode mode = kAbort,
size_t times = kAlways,
size_t delay = 0) {
- return FlashError(status, address, size, mode, times, delay);
+ return FlashError(status, address, size, times, delay);
}
- struct Result {
- Status status; // what to return from the operation
- bool finish_operation; // whether to complete the operation
- };
-
// Determines if this FlashError applies to the operation.
- Result Check(FlashMemory::Address start_address, size_t size);
+ Status Check(FlashMemory::Address start_address, size_t size);
// Determines if any of a series of FlashErrors applies to the operation.
- static Result Check(span<FlashError> errors,
+ static Status Check(span<FlashError> errors,
FlashMemory::Address address,
size_t size);
@@ -70,13 +58,11 @@
constexpr FlashError(Status status,
FlashMemory::Address address,
size_t size, // not used if address is kAnyAddress
- Mode mode,
size_t times,
size_t delay)
: status_(status),
begin_(address),
end_(address + size), // not used if address is kAnyAddress
- mode_(mode),
delay_(delay),
remaining_(times) {}
@@ -85,8 +71,6 @@
const FlashMemory::Address begin_;
const FlashMemory::Address end_; // exclusive
- const Mode mode_;
-
size_t delay_;
size_t remaining_;
};
diff --git a/pw_kvs/public/pw_kvs/key_value_store.h b/pw_kvs/public/pw_kvs/key_value_store.h
index 1cf60d7..acd0955 100644
--- a/pw_kvs/public/pw_kvs/key_value_store.h
+++ b/pw_kvs/public/pw_kvs/key_value_store.h
@@ -33,16 +33,23 @@
namespace pw::kvs {
-// TODO: Select the appropriate defaults, add descriptions.
struct Options {
+ // Perform garbage collection if necessary when writing. If true, garbage
+ // collection is attempted if space for an entry cannot be found. This is a
+ // relatively lengthy operation. If false, Put calls that would require
+ // garbage collection fail with RESOURCE_EXHAUSTED.
bool partial_gc_on_write = true;
+
+ // Verify an entry's checksum after reading it from flash.
bool verify_on_read = true;
+
+ // Verify an in-flash entry's checksum after writing it.
bool verify_on_write = true;
};
class KeyValueStore {
public:
- // TODO: Make this configurable.
+ // TODO: Rework entry relocation to not need a large buffer.
static constexpr size_t kWorkingBufferSizeBytes = (4 * 1024);
// KeyValueStores are declared as instances of
@@ -226,7 +233,7 @@
using KeyDescriptor = internal::KeyDescriptor;
using SectorDescriptor = internal::SectorDescriptor;
- // In the future, will be able to provide additional EntryHeaderFormats for
+ // In the future, will be able to provide additional EntryFormats for
// backwards compatibility.
KeyValueStore(FlashPartition* partition,
Vector<KeyDescriptor>& key_descriptor_list,
