From fe2cb0e1fc808e4c6b2c54e6ad556250e73512f7 Mon Sep 17 00:00:00 2001 From: doru91 Date: Mon, 15 Nov 2021 20:01:04 +0200 Subject: [PATCH] Account for KVS collisions in NVM identifiers (#9489) * Fix KVS implementation Old implementation was not taking into account collissions. The new implementation keeps an unorderded hash having as keys the matter string keys and as values the key ids used as PDM identifiers. Used key ids are saved in a list which is updated at each Put/Delete operation. Also, in order to be able to restore from flash the unordered hash, the matter string keys are saved in flash. Signed-off-by: Doru Gucea * Fixes after review Signed-off-by: Doru Gucea * Restyled by clang-format (#11433) Co-authored-by: Restyled.io Signed-off-by: Doru Gucea * Restyled by clang-format (#11464) Co-authored-by: Restyled.io Signed-off-by: Doru Gucea Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io --- .../k32w/k32w0/KeyValueStoreManagerImpl.cpp | 218 +++++++++++++++--- 1 file changed, 190 insertions(+), 28 deletions(-) diff --git a/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp b/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp index 46ec80d759feab..5753f614a0550a 100644 --- a/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp +++ b/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp @@ -26,37 +26,118 @@ #include #include #include +#include #include #include "PDM.h" +#include + namespace chip { namespace DeviceLayer { namespace PersistedStorage { -/* TODO: adjust this value */ -#define MAX_NO_OF_KEYS 255 +/* TODO: adjust these values */ +constexpr size_t kMaxNumberOfKeys = 20; +constexpr size_t kMaxKeyValueBytes = 255; KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance; +/* hashmap having: + * - the matter key string as key; + * - internal PDM identifier as value; + */ +std::unordered_map g_kvs_map; + +/* set containing used PDM identifiers */ +std::set g_key_ids_set; + +/* used to check if we need to restore values from flash (e.g.: reset) */ +static bool g_restored_from_flash = false; + +CHIP_ERROR RestoreFromFlash() +{ + CHIP_ERROR err = CHIP_NO_ERROR; + uint8_t key_id = 0; + char key_string_id[kMaxKeyValueBytes] = { 0 }; + size_t key_string_id_size = 0; + uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; + uint16_t pdm_internal_id = 0; + + if (g_restored_from_flash) + { + /* already restored from flash, nothing to do */ + return err; + } + + for (key_id = 0; key_id < kMaxNumberOfKeys; key_id++) + { + /* key was saved as string in flash (key_string_id) using (key_id + kMaxNumberOfKeys) as PDM key + * value corresponding to key_string_id was saved in flash using key_id as PDM key + */ + + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys); + err = chip::DeviceLayer::Internal::K32WConfig::ReadConfigValueStr(pdm_internal_id, key_string_id, kMaxKeyValueBytes, + key_string_id_size); + + if (err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) + { + err = CHIP_NO_ERROR; + continue; + } + else if (err != CHIP_NO_ERROR) + { + ChipLogProgress(DeviceLayer, "KVS, Error while restoring Matter key [%s] from flash with PDM id: %i", key_string_id, + pdm_internal_id); + return err; + } + + if (key_string_id_size) + { + g_key_ids_set.insert(key_id); + key_string_id_size = 0; + if (!g_kvs_map.insert(std::make_pair(std::string(key_string_id), key_id)).second) + { + /* key collision is not expected when restoring from flash */ + ChipLogProgress(DeviceLayer, "KVS, Unexpected collision while restoring Matter key [%s] from flash with PDM id: %i", + key_string_id, pdm_internal_id); + } + else + { + ChipLogProgress(DeviceLayer, "KVS, restored Matter key [%s] from flash with PDM id: %i", key_string_id, + pdm_internal_id); + } + } + } + + g_restored_from_flash = true; + + return err; +} + CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t value_size, size_t * read_bytes_size, size_t offset_bytes) { - CHIP_ERROR err = CHIP_NO_ERROR; - std::hash hash_fn; - uint16_t key_id; + CHIP_ERROR err = CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND; uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; - size_t read_bytes; + std::unordered_map::const_iterator it; + size_t read_bytes = 0; + uint8_t key_id = 0; + uint16_t pdm_internal_id = 0; - VerifyOrExit((key != NULL) && (value != NULL), err = CHIP_ERROR_INVALID_ARGUMENT); - key_id = hash_fn(key) % MAX_NO_OF_KEYS; + VerifyOrExit((key != NULL) && (value != NULL) && (RestoreFromFlash() == CHIP_NO_ERROR), err = CHIP_ERROR_INVALID_ARGUMENT); - ChipLogProgress(DeviceLayer, "KVS, get key id:: %i", key_id); + if ((it = g_kvs_map.find(key)) != g_kvs_map.end()) + { + key_id = it->second; + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id); - err = chip::DeviceLayer::Internal::K32WConfig::ReadConfigValueBin( - chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id), (uint8_t *) value, value_size, read_bytes); + err = + chip::DeviceLayer::Internal::K32WConfig::ReadConfigValueBin(pdm_internal_id, (uint8_t *) value, value_size, read_bytes); + *read_bytes_size = read_bytes; - *read_bytes_size = read_bytes; + ChipLogProgress(DeviceLayer, "KVS, get Matter key [%s] with PDM id: %i", key, pdm_internal_id); + } exit: return err; @@ -64,18 +145,69 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Get(const char * key, void * value, size_t CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, size_t value_size) { - CHIP_ERROR err = CHIP_NO_ERROR; - std::hash hash_fn; - uint16_t key_id; + CHIP_ERROR err = CHIP_ERROR_INVALID_ARGUMENT; + uint8_t key_id; + bool_t put_key = false; uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; + std::unordered_map::const_iterator it; + uint16_t pdm_internal_id = 0; + + VerifyOrExit((key != NULL) && (value != NULL) && (RestoreFromFlash() == CHIP_NO_ERROR), err = CHIP_ERROR_INVALID_ARGUMENT); + + if ((it = g_kvs_map.find(key)) == g_kvs_map.end()) /* new key */ + { + for (key_id = 0; key_id < kMaxNumberOfKeys; key_id++) + { + std::set::iterator iter = std::find(g_key_ids_set.begin(), g_key_ids_set.end(), key_id); + + if (iter == g_key_ids_set.end()) + { + if (g_key_ids_set.size() >= kMaxNumberOfKeys) + { + return CHIP_ERROR_NO_MEMORY; + } + + g_key_ids_set.insert(key_id); + + put_key = true; + break; + } + } + } + else /* overwrite key */ + { + put_key = true; + key_id = it->second; + } - VerifyOrExit((key != NULL) && (value != NULL), err = CHIP_ERROR_INVALID_ARGUMENT); - key_id = hash_fn(key) % MAX_NO_OF_KEYS; + if (put_key) + { + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id); + ChipLogProgress(DeviceLayer, "KVS, save in flash the value of the Matter key [%s] with PDM id: %i", key, pdm_internal_id); - ChipLogProgress(DeviceLayer, "KVS, put key id:: %i", key_id); + g_kvs_map.insert(std::make_pair(std::string(key), key_id)); + err = chip::DeviceLayer::Internal::K32WConfig::WriteConfigValueBin(pdm_internal_id, (uint8_t *) value, value_size); - err = chip::DeviceLayer::Internal::K32WConfig::WriteConfigValueBin( - chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id), (uint8_t *) value, value_size); + /* save the 'key' in flash such that it can be retrieved later on */ + if (err == CHIP_NO_ERROR) + { + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys); + ChipLogProgress(DeviceLayer, "KVS, save in flash the Matter key [%s] with PDM id: %i", key, pdm_internal_id); + + err = chip::DeviceLayer::Internal::K32WConfig::WriteConfigValueStr(pdm_internal_id, key, strlen(key)); + + if (err != CHIP_NO_ERROR) + { + ChipLogProgress(DeviceLayer, "KVS, Error while saving in flash the Matter key [%s] with PDM id: %i", key, + pdm_internal_id); + } + } + else + { + ChipLogProgress(DeviceLayer, "KVS, Error while saving in flash the value of the Matter key [%s] with PDM id: %i", key, + pdm_internal_id); + } + } exit: return err; @@ -83,17 +215,47 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) { - CHIP_ERROR err = CHIP_NO_ERROR; - std::hash hash_fn; - uint16_t key_id; - uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; + CHIP_ERROR err = CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND; + std::unordered_map::const_iterator it; + uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; + uint8_t key_id = 0; + uint16_t pdm_internal_id = 0; + + VerifyOrExit((key != NULL) && (RestoreFromFlash() == CHIP_NO_ERROR), err = CHIP_ERROR_INVALID_ARGUMENT); + + if ((it = g_kvs_map.find(key)) != g_kvs_map.end()) + { + key_id = it->second; + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id); + + g_key_ids_set.erase(key_id); + g_kvs_map.erase(it); + + ChipLogProgress(DeviceLayer, "KVS, delete from flash the Matter key [%s] with PDM id: %i", key, pdm_internal_id); + err = chip::DeviceLayer::Internal::K32WConfig::ClearConfigValue(pdm_internal_id); - VerifyOrExit(key != NULL, err = CHIP_ERROR_INVALID_ARGUMENT); - key_id = hash_fn(key) % MAX_NO_OF_KEYS; + /* also delete the 'key string' from flash */ + if (err == CHIP_NO_ERROR) + { + pdm_internal_id = chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys); + ChipLogProgress(DeviceLayer, "KVS, delete from flash the value of the Matter key [%s] with PDM id: %i", key, + pdm_internal_id); - ChipLogProgress(DeviceLayer, "KVS, deleting key id:: %i", key_id); + err = chip::DeviceLayer::Internal::K32WConfig::ClearConfigValue(pdm_internal_id); - err = chip::DeviceLayer::Internal::K32WConfig::ClearConfigValue(chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id)); + if (err != CHIP_NO_ERROR) + { + ChipLogProgress(DeviceLayer, + "KVS, Error while deleting from flash the value of the Matter key [%s] with PDM id: %i", key, + pdm_internal_id); + } + } + else + { + ChipLogProgress(DeviceLayer, "KVS, Error while deleting from flash the Matter key [%s] with PDM id: %i", key, + pdm_internal_id); + } + } exit: return err;