diff --git a/src/app/DefaultAttributePersistenceProvider.cpp b/src/app/DefaultAttributePersistenceProvider.cpp index 5bef4c11fb8c1a..6e7120999fe519 100644 --- a/src/app/DefaultAttributePersistenceProvider.cpp +++ b/src/app/DefaultAttributePersistenceProvider.cpp @@ -37,34 +37,40 @@ CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const Storage return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast(aValue.size())); } -CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, - size_t aSize, MutableByteSpan & aValue) +CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); uint16_t size = static_cast(min(aValue.size(), static_cast(UINT16_MAX))); ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size)); - EmberAfAttributeType type = aType; - if (emberAfIsStringAttributeType(type)) + aValue.reduce_size(size); + return CHIP_NO_ERROR; +} + +CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, + size_t aExpectedSize, MutableByteSpan & aValue) +{ + ReturnErrorOnFailure(InternalReadValue(aKey, aValue)); + size_t size = aValue.size(); + if (emberAfIsStringAttributeType(aType)) { // Ensure that we've read enough bytes that we are not ending up with // un-initialized memory. Should have read length + 1 (for the length // byte). - VerifyOrReturnError(size >= emberAfStringLength(aValue.data()) + 1, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(size >= 1 && size - 1 >= emberAfStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE); } - else if (emberAfIsLongStringAttributeType(type)) + else if (emberAfIsLongStringAttributeType(aType)) { // Ensure that we've read enough bytes that we are not ending up with // un-initialized memory. Should have read length + 2 (for the length // bytes). - VerifyOrReturnError(size >= emberAfLongStringLength(aValue.data()) + 2, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(size >= 2 && size - 2 >= emberAfLongStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE); } else { // Ensure we got the expected number of bytes for all other types. - VerifyOrReturnError(size == aSize, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(size == aExpectedSize, CHIP_ERROR_INVALID_ARGUMENT); } - aValue.reduce_size(size); return CHIP_NO_ERROR; } @@ -90,8 +96,7 @@ CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAtt CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) { return InternalReadValue( - DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20, - aValue.size(), aValue); + DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue); } namespace { diff --git a/src/app/DefaultAttributePersistenceProvider.h b/src/app/DefaultAttributePersistenceProvider.h index 4db77e8919222c..3e18808f366d37 100644 --- a/src/app/DefaultAttributePersistenceProvider.h +++ b/src/app/DefaultAttributePersistenceProvider.h @@ -63,7 +63,9 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, private: CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue); - CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue); + CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue); + CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aExpectedSize, + MutableByteSpan & aValue); }; } // namespace app diff --git a/src/app/SafeAttributePersistenceProvider.h b/src/app/SafeAttributePersistenceProvider.h index ed02d6039043ad..8d11d95d47b02d 100644 --- a/src/app/SafeAttributePersistenceProvider.h +++ b/src/app/SafeAttributePersistenceProvider.h @@ -40,8 +40,8 @@ class SafeAttributePersistenceProvider // The following API provides helper functions to simplify the access of commonly used types. // The API may not be complete. - // Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and - // their nullable varieties, and bool. + // Currently implemented write and read types are: bool, uint8_t, uint16_t, uint32_t, unit64_t and + // their nullable varieties, as well as ByteSpans. /** * Write an attribute value of type intX, uintX or bool to non-volatile memory. @@ -50,7 +50,7 @@ class SafeAttributePersistenceProvider * @param [in] aValue the data to write. */ template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue) + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T aValue) { uint8_t value[sizeof(T)]; auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T)); @@ -71,18 +71,16 @@ class SafeAttributePersistenceProvider * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute */ template ::value, bool> = true> CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue) { uint8_t attrData[sizeof(T)]; MutableByteSpan tempVal(attrData); - auto err = SafeReadValue(aPath, tempVal); - if (err != CHIP_NO_ERROR) - { - return err; - } - + ReturnErrorOnFailure(SafeReadValue(aPath, tempVal)); + VerifyOrReturnError(tempVal.size() == sizeof(T), CHIP_ERROR_INCORRECT_STATE); Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size()); r.RawReadLowLevelBeCareful(&aValue); return r.StatusCode(); @@ -95,7 +93,7 @@ class SafeAttributePersistenceProvider * @param [in] aValue the data to write. */ template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, const DataModel::Nullable & aValue) { typename NumericAttributeTraits::StorageType storageValue; if (aValue.IsNull()) @@ -114,16 +112,14 @@ class SafeAttributePersistenceProvider * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute */ template ::value, bool> = true> CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) { typename NumericAttributeTraits::StorageType storageValue; - CHIP_ERROR err = ReadScalarValue(aPath, storageValue); - if (err != CHIP_NO_ERROR) - { - return err; - } + ReturnErrorOnFailure(ReadScalarValue(aPath, storageValue)); if (NumericAttributeTraits::IsNullValue(storageValue)) { @@ -137,25 +133,25 @@ class SafeAttributePersistenceProvider return CHIP_NO_ERROR; } -protected: /** * Write an attribute value from the attribute store (i.e. not a struct or * list) to non-volatile memory. * * @param [in] aPath the attribute path for the data being written. - * @param [in] aValue the data to write. The value should be stored as-is. + * @param [in] aValue the data to write. The value will be stored as-is. */ virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) = 0; /** - * Read an attribute value from non-volatile memory. - * It can be assumed that this method will never be called upon to read - * an attribute of type string or long-string. + * Read an attribute value as a raw sequence of bytes from non-volatile memory. * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. The size of the buffer - * will be equal to `aValue.size()`. The callee is expected to adjust - * aValue's size to the actual number of bytes read. + * will be equal to `aValue.size()`. On success aValue.size() + * will be the actual number of bytes read. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute + * @retval CHIP_ERROR_BUFFER_TOO_SMALL aValue.size() is too small to hold the value. */ virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0; }; diff --git a/src/app/tests/TestAttributePersistenceProvider.cpp b/src/app/tests/TestAttributePersistenceProvider.cpp index 7ae40b52402fe0..1b8c7ee3d3872c 100644 --- a/src/app/tests/TestAttributePersistenceProvider.cpp +++ b/src/app/tests/TestAttributePersistenceProvider.cpp @@ -59,7 +59,13 @@ TEST_F(TestAttributePersistenceProvider, TestStorageAndRetrivalByteSpans) MutableByteSpan valueReadBack(getArray); err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack); EXPECT_EQ(err, CHIP_NO_ERROR); - EXPECT_TRUE(std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end())); + EXPECT_TRUE(valueReadBack.data_equal(value)); + + uint8_t getArrayThatIsLongerThanNeeded[10]; + MutableByteSpan valueReadBack2(getArrayThatIsLongerThanNeeded); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack2); + EXPECT_EQ(err, CHIP_NO_ERROR); + EXPECT_TRUE(valueReadBack2.data_equal(value)); // Finishing persistenceProvider.Shutdown(); @@ -320,6 +326,7 @@ TEST_F(TestAttributePersistenceProvider, TestBufferTooSmallErrors) err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan8); EXPECT_EQ(err, CHIP_ERROR_BUFFER_TOO_SMALL); + // TODO: ReadScalarValue() does not take a buffer, so expecting CHIP_ERROR_BUFFER_TOO_SMALL is bad API // Fail to get value as uint8_t uint8_t valueReadBack8; err = persistenceProvider.ReadScalarValue(TestConcretePath, valueReadBack8);