pw_kvs: Put/Get updates
- Combine the two versions of Put into one template function. Values
that convert to span are converted to byte spans. A span is created
for other objects, if they are trivially copyable.
- Update Item::Get and Item::ValueSize to access the value from the
descriptor instead of the key, avoiding the unnecessary descriptor
search.
Change-Id: I11e7ee62d19c1d67a63174208bd82a5cbf66636b
diff --git a/pw_kvs/key_value_store.cc b/pw_kvs/key_value_store.cc
index faf7944..9aed5fe 100644
--- a/pw_kvs/key_value_store.cc
+++ b/pw_kvs/key_value_store.cc
@@ -345,24 +345,10 @@
const KeyDescriptor* key_descriptor;
TRY_WITH_SIZE(FindExistingKeyDescriptor(key, &key_descriptor));
- Entry entry;
- TRY_WITH_SIZE(Entry::Read(partition_, key_descriptor->address(), &entry));
-
- StatusWithSize result = entry.ReadValue(value_buffer, offset_bytes);
- if (result.ok() && options_.verify_on_read && offset_bytes == 0u) {
- Status verify_result = entry.VerifyChecksum(
- entry_header_format_.checksum, key, value_buffer.first(result.size()));
- if (!verify_result.ok()) {
- std::memset(value_buffer.data(), 0, result.size());
- return StatusWithSize(verify_result, 0);
- }
-
- return StatusWithSize(verify_result, result.size());
- }
- return result;
+ return Get(key, *key_descriptor, value_buffer, offset_bytes);
}
-Status KeyValueStore::Put(string_view key, span<const byte> value) {
+Status KeyValueStore::PutBytes(string_view key, span<const byte> value) {
DBG("Writing key/value; key length=%zu, value length=%zu",
key.size(),
value.size());
@@ -453,26 +439,65 @@
const KeyDescriptor* key_descriptor;
TRY_WITH_SIZE(FindExistingKeyDescriptor(key, &key_descriptor));
- Entry entry;
- TRY_WITH_SIZE(Entry::Read(partition_, key_descriptor->address(), &entry));
+ return ValueSize(*key_descriptor);
+}
- return StatusWithSize(entry.value_size());
+StatusWithSize KeyValueStore::Get(string_view key,
+ const KeyDescriptor& descriptor,
+ span<std::byte> value_buffer,
+ size_t offset_bytes) const {
+ Entry entry;
+ TRY_WITH_SIZE(Entry::Read(partition_, descriptor.address(), &entry));
+
+ StatusWithSize result = entry.ReadValue(value_buffer, offset_bytes);
+ if (result.ok() && options_.verify_on_read && offset_bytes == 0u) {
+ Status verify_result = entry.VerifyChecksum(
+ entry_header_format_.checksum, key, value_buffer.first(result.size()));
+ if (!verify_result.ok()) {
+ std::memset(value_buffer.data(), 0, result.size());
+ return StatusWithSize(verify_result, 0);
+ }
+
+ return StatusWithSize(verify_result, result.size());
+ }
+ return result;
}
Status KeyValueStore::FixedSizeGet(std::string_view key,
- byte* value,
+ void* value,
+ size_t size_bytes) const {
+ TRY(CheckOperation(key));
+
+ const KeyDescriptor* descriptor;
+ TRY(FindExistingKeyDescriptor(key, &descriptor));
+
+ return FixedSizeGet(key, *descriptor, value, size_bytes);
+}
+
+Status KeyValueStore::FixedSizeGet(std::string_view key,
+ const KeyDescriptor& descriptor,
+ void* value,
size_t size_bytes) const {
// Ensure that the size of the stored value matches the size of the type.
// Otherwise, report error. This check avoids potential memory corruption.
- StatusWithSize result = ValueSize(key);
- if (!result.ok()) {
- return result.status();
- }
- if (result.size() != size_bytes) {
- DBG("Requested %zu B read, but value is %zu B", size_bytes, result.size());
+ TRY_ASSIGN(const size_t actual_size, ValueSize(descriptor));
+
+ if (actual_size != size_bytes) {
+ DBG("Requested %zu B read, but value is %zu B", size_bytes, actual_size);
return Status::INVALID_ARGUMENT;
}
- return Get(key, span(value, size_bytes)).status();
+
+ StatusWithSize result =
+ Get(key, descriptor, span(static_cast<byte*>(value), size_bytes), 0);
+
+ return result.status();
+}
+
+StatusWithSize KeyValueStore::ValueSize(const KeyDescriptor& descriptor) const {
+ Entry entry;
+ TRY_WITH_SIZE(Entry::Read(partition_, descriptor.address(), &entry));
+
+ return StatusWithSize(entry.value_size());
}
Status KeyValueStore::CheckOperation(string_view key) const {
diff --git a/pw_kvs/key_value_store_test.cc b/pw_kvs/key_value_store_test.cc
index e914c39..9d0e3d4 100644
--- a/pw_kvs/key_value_store_test.cc
+++ b/pw_kvs/key_value_store_test.cc
@@ -93,6 +93,10 @@
static_assert(ConvertsToSpan<const std::string_view&>());
static_assert(ConvertsToSpan<const std::string_view&&>());
+static_assert(ConvertsToSpan<bool[1]>());
+static_assert(ConvertsToSpan<char[35]>());
+static_assert(ConvertsToSpan<const int[35]>());
+
static_assert(ConvertsToSpan<span<int>>());
static_assert(ConvertsToSpan<span<byte>>());
static_assert(ConvertsToSpan<span<const int*>>());
@@ -284,6 +288,41 @@
EXPECT_EQ(Status::INVALID_ARGUMENT, kvs_.Put("K", big_data));
}
+TEST_F(EmptyInitializedKvs, PutAndGetByValue_ConvertibleToSpan) {
+ constexpr float input[] = {1.0, -3.5};
+ ASSERT_EQ(Status::OK, kvs_.Put("key", input));
+
+ float output[2] = {};
+ ASSERT_EQ(Status::OK, kvs_.Get("key", &output));
+ EXPECT_EQ(input[0], output[0]);
+ EXPECT_EQ(input[1], output[1]);
+}
+
+TEST_F(EmptyInitializedKvs, PutAndGetByValue_Span) {
+ float input[] = {1.0, -3.5};
+ ASSERT_EQ(Status::OK, kvs_.Put("key", span(input)));
+
+ float output[2] = {};
+ ASSERT_EQ(Status::OK, kvs_.Get("key", &output));
+ EXPECT_EQ(input[0], output[0]);
+ EXPECT_EQ(input[1], output[1]);
+}
+
+TEST_F(EmptyInitializedKvs, PutAndGetByValue_NotConvertibleToSpan) {
+ struct TestStruct {
+ double a;
+ bool b;
+ };
+ const TestStruct input{-1234.5, true};
+
+ ASSERT_EQ(Status::OK, kvs_.Put("key", input));
+
+ TestStruct output;
+ ASSERT_EQ(Status::OK, kvs_.Get("key", &output));
+ EXPECT_EQ(input.a, output.a);
+ EXPECT_EQ(input.b, output.b);
+}
+
TEST_F(EmptyInitializedKvs, Get_Simple) {
ASSERT_EQ(Status::OK, kvs_.Put("Charles", as_bytes(span("Mingus"))));
@@ -324,6 +363,30 @@
EXPECT_EQ(0u, result.size());
}
+TEST_F(EmptyInitializedKvs, GetValue) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ uint32_t value = 0;
+ EXPECT_EQ(Status::OK, kvs_.Get("key", &value));
+ EXPECT_EQ(uint32_t(0xfeedbeef), value);
+}
+
+TEST_F(EmptyInitializedKvs, GetValue_TooSmall) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ uint8_t value = 0;
+ EXPECT_EQ(Status::INVALID_ARGUMENT, kvs_.Get("key", &value));
+ EXPECT_EQ(0u, value);
+}
+
+TEST_F(EmptyInitializedKvs, GetValue_TooLarge) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ uint64_t value = 0;
+ EXPECT_EQ(Status::INVALID_ARGUMENT, kvs_.Get("key", &value));
+ EXPECT_EQ(0u, value);
+}
+
TEST_F(EmptyInitializedKvs, Delete_GetDeletedKey_ReturnsNotFound) {
ASSERT_EQ(Status::OK, kvs_.Put("kEy", as_bytes(span("123"))));
ASSERT_EQ(Status::OK, kvs_.Delete("kEy"));
@@ -435,6 +498,36 @@
}
}
+TEST_F(EmptyInitializedKvs, Iteration_GetValue) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ for (KeyValueStore::Item entry : kvs_) {
+ uint32_t value = 0;
+ EXPECT_EQ(Status::OK, entry.Get(&value));
+ EXPECT_EQ(uint32_t(0xfeedbeef), value);
+ }
+}
+
+TEST_F(EmptyInitializedKvs, Iteration_GetValue_TooSmall) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ for (KeyValueStore::Item entry : kvs_) {
+ uint8_t value = 0;
+ EXPECT_EQ(Status::INVALID_ARGUMENT, entry.Get(&value));
+ EXPECT_EQ(0u, value);
+ }
+}
+
+TEST_F(EmptyInitializedKvs, Iteration_GetValue_TooLarge) {
+ ASSERT_EQ(Status::OK, kvs_.Put("key", uint32_t(0xfeedbeef)));
+
+ for (KeyValueStore::Item entry : kvs_) {
+ uint64_t value = 0;
+ EXPECT_EQ(Status::INVALID_ARGUMENT, entry.Get(&value));
+ EXPECT_EQ(0u, value);
+ }
+}
+
TEST_F(EmptyInitializedKvs, Iteration_EmptyAfterDeletion) {
ASSERT_EQ(Status::OK, kvs_.Put("kEy", as_bytes(span("123"))));
ASSERT_EQ(Status::OK, kvs_.Delete("kEy"));
diff --git a/pw_kvs/public/pw_kvs/internal/span_traits.h b/pw_kvs/public/pw_kvs/internal/span_traits.h
index 4a0bd3a..18098ed 100644
--- a/pw_kvs/public/pw_kvs/internal/span_traits.h
+++ b/pw_kvs/public/pw_kvs/internal/span_traits.h
@@ -31,9 +31,10 @@
} // namespace internal
-// Traits class to detect if the type is a span.
+// Traits class to detect if the type converts to a span.
template <typename T>
-using ConvertsToSpan =
- std::bool_constant<internal::ConvertsToSpan<std::remove_reference_t<T>>(0)>;
+struct ConvertsToSpan
+ : public std::bool_constant<
+ internal::ConvertsToSpan<std::remove_reference_t<T>>(0)> {};
} // namespace pw::kvs
diff --git a/pw_kvs/public/pw_kvs/key_value_store.h b/pw_kvs/public/pw_kvs/key_value_store.h
index e605754..5e64146 100644
--- a/pw_kvs/public/pw_kvs/key_value_store.h
+++ b/pw_kvs/public/pw_kvs/key_value_store.h
@@ -90,23 +90,21 @@
size_t offset_bytes = 0) const;
// This overload of Get accepts a pointer to a trivially copyable object.
- // const T& is used instead of T* to prevent arrays from satisfying this
- // overload. To call Get with an array, pass as_writable_bytes(span(array)),
+ // If the value is an array, call Get with as_writable_bytes(span(array)),
// or pass a pointer to the array instead of the array itself.
template <typename Pointer,
typename = std::enable_if_t<std::is_pointer_v<Pointer>>>
Status Get(const std::string_view& key, const Pointer& pointer) const {
using T = std::remove_reference_t<std::remove_pointer_t<Pointer>>;
-
- static_assert(std::is_trivially_copyable<T>(), "Values must be copyable");
- static_assert(!std::is_pointer<T>(), "Values cannot be pointers");
-
- return FixedSizeGet(key, reinterpret_cast<std::byte*>(pointer), sizeof(T));
+ CheckThatObjectCanBePutOrGet<T>();
+ return FixedSizeGet(key, pointer, sizeof(T));
}
// Adds a key-value entry to the KVS. If the key was already present, its
// value is overwritten.
//
+ // The value may be a span of bytes or a trivially copyable object.
+ //
// In the current implementation, all keys in the KVS must have a unique hash.
// If Put is called with a key whose hash matches an existing key, nothing
// is added and ALREADY_EXISTS is returned.
@@ -119,15 +117,14 @@
// FAILED_PRECONDITION: the KVS is not initialized
// INVALID_ARGUMENT: key is empty or too long or value is too large
//
- Status Put(std::string_view key, span<const std::byte> value);
-
- // Adds a key-value entry to the KVS, using an object as the value.
- template <typename T,
- typename = std::enable_if_t<std::is_trivially_copyable_v<T> &&
- !std::is_pointer_v<T> &&
- !ConvertsToSpan<T>::value>>
+ template <typename T>
Status Put(const std::string_view& key, const T& value) {
- return Put(key, as_bytes(span(&value, 1)));
+ if constexpr (ConvertsToSpan<T>::value) {
+ return PutBytes(key, as_bytes(span(value)));
+ } else {
+ CheckThatObjectCanBePutOrGet<T>();
+ return PutBytes(key, as_bytes(span(&value, 1)));
+ }
}
// Removes a key-value entry from the KVS.
@@ -168,18 +165,24 @@
// The key as a null-terminated string.
const char* key() const { return key_buffer_.data(); }
+ // Gets the value referred to by this iterator. Equivalent to
+ // KeyValueStore::Get.
StatusWithSize Get(span<std::byte> value_buffer,
size_t offset_bytes = 0) const {
- return kvs_.Get(key(), value_buffer, offset_bytes);
+ return kvs_.Get(key(), *descriptor_, value_buffer, offset_bytes);
}
template <typename Pointer,
typename = std::enable_if_t<std::is_pointer_v<Pointer>>>
Status Get(const Pointer& pointer) const {
- return kvs_.Get(key(), pointer);
+ using T = std::remove_reference_t<std::remove_pointer_t<Pointer>>;
+ CheckThatObjectCanBePutOrGet<T>();
+ return kvs_.FixedSizeGet(key(), *descriptor_, pointer, sizeof(T));
}
- StatusWithSize ValueSize() const { return kvs_.ValueSize(key()); }
+ // Reads the size of the value referred to by this iterator. Equivalent to
+ // KeyValueStore::ValueSize.
+ StatusWithSize ValueSize() const { return kvs_.ValueSize(*descriptor_); }
private:
friend class iterator;
@@ -272,10 +275,33 @@
const Options& options);
private:
+ template <typename T>
+ static constexpr void CheckThatObjectCanBePutOrGet() {
+ static_assert(
+ std::is_trivially_copyable_v<T> && !std::is_pointer_v<T>,
+ "Only trivially copyable, non-pointer objects may be Put and Get by "
+ "value. Any value may be stored by converting it to a byte span with "
+ "as_bytes(span(&value, 1)) or as_writable_bytes(span(&value, 1)).");
+ }
+
+ Status PutBytes(std::string_view key, span<const std::byte> value);
+
+ StatusWithSize Get(std::string_view key,
+ const KeyDescriptor& descriptor,
+ span<std::byte> value_buffer,
+ size_t offset_bytes) const;
+
Status FixedSizeGet(std::string_view key,
- std::byte* value,
+ void* value,
size_t size_bytes) const;
+ Status FixedSizeGet(std::string_view key,
+ const KeyDescriptor& descriptor,
+ void* value,
+ size_t size_bytes) const;
+
+ StatusWithSize ValueSize(const KeyDescriptor& descriptor) const;
+
Status CheckOperation(std::string_view key) const;
Status FindKeyDescriptor(std::string_view key,
