From 5dd4b068ab883c8079868d5de18f2da632c64f5d Mon Sep 17 00:00:00 2001 From: William Date: Fri, 28 Jul 2023 12:45:52 +0100 Subject: [PATCH] Reverted the AttributePersistenceProvider and added a SafeAttributePersistenceProvider (#28302) * Renamed AttributePersistenceProvider to SafeAttributePersistenceProvider. Reverted to the previous AttributePersistenceProvider. Updated the tests and examples that used the AttributePersistenceProvider. * Restyled by whitespace * Restyled by clang-format * Restyled by gn * Fixed the key generated for safe attributes. Co-authored-by: Tennessee Carmel-Veilleux * Apply documentation suggestions from code review Co-authored-by: Boris Zbarsky * Removed unused aPath and import. * Fixed some docs. Removed size from the SafeAttributePersistenceProvider::SafeReadValue method. * Replaced the aType input var from the SafeAttributePersistenceProvider::SafeReadValue method with documentation on the expectet types. * Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType logic with that used in NumericAttributeTraits::GetNullValue. Mode the GetNullValueForNullableType methods private and commented out the relevant unit tests. * Restyled by clang-format * Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType methods with NumericAttributeTraits. * Apply documentation suggestions from code review Co-authored-by: Boris Zbarsky * Restyled by clang-format * Refactored SafeAttributePersistenceProvider reducing the number of templated functions. Co-authored-by: Boris Zbarsky * Fixed minor bug in suggestions. * Restyled by whitespace --------- Co-authored-by: Restyled.io Co-authored-by: Tennessee Carmel-Veilleux Co-authored-by: Boris Zbarsky --- src/app/AttributePersistenceProvider.h | 170 +---------------- src/app/BUILD.gn | 1 + .../DefaultAttributePersistenceProvider.cpp | 69 ++++++- src/app/DefaultAttributePersistenceProvider.h | 13 +- .../DeferredAttributePersistenceProvider.cpp | 6 +- .../DeferredAttributePersistenceProvider.h | 2 +- src/app/SafeAttributePersistenceProvider.h | 177 ++++++++++++++++++ .../mode-base-server/mode-base-server.cpp | 16 +- .../resource-monitoring-server.cpp | 6 +- src/app/server/Server.cpp | 1 + .../TestAttributePersistenceProvider.cpp | 28 +-- src/app/util/attribute-storage.cpp | 5 +- src/lib/support/DefaultStorageKeyAllocator.h | 8 + 13 files changed, 288 insertions(+), 214 deletions(-) create mode 100644 src/app/SafeAttributePersistenceProvider.h diff --git a/src/app/AttributePersistenceProvider.h b/src/app/AttributePersistenceProvider.h index de2ae5db8fe2e8..527582907ab15e 100644 --- a/src/app/AttributePersistenceProvider.h +++ b/src/app/AttributePersistenceProvider.h @@ -16,19 +16,17 @@ #pragma once #include -#include #include -#include -#include -#include -#include #include namespace chip { namespace app { /** - * Interface for persisting attribute values. + * Interface for persisting attribute values. This will write attributes in storage with platform endianness for scalars + * and uses a different key space from SafeAttributePersistenceProvider. + * When storing cluster attributes that are managed via the AttributeAccessInterface, it is recommended to + * use SafeAttributePersistenceProvider. */ class AttributePersistenceProvider @@ -61,171 +59,17 @@ class AttributePersistenceProvider * Read an attribute value from non-volatile memory. * * @param [in] aPath the attribute path for the data being persisted. - * @param [in] aType the attribute type. - * @param [in] aSize the attribute size. + * @param [in] aMetadata the attribute metadata, as a convenience. * @param [in,out] aValue where to place the data. The size of the buffer - * will be equal to `size`. + * will be equal to `size` member of aMetadata. * * The data is expected to be in native endianness for * integers and floats. For strings, see the string * representation description in the WriteValue * documentation. */ - virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize, + virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue) = 0; - - /** - * Get the KVS representation of null for the given type. - * @tparam T The type for which the null representation should be returned. - * @return A value of type T that in the KVS represents null. - */ - template ::value, bool> = true> - static uint8_t GetNullValueForNullableType() - { - return 0xff; - } - - /** - * Get the KVS representation of null for the given type. - * @tparam T The type for which the null representation should be returned. - * @return A value of type T that in the KVS represents null. - */ - template ::value && !std::is_same::value, bool> = true> - static T GetNullValueForNullableType() - { - T nullValue = 0; - nullValue = T(~nullValue); - return nullValue; - } - - /** - * Get the KVS representation of null for the given type. - * @tparam T The type for which the null representation should be returned. - * @return A value of type T that in the KVS represents null. - */ - template ::value && !std::is_same::value, bool> = true> - static T GetNullValueForNullableType() - { - T shiftBit = 1; - return T(shiftBit << ((sizeof(T) * 8) - 1)); - } - - // 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. - - /** - * Write an attribute value of type intX, uintX or bool to non-volatile memory. - * - * @param [in] aPath the attribute path for the data being written. - * @param [in] aValue the data to write. - */ - template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue) - { - uint8_t value[sizeof(T)]; - auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T)); - w.EndianPut(uint64_t(aValue), sizeof(T)); - - return WriteValue(aPath, ByteSpan(value)); - } - - /** - * Read an attribute of type intX, uintX or bool from non-volatile memory. - * - * @param [in] aPath the attribute path for the data being persisted. - * @param [in,out] aValue where to place the data. - */ - template ::value, bool> = true> - CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue) - { - uint8_t attrData[sizeof(T)]; - MutableByteSpan tempVal(attrData); - // **Note** aType in the ReadValue function is only used to check if the value is of a string type. Since this template - // function is only enabled for integral values, we know that this case will not occur, so we can pass the enum of an - // arbitrary integral type. 0x20 is the ZCL enum type for ZCL_INT8U_ATTRIBUTE_TYPE. - auto err = ReadValue(aPath, 0x20, sizeof(T), tempVal); - if (err != CHIP_NO_ERROR) - { - return err; - } - - chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size()); - r.RawReadLowLevelBeCareful(&aValue); - return r.StatusCode(); - } - - /** - * Write an attribute value of type nullable intX, uintX or bool to non-volatile memory. - * - * @param [in] aPath the attribute path for the data being written. - * @param [in] aValue the data to write. - */ - template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) - { - if (aValue.IsNull()) - { - auto nullVal = GetNullValueForNullableType(); - return WriteScalarValue(aPath, nullVal); - } - return WriteScalarValue(aPath, aValue.Value()); - } - - /** - * Read an attribute of type nullable intX, uintX from non-volatile memory. - * - * @param [in] aPath the attribute path for the data being persisted. - * @param [in,out] aValue where to place the data. - */ - template ::value && !std::is_same::value, bool> = true> - CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) - { - T tempIntegral; - - CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral); - if (err != CHIP_NO_ERROR) - { - return err; - } - - if (tempIntegral == GetNullValueForNullableType()) - { - aValue.SetNull(); - return CHIP_NO_ERROR; - } - - aValue.SetNonNull(tempIntegral); - return CHIP_NO_ERROR; - } - - /** - * Read an attribute of type nullable bool from non-volatile memory. - * - * @param [in] aPath the attribute path for the data being persisted. - * @param [in,out] aValue where to place the data. - */ - template ::value, bool> = true> - CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) - { - uint8_t tempIntegral; - - CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral); - if (err != CHIP_NO_ERROR) - { - return err; - } - - if (tempIntegral == GetNullValueForNullableType()) - { - aValue.SetNull(); - return CHIP_NO_ERROR; - } - - aValue.SetNonNull(tempIntegral); - return CHIP_NO_ERROR; - } }; /** diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index aade1882cb5f5a..83ffe632da7684 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -183,6 +183,7 @@ static_library("app") { "ReadHandler.cpp", "RequiredPrivilege.cpp", "RequiredPrivilege.h", + "SafeAttributePersistenceProvider.h", "StatusResponse.cpp", "StatusResponse.h", "SubscriptionResumptionStorage.h", diff --git a/src/app/DefaultAttributePersistenceProvider.cpp b/src/app/DefaultAttributePersistenceProvider.cpp index 164eba6d93fbeb..36497704a94a87 100644 --- a/src/app/DefaultAttributePersistenceProvider.cpp +++ b/src/app/DefaultAttributePersistenceProvider.cpp @@ -22,7 +22,7 @@ namespace chip { namespace app { -CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) +CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -33,20 +33,16 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu { return CHIP_ERROR_BUFFER_TOO_SMALL; } - return mStorage->SyncSetKeyValue( - DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(), - aValue.data(), static_cast(aValue.size())); + return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast(aValue.size())); } -CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, - size_t aSize, MutableByteSpan & aValue) +CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, + size_t aSize, MutableByteSpan & aValue) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); uint16_t size = static_cast(min(aValue.size(), static_cast(UINT16_MAX))); - ReturnErrorOnFailure(mStorage->SyncGetKeyValue( - DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(), - aValue.data(), size)); + ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size)); EmberAfAttributeType type = aType; if (emberAfIsStringAttributeType(type)) { @@ -71,12 +67,45 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut return CHIP_NO_ERROR; } +CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) +{ + return InternalWriteValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), + aValue); +} + +CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, + const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue) +{ + return InternalReadValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), + aMetadata->attributeType, aMetadata->size, aValue); +} + +CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) +{ + return InternalWriteValue( + DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue); +} + +CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) +{ + return InternalReadValue( + DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20, + aValue.size(), aValue); +} + namespace { AttributePersistenceProvider * gAttributeSaver = nullptr; } // anonymous namespace +/** + * Gets the global attribute saver. + * + * Note: When storing cluster attributes that are managed via AttributeAccessInterface, it is recommended to + * use SafeAttributePersistenceProvider. See AttributePersistenceProvider and SafeAttributePersistenceProvider + * class documentation for more information. + */ AttributePersistenceProvider * GetAttributePersistenceProvider() { return gAttributeSaver; @@ -90,5 +119,27 @@ void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider) } } +namespace { + +SafeAttributePersistenceProvider * gSafeAttributeSaver = nullptr; + +} // anonymous namespace + +/** + * Gets the global attribute safe saver. + */ +SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider() +{ + return gSafeAttributeSaver; +} + +void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider) +{ + if (aProvider != nullptr) + { + gSafeAttributeSaver = aProvider; + } +} + } // namespace app } // namespace chip diff --git a/src/app/DefaultAttributePersistenceProvider.h b/src/app/DefaultAttributePersistenceProvider.h index ef57cd4fbdf3fd..4db77e8919222c 100644 --- a/src/app/DefaultAttributePersistenceProvider.h +++ b/src/app/DefaultAttributePersistenceProvider.h @@ -16,6 +16,7 @@ #pragma once #include +#include #include #include @@ -30,7 +31,7 @@ namespace app { * of this class, since it can't be constructed automatically without knowing * what PersistentStorageDelegate is to be used. */ -class DefaultAttributePersistenceProvider : public AttributePersistenceProvider +class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, public SafeAttributePersistenceProvider { public: DefaultAttributePersistenceProvider() {} @@ -50,11 +51,19 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider // AttributePersistenceProvider implementation. CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override; - CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize, + CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue) override; + // SafeAttributePersistenceProvider implementation. + CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override; + CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) override; + protected: PersistentStorageDelegate * mStorage; + +private: + CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue); + CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue); }; } // namespace app diff --git a/src/app/DeferredAttributePersistenceProvider.cpp b/src/app/DeferredAttributePersistenceProvider.cpp index 46c1996b81cc58..62d5c446fe2091 100644 --- a/src/app/DeferredAttributePersistenceProvider.cpp +++ b/src/app/DeferredAttributePersistenceProvider.cpp @@ -57,10 +57,10 @@ CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttrib return mPersister.WriteValue(aPath, aValue); } -CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, - size_t aSize, MutableByteSpan & aValue) +CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, + const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue) { - return mPersister.ReadValue(aPath, aType, aSize, aValue); + return mPersister.ReadValue(aPath, aMetadata, aValue); } void DeferredAttributePersistenceProvider::FlushAndScheduleNext() diff --git a/src/app/DeferredAttributePersistenceProvider.h b/src/app/DeferredAttributePersistenceProvider.h index 43f6653e08408c..c1023e9b8b8582 100644 --- a/src/app/DeferredAttributePersistenceProvider.h +++ b/src/app/DeferredAttributePersistenceProvider.h @@ -67,7 +67,7 @@ class DeferredAttributePersistenceProvider : public AttributePersistenceProvider * For other attributes, immediately pass the write operation to the decorated persister. */ CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override; - CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize, + CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue) override; private: diff --git a/src/app/SafeAttributePersistenceProvider.h b/src/app/SafeAttributePersistenceProvider.h new file mode 100644 index 00000000000000..c6cba4e9f7135d --- /dev/null +++ b/src/app/SafeAttributePersistenceProvider.h @@ -0,0 +1,177 @@ +/* + * Copyright (c) 2021 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { + +/** + * Interface for persisting attribute values. This will always write attributes in storage as little-endian + * and uses a different key space from AttributePersistenceProvider. + */ + +class SafeAttributePersistenceProvider +{ +public: + virtual ~SafeAttributePersistenceProvider() = default; + SafeAttributePersistenceProvider() = default; + + // 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. + + /** + * Write an attribute value of type intX, uintX or bool to non-volatile memory. + * + * @param [in] aPath the attribute path for the data being written. + * @param [in] aValue the data to write. + */ + template ::value, bool> = true> + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue) + { + uint8_t value[sizeof(T)]; + auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T)); + w.EndianPut(uint64_t(aValue), sizeof(T)); + + return SafeWriteValue(aPath, ByteSpan(value)); + } + + /** + * Read an attribute of type intX, uintX or bool from non-volatile memory. + * + * @param [in] aPath the attribute path for the data being persisted. + * @param [in,out] aValue where to place the data. + */ + 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; + } + + chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size()); + r.RawReadLowLevelBeCareful(&aValue); + return r.StatusCode(); + } + + /** + * Write an attribute value of type nullable intX, uintX to non-volatile memory. + * + * @param [in] aPath the attribute path for the data being written. + * @param [in] aValue the data to write. + */ + template ::value, bool> = true> + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) + { + typename NumericAttributeTraits::StorageType storageValue; + if (aValue.IsNull()) + { + NumericAttributeTraits::SetNull(storageValue); + } + else + { + NumericAttributeTraits::WorkingToStorage(aValue.Value(), storageValue); + } + return WriteScalarValue(aPath, storageValue); + } + + /** + * Read an attribute of type nullable intX, uintX from non-volatile memory. + * + * @param [in] aPath the attribute path for the data being persisted. + * @param [in,out] aValue where to place the data. + */ + 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; + } + + if (NumericAttributeTraits::IsNullValue(storageValue)) + { + aValue.SetNull(); + return CHIP_NO_ERROR; + } + + // Consider checking CanRepresentValue here, so we don't produce invalid data + // if the storage hands us invalid values. + aValue.SetNonNull(NumericAttributeTraits::StorageToWorking(storageValue)); + 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. + */ + 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. + * + * @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. + */ + virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0; +}; + +/** + * Instance getter for the global SafeAttributePersistenceProvider. + * + * Callers have to externally synchronize usage of this function. + * + * @return The global SafeAttributePersistenceProvider. This must never be null. + */ +SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider(); + +/** + * Instance setter for the global SafeAttributePersistenceProvider. + * + * Callers have to externally synchronize usage of this function. + * + * If the `provider` is nullptr, the value is not changed. + * + * @param[in] aProvider the SafeAttributePersistenceProvider implementation to use. + */ +void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider); + +} // namespace app +} // namespace chip diff --git a/src/app/clusters/mode-base-server/mode-base-server.cpp b/src/app/clusters/mode-base-server/mode-base-server.cpp index f4ec6a46ec7581..d2e56f172627dc 100644 --- a/src/app/clusters/mode-base-server/mode-base-server.cpp +++ b/src/app/clusters/mode-base-server/mode-base-server.cpp @@ -17,8 +17,8 @@ */ #include -#include #include +#include #include #include #include @@ -60,7 +60,7 @@ void Instance::LoadPersistentAttributes() { // Load Current Mode uint8_t tempCurrentMode; - CHIP_ERROR err = GetAttributePersistenceProvider()->ReadScalarValue( + CHIP_ERROR err = GetSafeAttributePersistenceProvider()->ReadScalarValue( ConcreteAttributePath(mEndpointId, mClusterId, Attributes::CurrentMode::Id), tempCurrentMode); if (err == CHIP_NO_ERROR) { @@ -83,7 +83,7 @@ void Instance::LoadPersistentAttributes() // Load Start-Up Mode DataModel::Nullable tempStartUpMode; - err = GetAttributePersistenceProvider()->ReadScalarValue( + err = GetSafeAttributePersistenceProvider()->ReadScalarValue( ConcreteAttributePath(mEndpointId, mClusterId, Attributes::StartUpMode::Id), tempStartUpMode); if (err == CHIP_NO_ERROR) { @@ -111,8 +111,8 @@ void Instance::LoadPersistentAttributes() // Load On Mode DataModel::Nullable tempOnMode; - err = GetAttributePersistenceProvider()->ReadScalarValue(ConcreteAttributePath(mEndpointId, mClusterId, Attributes::OnMode::Id), - tempOnMode); + err = GetSafeAttributePersistenceProvider()->ReadScalarValue( + ConcreteAttributePath(mEndpointId, mClusterId, Attributes::OnMode::Id), tempOnMode); if (err == CHIP_NO_ERROR) { Status status = UpdateOnMode(tempOnMode); @@ -398,7 +398,7 @@ Status Instance::UpdateCurrentMode(uint8_t aNewMode) { // Write new value to persistent storage. ConcreteAttributePath path = ConcreteAttributePath(mEndpointId, mClusterId, Attributes::CurrentMode::Id); - GetAttributePersistenceProvider()->WriteScalarValue(path, mCurrentMode); + GetSafeAttributePersistenceProvider()->WriteScalarValue(path, mCurrentMode); MatterReportingAttributeChangeCallback(path); } return Protocols::InteractionModel::Status::Success; @@ -419,7 +419,7 @@ Status Instance::UpdateStartUpMode(DataModel::Nullable aNewStartUpMode) { // Write new value to persistent storage. ConcreteAttributePath path = ConcreteAttributePath(mEndpointId, mClusterId, Attributes::StartUpMode::Id); - GetAttributePersistenceProvider()->WriteScalarValue(path, mStartUpMode); + GetSafeAttributePersistenceProvider()->WriteScalarValue(path, mStartUpMode); MatterReportingAttributeChangeCallback(path); } return Protocols::InteractionModel::Status::Success; @@ -440,7 +440,7 @@ Status Instance::UpdateOnMode(DataModel::Nullable aNewOnMode) { // Write new value to persistent storage. ConcreteAttributePath path = ConcreteAttributePath(mEndpointId, mClusterId, Attributes::OnMode::Id); - GetAttributePersistenceProvider()->WriteScalarValue(path, mOnMode); + GetSafeAttributePersistenceProvider()->WriteScalarValue(path, mOnMode); MatterReportingAttributeChangeCallback(path); } return Protocols::InteractionModel::Status::Success; diff --git a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp index c5687467d00ca0..8b2f9ea467fc37 100644 --- a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp +++ b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp @@ -17,11 +17,11 @@ */ #include -#include #include #include #include #include +#include #include #include #include @@ -116,7 +116,7 @@ chip::Protocols::InteractionModel::Status Instance::UpdateLastChangedTime(DataMo mLastChangedTime = aNewLastChangedTime; if (mLastChangedTime != oldLastchangedTime) { - chip::app::GetAttributePersistenceProvider()->WriteScalarValue( + chip::app::GetSafeAttributePersistenceProvider()->WriteScalarValue( ConcreteAttributePath(mEndpointId, mClusterId, Attributes::LastChangedTime::Id), mLastChangedTime); MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::LastChangedTime::Id); } @@ -351,7 +351,7 @@ void Instance::HandleCommand(HandlerContext & handlerContext, FuncT func) void Instance::LoadPersistentAttributes() { - CHIP_ERROR err = chip::app::GetAttributePersistenceProvider()->ReadScalarValue( + CHIP_ERROR err = chip::app::GetSafeAttributePersistenceProvider()->ReadScalarValue( ConcreteAttributePath(mEndpointId, mClusterId, Attributes::LastChangedTime::Id), mLastChangedTime); if (err == CHIP_NO_ERROR) { diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 96847ace574100..a4c8b95d550f79 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -150,6 +150,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) // handler. SuccessOrExit(err = mAttributePersister.Init(mDeviceStorage)); SetAttributePersistenceProvider(&mAttributePersister); + SetSafeAttributePersistenceProvider(&mAttributePersister); { FabricTable::InitParams fabricTableInitParams; diff --git a/src/app/tests/TestAttributePersistenceProvider.cpp b/src/app/tests/TestAttributePersistenceProvider.cpp index 045ef77be69d9f..9c75715d14e03d 100644 --- a/src/app/tests/TestAttributePersistenceProvider.cpp +++ b/src/app/tests/TestAttributePersistenceProvider.cpp @@ -70,12 +70,12 @@ void TestStorageAndRetrivalByteSpans(nlTestSuite * inSuite, void * inContext) // Store ByteSpan of size 1 uint8_t valueArray[1] = { 0x42 }; ByteSpan value(valueArray); - err = persistenceProvider.WriteValue(TestConcretePath, value); + err = persistenceProvider.SafeWriteValue(TestConcretePath, value); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); uint8_t getArray[1]; MutableByteSpan valueReadBack(getArray); - err = persistenceProvider.ReadValue(TestConcretePath, 0x20, sizeof(getArray), valueReadBack); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end())); @@ -198,21 +198,6 @@ void TestStorageAndRetrivalSignedScalarValues(nlTestSuite * inSuite, void * inCo persistenceProvider.Shutdown(); } -void TestGetNullFunctions(nlTestSuite * inSuite, void * inContext) -{ - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == uint8_t(0xff)); - - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == uint8_t(0xff)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == uint16_t(0xffff)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == uint32_t(0xffffffff)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == uint64_t(0xffffffffffffffff)); - - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == int8_t(0x80)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == int16_t(0x8000)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == int32_t(0x80000000)); - NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType() == int64_t(0x8000000000000000)); -} - /** * Tests the storage and retrival of data from the KVS of DataModel::Nullable types bool, uint8_t, uint16_t, uint32_t, uint64_t. */ @@ -332,26 +317,26 @@ void TestBufferTooSmallErrors(nlTestSuite * inSuite, void * inContext) // Store large data uint8_t valueArray[9] = { 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42 }; ByteSpan value(valueArray); - err = persistenceProvider.WriteValue(TestConcretePath, value); + err = persistenceProvider.SafeWriteValue(TestConcretePath, value); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); // Confirm the daya is there uint8_t getArray[9]; MutableByteSpan valueReadBack(getArray); - err = persistenceProvider.ReadValue(TestConcretePath, 0x20, sizeof(getArray), valueReadBack); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end())); // Fail to get data as ByteSpace of size 0 uint8_t getArray0[0]; MutableByteSpan valueReadBackByteSpan0(getArray0, 0); - err = persistenceProvider.ReadValue(TestConcretePath, 0x20, 1, valueReadBackByteSpan0); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan0); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); // Fail to get data as ByteSpace of size > 0 but < required uint8_t getArray8[8]; MutableByteSpan valueReadBackByteSpan8(getArray8, sizeof(getArray8)); - err = persistenceProvider.ReadValue(TestConcretePath, 0x20, 1, valueReadBackByteSpan8); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan8); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_BUFFER_TOO_SMALL); // Fail to get value as uint8_t @@ -385,7 +370,6 @@ const nlTest sTests[] = { NL_TEST_DEF("Storage and retrival of ByteSpans", TestStorageAndRetrivalByteSpans), NL_TEST_DEF("Storage and retrival of unsigned scalar values", TestStorageAndRetrivalScalarValues), NL_TEST_DEF("Storage and retrival of signed scalar values", TestStorageAndRetrivalSignedScalarValues), - NL_TEST_DEF("Check GetNullValueForNullableType return values", TestGetNullFunctions), NL_TEST_DEF("Storage and retrival of unsigned nullable scalar values", TestStorageAndRetrivalNullableScalarValues), NL_TEST_DEF("Storage and retrival of signed nullable scalar values", TestStorageAndRetrivalSignedNullableScalarValues), NL_TEST_DEF("Small buffer errors", TestBufferTooSmallErrors), diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index eb873bdc02b8dc..788723a2162bf5 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -1225,9 +1225,8 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional { VerifyOrDie(attrStorage && "Attribute persistence needs a persistence provider"); MutableByteSpan bytes(attrData); - CHIP_ERROR err = - attrStorage->ReadValue(app::ConcreteAttributePath(de->endpoint, cluster->clusterId, am->attributeId), - am->attributeType, am->size, bytes); + CHIP_ERROR err = attrStorage->ReadValue( + app::ConcreteAttributePath(de->endpoint, cluster->clusterId, am->attributeId), am, bytes); if (err == CHIP_NO_ERROR) { ptr = attrData; diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index 9adc8fdf287353..65cd42c1a70063 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -170,6 +170,14 @@ class DefaultStorageKeyAllocator return StorageKeyName::Formatted("g/a/%x/%" PRIx32 "/%" PRIx32, endpointId, clusterId, attributeId); } + // Returns the key for Safely stored attributes. + static StorageKeyName SafeAttributeValue(EndpointId endpointId, ClusterId clusterId, AttributeId attributeId) + { + // Needs at most 26 chars: 6 for "s/a///", 4 for the endpoint id, 8 each + // for the cluster and attribute ids. + return StorageKeyName::Formatted("g/sa/%x/%" PRIx32 "/%" PRIx32, endpointId, clusterId, attributeId); + } + // TODO: Should store fabric-specific parts of the binding list under keys // starting with "f/%x/". static StorageKeyName BindingTable() { return StorageKeyName::FromConst("g/bt"); }