From e8992f500cbd5c2c91f01cdb312631ee33058c35 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Fri, 25 Aug 2023 17:15:18 +0200 Subject: [PATCH] fix(nvs_flash): nvs_set with same key and different data type Function now always rewrites old value under same key regardless existing data type. Users requiring old API behaviour can enable it by kconfig option CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY --- components/nvs_flash/.build-test-rules.yml | 3 + components/nvs_flash/Kconfig | 9 +++ .../host_test/nvs_host_test/main/test_nvs.cpp | 65 +++++++++++++++++++ .../sdkconfig.ci.default_set_key | 1 + .../nvs_host_test/sdkconfig.ci.legacy_set_key | 1 + components/nvs_flash/src/nvs_page.cpp | 2 +- components/nvs_flash/src/nvs_storage.cpp | 30 +++++++-- docs/en/api-reference/storage/nvs_flash.rst | 7 +- .../zh_CN/api-reference/storage/nvs_flash.rst | 7 +- 9 files changed, 110 insertions(+), 15 deletions(-) create mode 100644 components/nvs_flash/.build-test-rules.yml create mode 100644 components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.default_set_key create mode 100644 components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.legacy_set_key diff --git a/components/nvs_flash/.build-test-rules.yml b/components/nvs_flash/.build-test-rules.yml new file mode 100644 index 0000000000..93b76378a9 --- /dev/null +++ b/components/nvs_flash/.build-test-rules.yml @@ -0,0 +1,3 @@ +components/nvs_flash/host_test: + enable: + - if: IDF_TARGET == "linux" diff --git a/components/nvs_flash/Kconfig b/components/nvs_flash/Kconfig index 38d0a311f3..3689bfa1bf 100644 --- a/components/nvs_flash/Kconfig +++ b/components/nvs_flash/Kconfig @@ -25,4 +25,13 @@ menu "NVS" default n help This option switches error checking type between assertions (y) or return codes (n). + + config NVS_LEGACY_DUP_KEYS_COMPATIBILITY + bool "Enable legacy nvs_set function behavior when same key is reused with different data types" + default n + help + Enabling this will switch the nvs_set family of functions to the legacy mode. When called with same + key and different data type, existing value stored in NVS remains active and as a side effect, the + new value is also stored into NVS, although not accessible using respective nvs_get function. Use only + if your application relies on this NVS API behaviour. endmenu diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp index f7f9c6f7f5..44a9f44a9c 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp @@ -3255,6 +3255,71 @@ TEST_CASE("check and read data from partition generated via manufacturing utilit } } +TEST_CASE("nvs multiple write with same key but different types", "[nvs][xxx]") +{ + PartitionEmulationFixture f(0, 10); + + nvs_handle_t handle_1; + const uint32_t NVS_FLASH_SECTOR = 6; + const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 3; + TEMPORARILY_DISABLED(f.emu.setBounds(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN);) + + for (uint16_t j = NVS_FLASH_SECTOR; j < NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN; ++j) { + f.erase(j); + } + TEST_ESP_OK(nvs::NVSPartitionManager::get_instance()->init_custom(f.part(), + NVS_FLASH_SECTOR, + NVS_FLASH_SECTOR_COUNT_MIN)); + + TEST_ESP_OK(nvs_open("namespace1", NVS_READWRITE, &handle_1)); + + nvs_erase_all(handle_1); + + int32_t v32; + int8_t v8; + + TEST_ESP_OK(nvs_set_i32(handle_1, "foo", (int32_t)12345678)); + TEST_ESP_OK(nvs_set_i8(handle_1, "foo", (int8_t)12)); + TEST_ESP_OK(nvs_set_i8(handle_1, "foo", (int8_t)34)); + +#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY + // Legacy behavior + // First use of key hooks data type until removed by nvs_erase_key. Alternative re-use of same key with different + // data type is written to the storage as hidden active value. It is returned by nvs_get function after nvs_erase_key is called. + // Mixing more than 2 data types brings undefined behavior. It is not tested here. + + TEST_ESP_ERR(nvs_get_i8(handle_1, "foo", &v8), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_OK(nvs_get_i32(handle_1, "foo", &v32)); + TEST_ESP_OK(nvs_erase_key(handle_1, "foo")); + + TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8)); + TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_OK(nvs_erase_key(handle_1, "foo")); + + TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8)); + TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_OK(nvs_erase_key(handle_1, "foo")); + + TEST_ESP_ERR(nvs_erase_key(handle_1, "foo"), ESP_ERR_NVS_NOT_FOUND); +#else + // New behavior + // Latest nvs_set call replaces any existing value. Only one active value under the key exists in storage + + TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8)); + TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_OK(nvs_erase_key(handle_1, "foo")); + + TEST_ESP_ERR(nvs_get_i8(handle_1, "foo", &v8), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_ERR(nvs_erase_key(handle_1, "foo"), ESP_ERR_NVS_NOT_FOUND); +#endif + + nvs_close(handle_1); + + TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME)); +} + + /* Add new tests above */ /* This test has to be the final one */ diff --git a/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.default_set_key b/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.default_set_key new file mode 100644 index 0000000000..4ebf01d0f9 --- /dev/null +++ b/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.default_set_key @@ -0,0 +1 @@ +# CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY=n diff --git a/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.legacy_set_key b/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.legacy_set_key new file mode 100644 index 0000000000..226772f237 --- /dev/null +++ b/components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.legacy_set_key @@ -0,0 +1 @@ +CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY=y diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 36cde3db25..b69bcfbdaf 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -878,7 +878,7 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si end = ENTRY_COUNT; } - if (nsIndex != NS_ANY && datatype != ItemType::ANY && key != NULL) { + if (nsIndex != NS_ANY && key != NULL) { size_t cachedIndex = mHashList.find(start, Item(nsIndex, datatype, 0, key, chunkIdx)); if (cachedIndex < ENTRY_COUNT) { start = cachedIndex; diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 48c8a20fc0..5153ae7447 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -285,13 +285,27 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key } Page* findPage = nullptr; + bool matchedTypePageFound = false; Item item; esp_err_t err; if (datatype == ItemType::BLOB) { err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); + if(err == ESP_OK && findPage != nullptr) { + matchedTypePageFound = true; + } } else { +#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY err = findItem(nsIndex, datatype, key, findPage, item); + if(err == ESP_OK && findPage != nullptr) { + matchedTypePageFound = true; + } +#else + err = findItem(nsIndex, ItemType::ANY, key, findPage, item); + if(err == ESP_OK && findPage != nullptr && datatype == item.datatype) { + matchedTypePageFound = true; + } +#endif } if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { @@ -301,7 +315,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if (datatype == ItemType::BLOB) { VerOffset prevStart, nextStart; prevStart = nextStart = VerOffset::VER_0_OFFSET; - if (findPage) { + if (matchedTypePageFound) { // Do a sanity check that the item in question is actually being modified. // If it isn't, it is cheaper to purposefully not write out new data. // since it may invoke an erasure of flash. @@ -335,7 +349,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key return err; } - if (findPage) { + if (matchedTypePageFound) { /* Erase the blob with earlier version*/ err = eraseMultiPageBlob(nsIndex, key, prevStart); @@ -358,7 +372,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key // Do a sanity check that the item in question is actually being modified. // If it isn't, it is cheaper to purposefully not write out new data. // since it may invoke an erasure of flash. - if (findPage != nullptr && + if (matchedTypePageFound && findPage->cmpItem(nsIndex, datatype, key, data, dataSize) == ESP_OK) { return ESP_OK; } @@ -392,12 +406,20 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if (findPage) { if (findPage->state() == Page::PageState::UNINITIALIZED || findPage->state() == Page::PageState::INVALID) { +#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY err = findItem(nsIndex, datatype, key, findPage, item); +#else + err = findItem(nsIndex, ItemType::ANY, key, findPage, item); +#endif if (err != ESP_OK) { return err; } } +#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY err = findPage->eraseItem(nsIndex, datatype, key); +#else + err = findPage->eraseItem(nsIndex, ItemType::ANY, key); +#endif if (err == ESP_ERR_FLASH_OP_FAIL) { return ESP_ERR_NVS_REMOVE_FAILED; } diff --git a/docs/en/api-reference/storage/nvs_flash.rst b/docs/en/api-reference/storage/nvs_flash.rst index 8e0bec9db6..c25316c902 100644 --- a/docs/en/api-reference/storage/nvs_flash.rst +++ b/docs/en/api-reference/storage/nvs_flash.rst @@ -35,12 +35,9 @@ NVS operates on key-value pairs. Keys are ASCII strings; the maximum key length Additional types, such as ``float`` and ``double`` might be added later. -Keys are required to be unique. Assigning a new value to an existing key works as follows: +Keys are required to be unique. Assigning a new value to an existing key replaces the old value and data type with the value and data type specified by a write operation. -- If the new value is of the same type as the old one, value is updated. -- If the new value has a different data type, an error is returned. - -Data type check is also performed when reading a value. An error is returned if the data type of the read operation does not match the data type of the value. +A data type check is performed when reading a value. An error is returned if the data type expected by read operation does not match the data type of entry found for the key provided. Namespaces diff --git a/docs/zh_CN/api-reference/storage/nvs_flash.rst b/docs/zh_CN/api-reference/storage/nvs_flash.rst index fd494f1bee..8f05d495d5 100644 --- a/docs/zh_CN/api-reference/storage/nvs_flash.rst +++ b/docs/zh_CN/api-reference/storage/nvs_flash.rst @@ -35,12 +35,9 @@ NVS 的操作对象为键值对，其中键是 ASCII 字符串，当前支持的 后续可能会增加对 ``float`` 和 ``double`` 等其他类型数据的支持。 -键必须唯一。为现有的键写入新的值可能产生如下结果： +键必须唯一。为现有的键写入新值时，会将旧的值及数据类型更新为写入操作指定的值和数据类型。 -- 如果新旧值数据类型相同，则更新值； -- 如果新旧值数据类型不同，则返回错误。 - -读取值时也会执行数据类型检查。如果读取操作的数据类型与该值的数据类型不匹配，则返回错误。 +读取值时会执行数据类型检查。如果读取操作预期的数据类型与对应键的数据类型不匹配，则返回错误。 命名空间