From e8992f500cbd5c2c91f01cdb312631ee33058c35 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Fri, 25 Aug 2023 17:15:18 +0200 Subject: [PATCH 1/2] 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`` 等其他类型数据的支持。 -键必须唯一。为现有的键写入新的值可能产生如下结果： +键必须唯一。为现有的键写入新值时，会将旧的值及数据类型更新为写入操作指定的值和数据类型。 -- 如果新旧值数据类型相同，则更新值； -- 如果新旧值数据类型不同，则返回错误。 - -读取值时也会执行数据类型检查。如果读取操作的数据类型与该值的数据类型不匹配，则返回错误。 +读取值时会执行数据类型检查。如果读取操作预期的数据类型与对应键的数据类型不匹配，则返回错误。 命名空间 From 4f659b70e1eef889500a158ab24d2887d2a58118 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Tue, 5 Sep 2023 15:05:23 +0200 Subject: [PATCH 2/2] fix(nvs_flash): fixed failing nvs_page test --- components/nvs_flash/Kconfig | 9 +++--- .../nvs_page_test/main/nvs_page_test.cpp | 28 +++++++------------ components/nvs_flash/src/nvs_storage.cpp | 4 +-- tools/ci/check_copyright_ignore.txt | 1 - 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/components/nvs_flash/Kconfig b/components/nvs_flash/Kconfig index 3689bfa1bf..a432384ad9 100644 --- a/components/nvs_flash/Kconfig +++ b/components/nvs_flash/Kconfig @@ -30,8 +30,9 @@ menu "NVS" 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. + Enabling this option will switch the nvs_set() family of functions to the legacy mode: + when called repeatedly with the same key but different data type, the existing value + in the NVS remains active and the new value is just stored, actually not accessible through + corresponding nvs_get() call for the key given. Use this option only when your application + relies on such NVS API behaviour. endmenu diff --git a/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp b/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp index e5ca9ed089..81704974ff 100644 --- a/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp +++ b/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp @@ -1,16 +1,8 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD -// -// 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. +/* + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include "unity.h" #include "test_fixtures.hpp" @@ -863,7 +855,7 @@ void test_Page_calcEntries__uninit() NVSPageFixture fix; TEST_ASSERT_EQUAL(Page::PageState::UNINITIALIZED, fix.page.state()); - nvs_stats_t nvsStats = {0, 0, 0, 0}; + nvs_stats_t nvsStats = {0, 0, 0, 0, 0}; TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats)); TEST_ASSERT_EQUAL(0, nvsStats.used_entries); @@ -888,7 +880,7 @@ void test_Page_calcEntries__corrupt() TEST_ASSERT_EQUAL(Page::PageState::CORRUPT, page.state()); - nvs_stats_t nvsStats = {0, 0, 0, 0}; + nvs_stats_t nvsStats = {0, 0, 0, 0, 0}; TEST_ASSERT_EQUAL(ESP_OK, page.calcEntries(nvsStats)); TEST_ASSERT_EQUAL(0, nvsStats.used_entries); @@ -901,7 +893,7 @@ void test_Page_calcEntries__active_wo_blob() { NVSValidPageFixture fix; - nvs_stats_t nvsStats = {0, 0, 0, 0}; + nvs_stats_t nvsStats = {0, 0, 0, 0, 0}; TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats)); TEST_ASSERT_EQUAL(2, nvsStats.used_entries); @@ -914,7 +906,7 @@ void test_Page_calcEntries__active_with_blob() { NVSValidBlobPageFixture fix; - nvs_stats_t nvsStats = {0, 0, 0, 0}; + nvs_stats_t nvsStats = {0, 0, 0, 0, 0}; TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats)); TEST_ASSERT_EQUAL(4, nvsStats.used_entries); @@ -927,7 +919,7 @@ void test_Page_calcEntries__invalid() { Page page; - nvs_stats_t nvsStats = {0, 0, 0, 0}; + nvs_stats_t nvsStats = {0, 0, 0, 0, 0}; TEST_ASSERT_EQUAL(Page::PageState::INVALID, page.state()); diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 5153ae7447..bac1995580 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -291,7 +291,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key esp_err_t err; if (datatype == ItemType::BLOB) { err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); - if(err == ESP_OK && findPage != nullptr) { + if(err == ESP_OK) { matchedTypePageFound = true; } } else { @@ -302,7 +302,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key } #else err = findItem(nsIndex, ItemType::ANY, key, findPage, item); - if(err == ESP_OK && findPage != nullptr && datatype == item.datatype) { + if(err == ESP_OK && datatype == item.datatype) { matchedTypePageFound = true; } #endif diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 9405c40e15..9b35cbda32 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -587,7 +587,6 @@ components/mbedtls/port/include/sha512_alt.h components/mbedtls/port/sha/dma/include/esp_sha_dma_priv.h components/mbedtls/port/sha/dma/sha.c components/mbedtls/port/sha/parallel_engine/sha.c -components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp components/nvs_flash/include/nvs_handle.hpp components/nvs_flash/src/nvs_cxx_api.cpp components/nvs_flash/src/nvs_encrypted_partition.hpp