From d99edbffbc3a978c8819ac6ca7bae654651fbd31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Wed, 15 Sep 2021 08:05:19 +0200 Subject: [PATCH] [Zephyr] Erase key-value store on factory reset (#9683) * [Zephyr] Erase key-value store on factory reset ConfigurationManager::InitiateFactoryReset() method clears all keys added by ConfigurationManager methods, and OpenThread settings. However, the method was not updated when the generic KeyValueStoreManager was added, hence not all persistent data would be erased upon the factory reset. * Restyled by clang-format * Address code review comment Co-authored-by: Restyled.io --- src/include/platform/CHIPDeviceLayer.h | 1 + .../Zephyr/ConfigurationManagerImpl.cpp | 4 +- .../Zephyr/KeyValueStoreManagerImpl.cpp | 84 ++++++++++++++----- .../Zephyr/KeyValueStoreManagerImpl.h | 2 + src/platform/Zephyr/ZephyrConfig.cpp | 9 +- .../nrfconnect/CHIPDevicePlatformConfig.h | 5 ++ .../telink/CHIPDevicePlatformConfig.h | 5 ++ src/platform/tests/TestKeyValueStoreMgr.cpp | 28 ++++++- 8 files changed, 107 insertions(+), 31 deletions(-) diff --git a/src/include/platform/CHIPDeviceLayer.h b/src/include/platform/CHIPDeviceLayer.h index f285cb1917d40c..cce60cc6061ef6 100644 --- a/src/include/platform/CHIPDeviceLayer.h +++ b/src/include/platform/CHIPDeviceLayer.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include diff --git a/src/platform/Zephyr/ConfigurationManagerImpl.cpp b/src/platform/Zephyr/ConfigurationManagerImpl.cpp index 99211c882b5879..2ca0a1894cdfdd 100644 --- a/src/platform/Zephyr/ConfigurationManagerImpl.cpp +++ b/src/platform/Zephyr/ConfigurationManagerImpl.cpp @@ -90,10 +90,10 @@ void ConfigurationManagerImpl::_InitiateFactoryReset() void ConfigurationManagerImpl::DoFactoryReset(intptr_t arg) { ChipLogProgress(DeviceLayer, "Performing factory reset"); - const CHIP_ERROR err = FactoryResetConfig(); + const CHIP_ERROR err = PersistedStorage::KeyValueStoreMgrImpl().DoFactoryReset(); if (err != CHIP_NO_ERROR) - ChipLogError(DeviceLayer, "FactoryResetConfig() failed: %s", ErrorStr(err)); + ChipLogError(DeviceLayer, "Factory reset failed: %s", ErrorStr(err)); #if CHIP_DEVICE_CONFIG_ENABLE_THREAD ThreadStackMgr().ErasePersistentInfo(); diff --git a/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp b/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp index dd28022f18430a..cd27bc68bb556a 100644 --- a/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp +++ b/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp @@ -21,26 +21,18 @@ * Platform-specific key value storage implementation for Zephyr */ +#include + #include #include -#include +#include -#include #include -#include - namespace chip { namespace DeviceLayer { namespace PersistedStorage { - -KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance; -const char * kChipKeyPrefix = "ch/"; - -void KeyValueStoreManagerImpl::Init() -{ - VerifyOrDie(settings_subsys_init() == 0); -} +namespace { struct ReadEntry { @@ -50,9 +42,22 @@ struct ReadEntry CHIP_ERROR result; // [out] read result }; -static int LoadEntryCallback(const char * name, size_t entrySize, settings_read_cb readCb, void * cbArg, void * param) +struct DeleteSubtreeEntry { - ReadEntry & entry = *reinterpret_cast(param); + CHIP_ERROR result; +}; + +// Prefix the input key with CHIP_DEVICE_CONFIG_SETTINGS_KEY "/" +CHIP_ERROR MakeFullKey(char (&fullKey)[SETTINGS_MAX_NAME_LEN + 1], const char * key) +{ + const int result = snprintf(fullKey, sizeof(fullKey), CHIP_DEVICE_CONFIG_SETTINGS_KEY "/%s", key); + VerifyOrReturnError(result > 0 && static_cast(result) < sizeof(fullKey), CHIP_ERROR_INVALID_ARGUMENT); + return CHIP_NO_ERROR; +} + +int LoadEntryCallback(const char * name, size_t entrySize, settings_read_cb readCb, void * cbArg, void * param) +{ + ReadEntry & entry = *static_cast(param); // If requested a key X, process just node X and ignore all its descendants: X/* if (settings_name_next(name, nullptr) > 0) @@ -74,6 +79,29 @@ static int LoadEntryCallback(const char * name, size_t entrySize, settings_read_ return 0; } +int DeleteSubtreeCallback(const char * name, size_t /* entrySize */, settings_read_cb /* readCb */, void * /* cbArg */, + void * param) +{ + DeleteSubtreeEntry & entry = *static_cast(param); + const CHIP_ERROR error = KeyValueStoreMgr().Delete(name); + + if (entry.result == CHIP_NO_ERROR) + { + entry.result = error; + } + + return 0; +} + +} // namespace + +KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance; + +void KeyValueStoreManagerImpl::Init() +{ + VerifyOrDie(settings_subsys_init() == 0); +} + CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t value_size, size_t * read_bytes_size, size_t offset_bytes) const { @@ -83,11 +111,11 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t // Support can be added in the future if this is needed. VerifyOrReturnError(offset_bytes == 0, CHIP_ERROR_NOT_IMPLEMENTED); - // Concatenate key with the "chip/" prefix to separate it from the other Zephyr settings. - std::string fullKey(std::string(kChipKeyPrefix) + key); + char fullKey[SETTINGS_MAX_NAME_LEN + 1]; + ReturnErrorOnFailure(MakeFullKey(fullKey, key)); ReadEntry entry{ value, value_size, 0, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND }; - settings_load_subtree_direct(fullKey.c_str(), LoadEntryCallback, &entry); + settings_load_subtree_direct(fullKey, LoadEntryCallback, &entry); // Assign readSize only in case read_bytes_size is not nullptr, as it is optional argument if (read_bytes_size) @@ -102,24 +130,34 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, VerifyOrReturnError(value, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(value_size > 0, CHIP_ERROR_INVALID_ARGUMENT); - // Concatenate key with the "chip/" prefix to separate it from the other Zephyr settings. - std::string fullKey(std::string(kChipKeyPrefix) + key); - VerifyOrReturnError(settings_save_one(fullKey.c_str(), value, value_size) == 0, CHIP_ERROR_PERSISTED_STORAGE_FAILED); + char fullKey[SETTINGS_MAX_NAME_LEN + 1]; + ReturnErrorOnFailure(MakeFullKey(fullKey, key)); + + VerifyOrReturnError(settings_save_one(fullKey, value, value_size) == 0, CHIP_ERROR_PERSISTED_STORAGE_FAILED); return CHIP_NO_ERROR; } CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) { - // Concatenate key with the "chip/" prefix to separate it from the other Zephyr settings. - std::string fullKey(std::string(kChipKeyPrefix) + key); + char fullKey[SETTINGS_MAX_NAME_LEN + 1]; + ReturnErrorOnFailure(MakeFullKey(fullKey, key)); - if (settings_delete(fullKey.c_str()) != 0) + if (settings_delete(fullKey) != 0) return CHIP_ERROR_PERSISTED_STORAGE_FAILED; return CHIP_NO_ERROR; } +CHIP_ERROR KeyValueStoreManagerImpl::DoFactoryReset() +{ + DeleteSubtreeEntry entry{ CHIP_NO_ERROR }; + const int result = settings_load_subtree_direct(CHIP_DEVICE_CONFIG_SETTINGS_KEY, DeleteSubtreeCallback, &entry); + + VerifyOrReturnError(result == 0, System::MapErrorZephyr(result)); + return entry.result; +} + } // namespace PersistedStorage } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Zephyr/KeyValueStoreManagerImpl.h b/src/platform/Zephyr/KeyValueStoreManagerImpl.h index 75639f0dcd086b..daaba1144b497a 100644 --- a/src/platform/Zephyr/KeyValueStoreManagerImpl.h +++ b/src/platform/Zephyr/KeyValueStoreManagerImpl.h @@ -47,6 +47,8 @@ class KeyValueStoreManagerImpl final : public KeyValueStoreManager CHIP_ERROR _Put(const char * key, const void * value, size_t value_size); + CHIP_ERROR DoFactoryReset(); + private: // ===== Members for internal use by the following friends. diff --git a/src/platform/Zephyr/ZephyrConfig.cpp b/src/platform/Zephyr/ZephyrConfig.cpp index bd0d11453130c9..9d82e88b6db333 100644 --- a/src/platform/Zephyr/ZephyrConfig.cpp +++ b/src/platform/Zephyr/ZephyrConfig.cpp @@ -41,11 +41,12 @@ namespace Internal { (key); \ static_assert(sizeof(key) <= SETTINGS_MAX_NAME_LEN, "Config key too long: " key) -// Config namespaces +// Define the configuration keys to be part of the CHIP_DEVICE_CONFIG_SETTINGS_KEY subtree +// so that they get erased when KeyValueStoreManagerImpl::DoFactoryReset() is called. // clang-format off -#define NAMESPACE_FACTORY "chip-fact/" -#define NAMESPACE_CONFIG "chip-conf/" -#define NAMESPACE_COUNTERS "chip-cntr/" +#define NAMESPACE_FACTORY CHIP_DEVICE_CONFIG_SETTINGS_KEY "/fct/" +#define NAMESPACE_CONFIG CHIP_DEVICE_CONFIG_SETTINGS_KEY "/cfg/" +#define NAMESPACE_COUNTERS CHIP_DEVICE_CONFIG_SETTINGS_KEY "/ctr/" // clang-format on // Keys stored in the chip factory nam diff --git a/src/platform/nrfconnect/CHIPDevicePlatformConfig.h b/src/platform/nrfconnect/CHIPDevicePlatformConfig.h index e01be4fbc944fc..d788fc6cd0db97 100644 --- a/src/platform/nrfconnect/CHIPDevicePlatformConfig.h +++ b/src/platform/nrfconnect/CHIPDevicePlatformConfig.h @@ -47,6 +47,11 @@ // These are configuration options that are unique to Zephyr platforms. // These can be overridden by the application as needed. +#ifndef CHIP_DEVICE_CONFIG_SETTINGS_KEY +/// Key for all Matter persistent data stored using the Zephyr Settings API +#define CHIP_DEVICE_CONFIG_SETTINGS_KEY "mt" +#endif // CHIP_DEVICE_CONFIG_SETTINGS_KEY + // ========== Platform-specific Configuration Overrides ========= #ifndef CHIP_DEVICE_CONFIG_CHIP_TASK_PRIORITY diff --git a/src/platform/telink/CHIPDevicePlatformConfig.h b/src/platform/telink/CHIPDevicePlatformConfig.h index 84e43f65ddf3b4..53b146c5309eba 100644 --- a/src/platform/telink/CHIPDevicePlatformConfig.h +++ b/src/platform/telink/CHIPDevicePlatformConfig.h @@ -47,6 +47,11 @@ // These are configuration options that are unique to Zephyr platforms. // These can be overridden by the application as needed. +#ifndef CHIP_DEVICE_CONFIG_SETTINGS_KEY +/// Key for all Matter persistent data stored using the Zephyr Settings API +#define CHIP_DEVICE_CONFIG_SETTINGS_KEY "mt" +#endif // CHIP_DEVICE_CONFIG_SETTINGS_KEY + // ========== Platform-specific Configuration Overrides ========= #ifndef CHIP_DEVICE_CONFIG_CHIP_TASK_PRIORITY diff --git a/src/platform/tests/TestKeyValueStoreMgr.cpp b/src/platform/tests/TestKeyValueStoreMgr.cpp index 94e87d3d91f73b..6c1efa54445666 100644 --- a/src/platform/tests/TestKeyValueStoreMgr.cpp +++ b/src/platform/tests/TestKeyValueStoreMgr.cpp @@ -79,8 +79,8 @@ static void TestKeyValueStoreMgr_StringKey(nlTestSuite * inSuite, void * inConte static void TestKeyValueStoreMgr_Uint32Key(nlTestSuite * inSuite, void * inContext) { CHIP_ERROR err; - const char * kTestKey = "uint32_key"; - const char kTestValue = 5; + const char * kTestKey = "uint32_key"; + const uint32_t kTestValue = 5; uint32_t read_value; err = KeyValueStoreMgr().Put(kTestKey, kTestValue); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -199,6 +199,27 @@ static void TestKeyValueStoreMgr_MultiReadKey(nlTestSuite * inSuite, void * inCo NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); } +#ifdef __ZEPHYR__ +static void TestKeyValueStoreMgr_DoFactoryReset(nlTestSuite * inSuite, void * inContext) +{ + constexpr const char * kStrKey = "some_string_key"; + constexpr const char * kUintKey = "some_uint_key"; + + NL_TEST_ASSERT(inSuite, KeyValueStoreMgr().Put(kStrKey, "some_string") == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, KeyValueStoreMgr().Put(kUintKey, uint32_t(1234)) == CHIP_NO_ERROR); + + char readString[16]; + uint32_t readValue; + + NL_TEST_ASSERT(inSuite, KeyValueStoreMgr().Get(kStrKey, readString, sizeof(readString)) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, KeyValueStoreMgr().Get(kUintKey, &readValue) == CHIP_NO_ERROR); + + NL_TEST_ASSERT(inSuite, KeyValueStoreMgrImpl().DoFactoryReset() == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, + KeyValueStoreMgr().Get(kStrKey, readString, sizeof(readString)) == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + NL_TEST_ASSERT(inSuite, KeyValueStoreMgr().Get(kUintKey, &readValue) == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); +} +#endif /** * Test Suite. It lists all the test functions. */ @@ -212,6 +233,9 @@ static const nlTest sTests[] = { NL_TEST_DEF("Test KeyValueStoreMgr_EmptyStringK #ifndef __ZEPHYR__ // Zephyr platform does not support partial or offset reads yet. NL_TEST_DEF("Test KeyValueStoreMgr_MultiReadKey", TestKeyValueStoreMgr_MultiReadKey), +#endif +#ifdef __ZEPHYR__ + NL_TEST_DEF("Test TestKeyValueStoreMgr_DoFactoryReset", TestKeyValueStoreMgr_DoFactoryReset), #endif NL_TEST_SENTINEL() };