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 <tennessee.carmelveilleux@gmail.com>
* Apply documentation suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* 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 <bzbarsky@apple.com>
* Restyled by clang-format
* Refactored SafeAttributePersistenceProvider reducing the number of templated functions.
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Fixed minor bug in suggestions.
* Restyled by whitespace
---------
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/app/AttributePersistenceProvider.h b/src/app/AttributePersistenceProvider.h
index de2ae5d..5275829 100644
--- a/src/app/AttributePersistenceProvider.h
+++ b/src/app/AttributePersistenceProvider.h
@@ -16,19 +16,17 @@
#pragma once
#include <app/ConcreteAttributePath.h>
-#include <app/data-model/Nullable.h>
#include <app/util/attribute-metadata.h>
-#include <cstring>
-#include <inttypes.h>
-#include <lib/support/BufferReader.h>
-#include <lib/support/BufferWriter.h>
#include <lib/support/Span.h>
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 @@
* 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 <typename T, std::enable_if_t<std::is_same<bool, T>::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 <typename T, std::enable_if_t<std::is_unsigned<T>::value && !std::is_same<bool, T>::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 <typename T, std::enable_if_t<std::is_signed<T>::value && !std::is_same<bool, T>::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 <typename T, std::enable_if_t<std::is_integral<T>::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 <typename T, std::enable_if_t<std::is_integral<T>::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 <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
- CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
- {
- if (aValue.IsNull())
- {
- auto nullVal = GetNullValueForNullableType<T>();
- 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 <typename T, std::enable_if_t<std::is_integral<T>::value && !std::is_same<bool, T>::value, bool> = true>
- CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
- {
- T tempIntegral;
-
- CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
- if (err != CHIP_NO_ERROR)
- {
- return err;
- }
-
- if (tempIntegral == GetNullValueForNullableType<T>())
- {
- 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 <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
- CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
- {
- uint8_t tempIntegral;
-
- CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
- if (err != CHIP_NO_ERROR)
- {
- return err;
- }
-
- if (tempIntegral == GetNullValueForNullableType<T>())
- {
- 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 aade188..83ffe63 100644
--- a/src/app/BUILD.gn
+++ b/src/app/BUILD.gn
@@ -183,6 +183,7 @@
"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 164eba6..3649770 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 @@
{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}
- return mStorage->SyncSetKeyValue(
- DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
- aValue.data(), static_cast<uint16_t>(aValue.size()));
+ return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(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<uint16_t>(min(aValue.size(), static_cast<size_t>(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 @@
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 @@
}
}
+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 ef57cd4..4db77e8 100644
--- a/src/app/DefaultAttributePersistenceProvider.h
+++ b/src/app/DefaultAttributePersistenceProvider.h
@@ -16,6 +16,7 @@
#pragma once
#include <app/AttributePersistenceProvider.h>
+#include <app/SafeAttributePersistenceProvider.h>
#include <lib/core/CHIPPersistentStorageDelegate.h>
#include <lib/support/DefaultStorageKeyAllocator.h>
@@ -30,7 +31,7 @@
* 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 @@
// 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 46c1996..62d5c44 100644
--- a/src/app/DeferredAttributePersistenceProvider.cpp
+++ b/src/app/DeferredAttributePersistenceProvider.cpp
@@ -57,10 +57,10 @@
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 43f6653..c1023e9 100644
--- a/src/app/DeferredAttributePersistenceProvider.h
+++ b/src/app/DeferredAttributePersistenceProvider.h
@@ -67,7 +67,7 @@
* 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 0000000..c6cba4e
--- /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 <app/ConcreteAttributePath.h>
+#include <app/data-model/Nullable.h>
+#include <app/util/attribute-metadata.h>
+#include <cstring>
+#include <inttypes.h>
+#include <lib/support/BufferReader.h>
+#include <lib/support/BufferWriter.h>
+#include <lib/support/Span.h>
+
+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 <typename T, std::enable_if_t<std::is_integral<T>::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 <typename T, std::enable_if_t<std::is_integral<T>::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 <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
+ CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+ {
+ typename NumericAttributeTraits<T>::StorageType storageValue;
+ if (aValue.IsNull())
+ {
+ NumericAttributeTraits<T>::SetNull(storageValue);
+ }
+ else
+ {
+ NumericAttributeTraits<T>::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 <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
+ CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+ {
+ typename NumericAttributeTraits<T>::StorageType storageValue;
+ CHIP_ERROR err = ReadScalarValue(aPath, storageValue);
+ if (err != CHIP_NO_ERROR)
+ {
+ return err;
+ }
+
+ if (NumericAttributeTraits<T>::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<T>::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 f4ec6a4..d2e56f1 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 <app-common/zap-generated/attributes/Accessors.h>
-#include <app/AttributePersistenceProvider.h>
#include <app/InteractionModelEngine.h>
+#include <app/SafeAttributePersistenceProvider.h>
#include <app/clusters/mode-base-server/mode-base-server.h>
#include <app/clusters/on-off-server/on-off-server.h>
#include <app/reporting/reporting.h>
@@ -60,7 +60,7 @@
{
// 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 @@
// Load Start-Up Mode
DataModel::Nullable<uint8_t> tempStartUpMode;
- err = GetAttributePersistenceProvider()->ReadScalarValue(
+ err = GetSafeAttributePersistenceProvider()->ReadScalarValue(
ConcreteAttributePath(mEndpointId, mClusterId, Attributes::StartUpMode::Id), tempStartUpMode);
if (err == CHIP_NO_ERROR)
{
@@ -111,8 +111,8 @@
// Load On Mode
DataModel::Nullable<uint8_t> 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 @@
{
// 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 @@
{
// 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 @@
{
// 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 c568746..8b2f9ea 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 <app/AttributeAccessInterface.h>
-#include <app/AttributePersistenceProvider.h>
#include <app/CommandHandlerInterface.h>
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteClusterPath.h>
#include <app/InteractionModelEngine.h>
+#include <app/SafeAttributePersistenceProvider.h>
#include <app/clusters/resource-monitoring-server/resource-monitoring-cluster-objects.h>
#include <app/clusters/resource-monitoring-server/resource-monitoring-server.h>
#include <app/data-model/Nullable.h>
@@ -116,7 +116,7 @@
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::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 96847ac..a4c8b95 100644
--- a/src/app/server/Server.cpp
+++ b/src/app/server/Server.cpp
@@ -150,6 +150,7 @@
// 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 045ef77..9c75715 100644
--- a/src/app/tests/TestAttributePersistenceProvider.cpp
+++ b/src/app/tests/TestAttributePersistenceProvider.cpp
@@ -70,12 +70,12 @@
// 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 @@
persistenceProvider.Shutdown();
}
-void TestGetNullFunctions(nlTestSuite * inSuite, void * inContext)
-{
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<bool>() == uint8_t(0xff));
-
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<uint8_t>() == uint8_t(0xff));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<uint16_t>() == uint16_t(0xffff));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<uint32_t>() == uint32_t(0xffffffff));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<uint64_t>() == uint64_t(0xffffffffffffffff));
-
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<int8_t>() == int8_t(0x80));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<int16_t>() == int16_t(0x8000));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<int32_t>() == int32_t(0x80000000));
- NL_TEST_ASSERT(inSuite, AttributePersistenceProvider::GetNullValueForNullableType<int64_t>() == 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 @@
// 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 @@
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 eb873bd..788723a 100644
--- a/src/app/util/attribute-storage.cpp
+++ b/src/app/util/attribute-storage.cpp
@@ -1225,9 +1225,8 @@
{
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 9adc8fd..65cd42c 100644
--- a/src/lib/support/DefaultStorageKeyAllocator.h
+++ b/src/lib/support/DefaultStorageKeyAllocator.h
@@ -170,6 +170,14 @@
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"); }
