[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 <[email protected]>
project-chip · Sep 15, 2021 · d99edbf · d99edbf
1 parent c23ccf2
commit d99edbf
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 31 deletions.
diff --git a/src/include/platform/CHIPDeviceLayer.h b/src/include/platform/CHIPDeviceLayer.h
@@ -28,6 +28,7 @@
 #include <platform/ConfigurationManager.h>
 #include <platform/ConnectivityManager.h>
 #include <platform/GeneralUtils.h>
+#include <platform/KeyValueStoreManager.h>
 #include <platform/PlatformManager.h>
 #include <system/SystemClock.h>
 #include <system/SystemLayerImpl.h>

diff --git 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
@@ -21,26 +21,18 @@
  *          Platform-specific key value storage implementation for Zephyr
  */
 
+#include <platform/internal/CHIPDeviceLayerInternal.h>
+
 #include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
-#include <platform/KeyValueStoreManager.h>
+#include <system/SystemError.h>
 
-#include <logging/log.h>
 #include <settings/settings.h>
 
-#include <string>
-
 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<ReadEntry *>(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<size_t>(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<ReadEntry *>(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<DeleteSubtreeEntry *>(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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() };