From 521463503fbdfadc4920a0713defba6de9416266 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Fri, 25 Aug 2023 17:15:18 +0200 Subject: [PATCH 1/6] 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 + .../sdkconfig.ci.default_set_key | 1 + .../nvs_host_test/sdkconfig.ci.legacy_set_key | 1 + components/nvs_flash/include/nvs.h | 21 +- components/nvs_flash/include/nvs_handle.hpp | 15 + components/nvs_flash/src/nvs_api.cpp | 21 +- .../nvs_flash/src/nvs_handle_locked.cpp | 24 +- .../nvs_flash/src/nvs_handle_locked.hpp | 20 +- .../nvs_flash/src/nvs_handle_simple.cpp | 35 +- .../nvs_flash/src/nvs_handle_simple.hpp | 2 + components/nvs_flash/src/nvs_storage.cpp | 442 +++++++++++------- components/nvs_flash/src/nvs_storage.hpp | 2 + docs/en/api-reference/storage/nvs_flash.rst | 7 +- .../zh_CN/api-reference/storage/nvs_flash.rst | 7 +- tools/ci/check_copyright_ignore.txt | 3 - 16 files changed, 391 insertions(+), 222 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 c246d48f83..0acfcdeed1 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/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/include/nvs.h b/components/nvs_flash/include/nvs.h index b57d982776..9f4f1b68f0 100644 --- a/components/nvs_flash/include/nvs.h +++ b/components/nvs_flash/include/nvs.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -464,6 +464,25 @@ esp_err_t nvs_get_str (nvs_handle_t handle, const char* key, char* out_value, si esp_err_t nvs_get_blob(nvs_handle_t handle, const char* key, void* out_value, size_t* length); /**@}*/ +/** + * @brief Lookup key-value pair with given key name. + * + * Note that function may indicate both existence of the key as well as the data type of NVS entry if it is found. + * + * @param[in] handle Storage handle obtained with nvs_open. + * @param[in] key Key name. Maximum length is (NVS_KEY_NAME_MAX_SIZE-1) characters. Shouldn't be empty. + * @param[out] out_type Pointer to the output variable populated with data type of NVS entry in case key was found. + * May be NULL, respective data type is then not provided. + * @return + * - ESP_OK if NVS entry for key provided was found + * - ESP_ERR_NVS_NOT_FOUND if the requested key doesn't exist + * - ESP_ERR_NVS_INVALID_HANDLE if handle has been closed or is NULL + * - ESP_FAIL if there is an internal error; most likely due to corrupted + * NVS partition (only if NVS assertion checks are disabled) + * - other error codes from the underlying storage driver + */ +esp_err_t nvs_find_key(nvs_handle_t handle, const char* key, nvs_type_t* out_type); + /** * @brief Erase key-value pair with given key name. * diff --git a/components/nvs_flash/include/nvs_handle.hpp b/components/nvs_flash/include/nvs_handle.hpp index b09d013d22..d4ba6d3481 100644 --- a/components/nvs_flash/include/nvs_handle.hpp +++ b/components/nvs_flash/include/nvs_handle.hpp @@ -164,6 +164,21 @@ public: */ virtual esp_err_t get_item_size(ItemType datatype, const char *key, size_t &size) = 0; + /** + * @brief Checks whether key exists and optionally returns also data type of associated entry. + * + * @param[in] key Key name. Maximum length is (NVS_KEY_NAME_MAX_SIZE-1) characters. Shouldn't be empty. + * @param[out] nvstype Nvs data type to of entry, if it exists. + * + * @return - ESP_OK if NVS entry for key provided was found. Data type will be returned via \c nvstype. + * - ESP_ERR_NVS_NOT_FOUND if the requested key doesn't exist. + * - ESP_ERR_NVS_INVALID_HANDLE if handle has been closed or is NULL. + * - ESP_FAIL if there is an internal error; most likely due to corrupted + * NVS partition (only if NVS assertion checks are disabled). + * - other error codes from the underlying storage driver. + */ + virtual esp_err_t find_key(const char* key, nvs_type_t &nvstype) = 0; + /** * @brief Erases an entry. */ diff --git a/components/nvs_flash/src/nvs_api.cpp b/components/nvs_flash/src/nvs_api.cpp index cf8226bec3..5b4f15532b 100644 --- a/components/nvs_flash/src/nvs_api.cpp +++ b/components/nvs_flash/src/nvs_api.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -310,6 +310,25 @@ extern "C" void nvs_close(nvs_handle_t handle) delete static_cast(it); } +extern "C" esp_err_t nvs_find_key(nvs_handle_t c_handle, const char* key, nvs_type_t* out_type) +{ + Lock lock; + ESP_LOGD(TAG, "%s %s", __func__, key); + NVSHandleSimple *handle; + auto err = nvs_find_ns_handle(c_handle, &handle); + if (err != ESP_OK) { + return err; + } + + nvs_type_t nvstype; + err = handle->find_key(key, nvstype); + + if(err == ESP_OK && out_type != nullptr) + *out_type = nvstype; + + return err; +} + extern "C" esp_err_t nvs_erase_key(nvs_handle_t c_handle, const char* key) { Lock lock; diff --git a/components/nvs_flash/src/nvs_handle_locked.cpp b/components/nvs_flash/src/nvs_handle_locked.cpp index 3921887448..ede347d899 100644 --- a/components/nvs_flash/src/nvs_handle_locked.cpp +++ b/components/nvs_flash/src/nvs_handle_locked.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-2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include "nvs_handle_locked.hpp" namespace nvs { @@ -47,6 +39,12 @@ esp_err_t NVSHandleLocked::get_item_size(ItemType datatype, const char *key, siz return handle->get_item_size(datatype, key, size); } +esp_err_t NVSHandleLocked::find_key(const char* key, nvs_type_t &nvstype) +{ + Lock lock; + return handle->find_key(key, nvstype); +} + esp_err_t NVSHandleLocked::erase_item(const char* key) { Lock lock; return handle->erase_item(key); diff --git a/components/nvs_flash/src/nvs_handle_locked.hpp b/components/nvs_flash/src/nvs_handle_locked.hpp index b9b0cb5620..368162485e 100644 --- a/components/nvs_flash/src/nvs_handle_locked.hpp +++ b/components/nvs_flash/src/nvs_handle_locked.hpp @@ -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-2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #ifndef NVS_HANDLE_LOCKED_HPP_ #define NVS_HANDLE_LOCKED_HPP_ @@ -49,6 +41,8 @@ public: esp_err_t get_item_size(ItemType datatype, const char *key, size_t &size) override; + esp_err_t find_key(const char* key, nvs_type_t &nvstype) override; + esp_err_t erase_item(const char* key) override; esp_err_t erase_all() override; diff --git a/components/nvs_flash/src/nvs_handle_simple.cpp b/components/nvs_flash/src/nvs_handle_simple.cpp index 348e197b7c..faaea7fedf 100644 --- a/components/nvs_flash/src/nvs_handle_simple.cpp +++ b/components/nvs_flash/src/nvs_handle_simple.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-2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include "nvs_handle.hpp" #include "nvs_partition_manager.hpp" @@ -73,6 +65,23 @@ esp_err_t NVSHandleSimple::get_item_size(ItemType datatype, const char *key, siz return mStoragePtr->getItemDataSize(mNsIndex, datatype, key, size); } +esp_err_t NVSHandleSimple::find_key(const char* key, nvs_type_t &nvstype) +{ + if (!valid) return ESP_ERR_NVS_INVALID_HANDLE; + + nvs::ItemType datatype; + esp_err_t err = mStoragePtr->findKey(mNsIndex, key, &datatype); + if(err != ESP_OK) + return err; + + if(datatype == ItemType::BLOB_IDX || datatype == ItemType::BLOB) + datatype = ItemType::BLOB_DATA; + + nvstype = (nvs_type_t) datatype; + + return err; +} + esp_err_t NVSHandleSimple::erase_item(const char* key) { if (!valid) return ESP_ERR_NVS_INVALID_HANDLE; diff --git a/components/nvs_flash/src/nvs_handle_simple.hpp b/components/nvs_flash/src/nvs_handle_simple.hpp index ff0e93a3d6..7ddcc0a390 100644 --- a/components/nvs_flash/src/nvs_handle_simple.hpp +++ b/components/nvs_flash/src/nvs_handle_simple.hpp @@ -51,6 +51,8 @@ public: esp_err_t get_item_size(ItemType datatype, const char *key, size_t &size) override; + esp_err_t find_key(const char *key, nvs_type_t &nvstype) override; + esp_err_t erase_item(const char *key) override; esp_err_t erase_all() override; diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 4fd501094c..a4974563c1 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -38,7 +38,7 @@ void Storage::clearNamespaces() esp_err_t Storage::populateBlobIndices(TBlobIndexList& blobIdxList) { - for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { + for(auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { Page& p = *it; size_t itemIndex = 0; Item item; @@ -47,10 +47,10 @@ esp_err_t Storage::populateBlobIndices(TBlobIndexList& blobIdxList) * logic in pagemanager will remove the earlier index. So we should never find a * duplicate index at this point */ - while (p.findItem(Page::NS_ANY, ItemType::BLOB_IDX, nullptr, itemIndex, item) == ESP_OK) { + while(p.findItem(Page::NS_ANY, ItemType::BLOB_IDX, nullptr, itemIndex, item) == ESP_OK) { BlobIndexNode* entry = new (std::nothrow) BlobIndexNode; - if (!entry) return ESP_ERR_NO_MEM; + if(!entry) return ESP_ERR_NO_MEM; item.getKey(entry->key, sizeof(entry->key)); entry->nsIndex = item.nsIndex; @@ -76,7 +76,7 @@ esp_err_t Storage::populateBlobIndices(TBlobIndexList& blobIdxList) // later by the call to eraseOrphanDataBlobs(). void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList) { - for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { + for(auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { Page& p = *it; size_t itemIndex = 0; Item item; @@ -85,7 +85,7 @@ void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList) * 1) VER_0_OFFSET <= chunkIndex < VER_1_OFFSET-1 => Version0 chunks * 2) VER_1_OFFSET <= chunkIndex < VER_ANY => Version1 chunks */ - while (p.findItem(Page::NS_ANY, ItemType::BLOB_DATA, nullptr, itemIndex, item) == ESP_OK) { + while(p.findItem(Page::NS_ANY, ItemType::BLOB_DATA, nullptr, itemIndex, item) == ESP_OK) { auto iter = std::find_if(blobIdxList.begin(), blobIdxList.end(), @@ -94,7 +94,7 @@ void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList) && (item.nsIndex == e.nsIndex) && (item.chunkIndex >= static_cast (e.chunkStart)) && (item.chunkIndex < static_cast ((e.chunkStart == nvs::VerOffset::VER_0_OFFSET) ? nvs::VerOffset::VER_1_OFFSET : nvs::VerOffset::VER_ANY));}); - if (iter != std::end(blobIdxList)) { + if(iter != std::end(blobIdxList)) { // accumulate the size iter->observedDataSize += item.varLength.dataSize; iter->observedChunkCount++; @@ -104,15 +104,15 @@ void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList) } auto iter = blobIdxList.begin(); - while (iter != blobIdxList.end()) + while(iter != blobIdxList.end()) { - if ( (iter->observedDataSize != iter->dataSize) || (iter->observedChunkCount != iter->chunkCount) ) + if( (iter->observedDataSize != iter->dataSize) || (iter->observedChunkCount != iter->chunkCount) ) { // Delete blob_index from flash // This is very rare case, so we can loop over all pages - for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { + for(auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { // skip pages in non eligible states - if (it->state() == nvs::Page::PageState::CORRUPT + if(it->state() == nvs::Page::PageState::CORRUPT || it->state() == nvs::Page::PageState::INVALID || it->state() == nvs::Page::PageState::UNINITIALIZED){ continue; @@ -140,7 +140,7 @@ void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList) void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList) { - for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { + for(auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { Page& p = *it; size_t itemIndex = 0; Item item; @@ -149,7 +149,7 @@ void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList) * 1) VER_0_OFFSET <= chunkIndex < VER_1_OFFSET-1 => Version0 chunks * 2) VER_1_OFFSET <= chunkIndex < VER_ANY => Version1 chunks */ - while (p.findItem(Page::NS_ANY, ItemType::BLOB_DATA, nullptr, itemIndex, item) == ESP_OK) { + while(p.findItem(Page::NS_ANY, ItemType::BLOB_DATA, nullptr, itemIndex, item) == ESP_OK) { auto iter = std::find_if(blobIdxList.begin(), blobIdxList.end(), @@ -158,7 +158,7 @@ void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList) && (item.nsIndex == e.nsIndex) && (item.chunkIndex >= static_cast (e.chunkStart)) && (item.chunkIndex < static_cast (e.chunkStart) + e.chunkCount);}); - if (iter == std::end(blobIdxList)) { + if(iter == std::end(blobIdxList)) { p.eraseItem(item.nsIndex, item.datatype, item.key, item.chunkIndex); } @@ -170,7 +170,7 @@ void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList) esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) { auto err = mPageManager.load(mPartition, baseSector, sectorCount); - if (err != ESP_OK) { + if(err != ESP_OK) { mState = StorageState::INVALID; return err; } @@ -178,25 +178,25 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) // load namespaces list clearNamespaces(); std::fill_n(mNamespaceUsage.data(), mNamespaceUsage.byteSize() / 4, 0); - for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { + for(auto it = mPageManager.begin(); it != mPageManager.end(); ++it) { Page& p = *it; size_t itemIndex = 0; Item item; - while (p.findItem(Page::NS_INDEX, ItemType::U8, nullptr, itemIndex, item) == ESP_OK) { + while(p.findItem(Page::NS_INDEX, ItemType::U8, nullptr, itemIndex, item) == ESP_OK) { NamespaceEntry* entry = new (std::nothrow) NamespaceEntry; - if (!entry) { + if(!entry) { mState = StorageState::INVALID; return ESP_ERR_NO_MEM; } item.getKey(entry->mName, sizeof(entry->mName)); err = item.getValue(entry->mIndex); - if (err != ESP_OK) { + if(err != ESP_OK) { delete entry; return err; } - if (mNamespaceUsage.set(entry->mIndex, true) != ESP_OK) { + if(mNamespaceUsage.set(entry->mIndex, true) != ESP_OK) { delete entry; return ESP_FAIL; } @@ -204,17 +204,17 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) itemIndex += item.span; } } - if (mNamespaceUsage.set(0, true) != ESP_OK) { + if(mNamespaceUsage.set(0, true) != ESP_OK) { return ESP_FAIL; } - if (mNamespaceUsage.set(255, true) != ESP_OK) { + if(mNamespaceUsage.set(255, true) != ESP_OK) { return ESP_FAIL; } // Populate list of multi-page index entries. TBlobIndexList blobIdxList; err = populateBlobIndices(blobIdxList); - if (err != ESP_OK) { + if(err != ESP_OK) { mState = StorageState::INVALID; return ESP_ERR_NO_MEM; } @@ -243,10 +243,10 @@ bool Storage::isValid() const esp_err_t Storage::findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item, uint8_t chunkIdx, VerOffset chunkStart) { - for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { size_t itemIndex = 0; auto err = it->findItem(nsIndex, datatype, key, itemIndex, item, chunkIdx, chunkStart); - if (err == ESP_OK) { + if(err == ESP_OK) { page = it; return ESP_OK; } @@ -269,7 +269,7 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo max_pages = (Page::CHUNK_ANY-1)/2; } - if (dataSize > max_pages * Page::CHUNK_MAX_SIZE) { + if(dataSize > max_pages * Page::CHUNK_MAX_SIZE) { return ESP_ERR_NVS_VALUE_TOO_LONG; } @@ -277,16 +277,16 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo Page& page = getCurrentPage(); size_t tailroom = page.getVarDataTailroom(); size_t chunkSize = 0; - if (chunkCount == 0U && ((tailroom < dataSize) || (tailroom == 0 && dataSize == 0)) && tailroom < Page::CHUNK_MAX_SIZE/10) { + if(chunkCount == 0U && ((tailroom < dataSize) || (tailroom == 0 && dataSize == 0)) && tailroom < Page::CHUNK_MAX_SIZE/10) { /** This is the first chunk and tailroom is too small ***/ - if (page.state() != Page::PageState::FULL) { + if(page.state() != Page::PageState::FULL) { err = page.markFull(); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } } err = mPageManager.requestNewPage(); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } else if(getCurrentPage().getVarDataTailroom() == tailroom) { /* We got the same page or we are not improving.*/ @@ -294,7 +294,7 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo } else { continue; } - } else if (!tailroom) { + } else if(!tailroom) { err = ESP_ERR_NVS_NOT_ENOUGH_SPACE; break; } @@ -309,32 +309,32 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo static_cast (data) + offset, chunkSize, static_cast (chunkStart) + chunkCount); chunkCount++; - if (err != ESP_OK) { + if(err != ESP_OK) { NVS_ASSERT_OR_RETURN(err != ESP_ERR_NVS_PAGE_FULL, err); break; } else { UsedPageNode* node = new (std::nothrow) UsedPageNode(); - if (!node) { + if(!node) { err = ESP_ERR_NO_MEM; break; } node->mPage = &page; usedPages.push_back(node); - if (remainingSize || (tailroom - chunkSize) < Page::ENTRY_SIZE) { - if (page.state() != Page::PageState::FULL) { + if(remainingSize || (tailroom - chunkSize) < Page::ENTRY_SIZE) { + if(page.state() != Page::PageState::FULL) { err = page.markFull(); - if (err != ESP_OK) { + if(err != ESP_OK) { break; } } err = mPageManager.requestNewPage(); - if (err != ESP_OK) { + if(err != ESP_OK) { break; } } } offset += chunkSize; - if (!remainingSize) { + if(!remainingSize) { /* All pages are stored. Now store the index.*/ Item item; std::fill_n(item.data, sizeof(item.data), 0xff); @@ -346,12 +346,12 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo NVS_ASSERT_OR_RETURN(err != ESP_ERR_NVS_PAGE_FULL, err); break; } - } while (1); + } while(1); - if (err != ESP_OK) { + if(err != ESP_OK) { /* Anything failed, then we should erase all the written chunks*/ int ii=0; - for (auto it = std::begin(usedPages); it != std::end(usedPages); it++) { + for(auto it = std::begin(usedPages); it != std::end(usedPages); it++) { it->mPage->eraseItem(nsIndex, ItemType::BLOB_DATA, key, ii++); } } @@ -359,178 +359,264 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo return err; } +// datatype BLOB is written as BLOB_INDEX and BLOB_DATA and is searched for previous value as BLOB_INDEX and/or BLOB +// datatype BLOB_IDX and BLOB_DATA are not supported as input parameters, the layer above should always use BLOB esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } + // pointer to the page where the existing item was found Page* findPage = nullptr; + // page sequence number helping to detect whether the page with old value was relocated during the new write + uint32_t findPageSeqNumber = UINT32_MAX; + + // indicates the datatype representation match between the old value and the new one + bool matchedTypePageFound = false; + + // contains the item with the old value, if found Item item; - esp_err_t err; - if (datatype == ItemType::BLOB) { + esp_err_t err = ESP_OK; + + // Try to find existing item with the same key and namespace index + // We are performing the findItem with datatype specified (it is not ANY) to ensure the hash list lookup is done. + if(datatype == ItemType::BLOB) { + // Specific lookup if performed for input datatype BLOB. The searched datatype for exact match is BLOB_INDEX. + // The BLOB_INDEX is used to store the metadata of the (originally typed) BLOB data in current V2 implementation. err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); + if(err == ESP_OK && findPage != nullptr) { + matchedTypePageFound = true; + } } else { + // Handle all other data types than BLOB err = findItem(nsIndex, datatype, key, findPage, item); + if(err == ESP_OK && findPage != nullptr) { + matchedTypePageFound = true; + + // keep the sequence number of the page where the item was found for later check of relocation + err = findPage->getSeqNumber(findPageSeqNumber); + if(err != ESP_OK) { + return err; + } + } } - if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { +#ifndef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY + // If the item was not found, try to find it with any other datatype. Omit the BLOB_DATA and handle BLOB with respect + // to the V1 and V2 representation. + const ItemType dataTypes[] = { + ItemType::U8, + ItemType::U16, + ItemType::U32, + ItemType::U64, + ItemType::I8, + ItemType::I16, + ItemType::I32, + ItemType::I64, + ItemType::SZ, + ItemType::BLOB_IDX, + ItemType::BLOB + }; + + if(findPage == nullptr) { + // Iterate over potential data types to allow findItem() search using the hash list instead of bruteforce search. + for(const auto& currType : dataTypes) { + // Skip search for BLOB_IDX if the requested datatype is BLOB. BLOB_IDX was already searched above. + if(datatype == ItemType::BLOB && currType == ItemType::BLOB_IDX) continue; + + // Skip search if requested datatype is not BLOB and the current datatype is equal to the requested one. This was already searched above. + if(datatype != ItemType::BLOB && currType == datatype) continue; + + err = findItem(nsIndex, currType, key, findPage, item); + if(err == ESP_OK && findPage != nullptr) { + // keep the sequence number of the page where the item was found for later check of relocation + err = findPage->getSeqNumber(findPageSeqNumber); + if(err != ESP_OK) { + return err; + } + // item was found with the same key and namespace index but data type is different + matchedTypePageFound = false; + break; + } + } + } +#endif + + // Here the findPage is either nullptr or points to the page where the item was found. + // The matchedTypePageFound is true if the old value item was found and its datatype representation matches the new one. + // This flag is used to determine if the item should be checked for same value. + if(err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { return err; } - if (datatype == ItemType::BLOB) { + // Handle value update + if(datatype == ItemType::BLOB) { VerOffset prevStart, nextStart; prevStart = nextStart = VerOffset::VER_0_OFFSET; - if (findPage) { - // Do a sanity check that the item in question is actually being modified. + if(matchedTypePageFound) { + // Do a 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 (cmpMultiPageBlob(nsIndex, key, data, dataSize) == ESP_OK) { + if(cmpMultiPageBlob(nsIndex, key, data, dataSize) == ESP_OK) { return ESP_OK; } - if (findPage->state() == Page::PageState::UNINITIALIZED || - findPage->state() == Page::PageState::INVALID) { - err = findItem(nsIndex, datatype, key, findPage, item); - if (err != ESP_OK) { - return err; - } - } - /* Get the version of the previous index with same */ + // Get the version of the previous index with same prevStart = item.blobIndex.chunkStart; NVS_ASSERT_OR_RETURN(prevStart == VerOffset::VER_0_OFFSET || prevStart == VerOffset::VER_1_OFFSET, ESP_FAIL); - - /* Toggle the version by changing the offset */ + // Toggle the version by changing the offset nextStart = (prevStart == VerOffset::VER_1_OFFSET) ? VerOffset::VER_0_OFFSET : VerOffset::VER_1_OFFSET; } - /* Write the blob with new version*/ + // Write the blob with new version err = writeMultiPageBlob(nsIndex, key, data, dataSize, nextStart); - if (err == ESP_ERR_NVS_PAGE_FULL) { + if(err == ESP_ERR_NVS_PAGE_FULL) { return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } - if (err != ESP_OK) { + + if(err != ESP_OK) { return err; } - - if (findPage) { - /* Erase the blob with earlier version*/ - err = eraseMultiPageBlob(nsIndex, key, prevStart); - - if (err == ESP_ERR_FLASH_OP_FAIL) { - return ESP_ERR_NVS_REMOVE_FAILED; - } - if (err != ESP_OK) { - return err; - } - - findPage = nullptr; - } else { - /* Support for earlier versions where BLOBS were stored without index */ - err = findItem(nsIndex, datatype, key, findPage, item); - if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { - return err; - } - } } else { - // Do a sanity check that the item in question is actually being modified. + // Do a 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; } Page& page = getCurrentPage(); err = page.writeItem(nsIndex, datatype, key, data, dataSize); - if (err == ESP_ERR_NVS_PAGE_FULL) { - if (page.state() != Page::PageState::FULL) { + if(err == ESP_ERR_NVS_PAGE_FULL) { + if(page.state() != Page::PageState::FULL) { err = page.markFull(); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } } err = mPageManager.requestNewPage(); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } err = getCurrentPage().writeItem(nsIndex, datatype, key, data, dataSize); - if (err == ESP_ERR_NVS_PAGE_FULL) { + if(err == ESP_ERR_NVS_PAGE_FULL) { return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } - if (err != ESP_OK) { - return err; - } - } else if (err != ESP_OK) { + } + + if(err != ESP_OK) { return err; } } - if (findPage) { - if (findPage->state() == Page::PageState::UNINITIALIZED || - findPage->state() == Page::PageState::INVALID) { - err = findItem(nsIndex, datatype, key, findPage, item); - if (err != ESP_OK) { + // Delete previous value + // Note: The old entry won't be deleted if the new value is the same as the old value - code won't reach here in that case. + + // If findPage is null then previous value was not present in NVS and nothig is to be deleted. + if(findPage == nullptr) { + return err; + } + + if(item.datatype == ItemType::BLOB_IDX) { + // If the item found was BLOB_INDEX, the eraseMultiPageBlob is used to erase the whole multi-page blob. + // It is not necessary to check the potential page relocation as the function will find the blob again anyway. + VerOffset prevStart = item.blobIndex.chunkStart; + err = eraseMultiPageBlob(nsIndex, key, prevStart); + + if(err == ESP_ERR_FLASH_OP_FAIL) { + return ESP_ERR_NVS_REMOVE_FAILED; + } + } else { + // For all other data types, we have to check the potential page relocation. + + // The findPage might have been relocated as a part of space reclaiming. + // First indication is the page state. It might become the "spare" page thus changing the state from FULL or ACTIVE. + bool wasRelocated = false; + + if( findPage->state() != Page::PageState::ACTIVE && + findPage->state() != Page::PageState::FULL) { + wasRelocated = true; + } + + // Other indication of the multi step relocation is the page sequence number. If the sequence number is different than page + // sequence number at the moment initial item was found, the page was relocated. + if(!wasRelocated) { + uint32_t newPageSeqNumber; + err = findPage->getSeqNumber(newPageSeqNumber); + if(err != ESP_OK) { + return err; + } + if(newPageSeqNumber != findPageSeqNumber) { + wasRelocated = true; + } + } + + if(wasRelocated) { + // The page was relocated. We have to find the old value again from the beginning. + // As the item was already found before relocation, we can use the exact datatype from item. + err = findItem(nsIndex, item.datatype, key, findPage, item); + if(err != ESP_OK) { return err; } } - err = findPage->eraseItem(nsIndex, datatype, key); - if (err == ESP_ERR_FLASH_OP_FAIL) { + // Page containing the old value is now refreshed. We can erase the old value. + err = findPage->eraseItem(nsIndex, item.datatype, key); + if(err == ESP_ERR_FLASH_OP_FAIL) { return ESP_ERR_NVS_REMOVE_FAILED; } - if (err != ESP_OK) { - return err; - } } + #ifdef DEBUG_STORAGE debugCheck(); #endif - return ESP_OK; + return err; } esp_err_t Storage::createOrOpenNamespace(const char* nsName, bool canCreate, uint8_t& nsIndex) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } auto it = std::find_if(mNamespaces.begin(), mNamespaces.end(), [=] (const NamespaceEntry& e) -> bool { return strncmp(nsName, e.mName, sizeof(e.mName) - 1) == 0; }); - if (it == std::end(mNamespaces)) { - if (!canCreate) { + if(it == std::end(mNamespaces)) { + if(!canCreate) { return ESP_ERR_NVS_NOT_FOUND; } uint8_t ns; bool ns_state; - for (ns = 1; ns < 255; ++ns) { - if (mNamespaceUsage.get(ns, &ns_state) != ESP_OK) { + for(ns = 1; ns < 255; ++ns) { + if(mNamespaceUsage.get(ns, &ns_state) != ESP_OK) { return ESP_FAIL; } - if (!ns_state) { + if(!ns_state) { break; } } - if (ns == 255) { + if(ns == 255) { return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } auto err = writeItem(Page::NS_INDEX, ItemType::U8, nsName, &ns, sizeof(ns)); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } - if (mNamespaceUsage.set(ns, true) != ESP_OK) { + if(mNamespaceUsage.set(ns, true) != ESP_OK) { return ESP_FAIL; } nsIndex = ns; NamespaceEntry* entry = new (std::nothrow) NamespaceEntry; - if (!entry) { + if(!entry) { return ESP_ERR_NO_MEM; } @@ -552,7 +638,7 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat /* First read the blob index */ auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } @@ -563,21 +649,21 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat NVS_ASSERT_OR_RETURN(dataSize == item.blobIndex.dataSize, ESP_FAIL); /* Now read corresponding chunks */ - for (uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { + for(uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum); - if (err != ESP_OK) { - if (err == ESP_ERR_NVS_NOT_FOUND) { + if(err != ESP_OK) { + if(err == ESP_ERR_NVS_NOT_FOUND) { break; } return err; } - if (item.varLength.dataSize > dataSize - offset) { + if(item.varLength.dataSize > dataSize - offset) { /* The size of the entry in the index is inconsistent with the sum of the sizes of chunks */ err = ESP_ERR_NVS_INVALID_LENGTH; break; } err = findPage->readItem(nsIndex, ItemType::BLOB_DATA, key, static_cast(data) + offset, item.varLength.dataSize, static_cast (chunkStart) + chunkNum); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } NVS_ASSERT_OR_RETURN(static_cast (chunkStart) + chunkNum == item.chunkIndex, ESP_FAIL); @@ -585,7 +671,7 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat offset += item.varLength.dataSize; } - if (err == ESP_ERR_NVS_NOT_FOUND || err == ESP_ERR_NVS_INVALID_LENGTH) { + if(err == ESP_ERR_NVS_NOT_FOUND || err == ESP_ERR_NVS_INVALID_LENGTH) { // cleanup if a chunk is not found or the size is inconsistent eraseMultiPageBlob(nsIndex, key); } @@ -602,7 +688,7 @@ esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void /* First read the blob index */ auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } @@ -611,21 +697,21 @@ esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void size_t readSize = item.blobIndex.dataSize; size_t offset = 0; - if (dataSize != readSize) { + if(dataSize != readSize) { return ESP_ERR_NVS_CONTENT_DIFFERS; } /* Now read corresponding chunks */ - for (uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { + for(uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum); - if (err != ESP_OK) { - if (err == ESP_ERR_NVS_NOT_FOUND) { + if(err != ESP_OK) { + if(err == ESP_ERR_NVS_NOT_FOUND) { break; } return err; } err = findPage->cmpItem(nsIndex, ItemType::BLOB_DATA, key, static_cast(data) + offset, item.varLength.dataSize, static_cast (chunkStart) + chunkNum); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } NVS_ASSERT_OR_RETURN(static_cast (chunkStart) + chunkNum == item.chunkIndex, ESP_FAIL); @@ -639,21 +725,21 @@ esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void esp_err_t Storage::readItem(uint8_t nsIndex, ItemType datatype, const char* key, void* data, size_t dataSize) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } Item item; Page* findPage = nullptr; - if (datatype == ItemType::BLOB) { + if(datatype == ItemType::BLOB) { auto err = readMultiPageBlob(nsIndex, key, data, dataSize); - if (err != ESP_ERR_NVS_NOT_FOUND) { + if(err != ESP_ERR_NVS_NOT_FOUND) { return err; } // else check if the blob is stored with earlier version format without index } auto err = findItem(nsIndex, datatype, key, findPage, item); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } return findPage->readItem(nsIndex, datatype, key, data, dataSize); @@ -662,19 +748,19 @@ esp_err_t Storage::readItem(uint8_t nsIndex, ItemType datatype, const char* key, esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffset chunkStart) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } Item item; Page* findPage = nullptr; auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } // Erase the index first and make children blobs orphan err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } @@ -684,12 +770,12 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse if(chunkStart == VerOffset::VER_ANY) { err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart); - if (err == ESP_OK) { + if(err == ESP_OK) { err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } - } else if (err != ESP_ERR_NVS_NOT_FOUND) { + } else if(err != ESP_ERR_NVS_NOT_FOUND) { return err; } } @@ -700,17 +786,17 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse if(chunkStart == VerOffset::VER_0_OFFSET) { maxChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET; - } else if (chunkStart == VerOffset::VER_1_OFFSET) { + } else if(chunkStart == VerOffset::VER_1_OFFSET) { minChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET; } - for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { size_t itemIndex = 0; do { err = it->findItem(nsIndex, ItemType::BLOB_DATA, key, itemIndex, item); - if (err == ESP_ERR_NVS_NOT_FOUND) { + if(err == ESP_ERR_NVS_NOT_FOUND) { break; - } else if (err == ESP_OK) { + } else if(err == ESP_OK) { // check if item.chunkIndex is within the version range indicated by chunkStart, if so, delete it if((item.chunkIndex >= minChunkIndex) && (item.chunkIndex < maxChunkIndex)) { err = it->eraseEntryAndSpan(itemIndex); @@ -722,7 +808,7 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse if(err != ESP_OK) { return err; } - } while (err == ESP_OK && itemIndex < Page::ENTRY_COUNT); + } while(err == ESP_OK && itemIndex < Page::ENTRY_COUNT); } return ESP_OK; @@ -730,22 +816,22 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse esp_err_t Storage::eraseItem(uint8_t nsIndex, ItemType datatype, const char* key) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } - if (datatype == ItemType::BLOB) { + if(datatype == ItemType::BLOB) { return eraseMultiPageBlob(nsIndex, key); } Item item; Page* findPage = nullptr; auto err = findItem(nsIndex, datatype, key, findPage, item); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } - if (item.datatype == ItemType::BLOB_DATA || item.datatype == ItemType::BLOB_IDX) { + if(item.datatype == ItemType::BLOB_DATA || item.datatype == ItemType::BLOB_IDX) { return eraseMultiPageBlob(nsIndex, key); } @@ -754,17 +840,17 @@ esp_err_t Storage::eraseItem(uint8_t nsIndex, ItemType datatype, const char* key esp_err_t Storage::eraseNamespace(uint8_t nsIndex) { - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } - for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { - while (true) { + for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + while(true) { auto err = it->eraseItem(nsIndex, ItemType::ANY, nullptr); - if (err == ESP_ERR_NVS_NOT_FOUND) { + if(err == ESP_ERR_NVS_NOT_FOUND) { break; } - else if (err != ESP_OK) { + else if(err != ESP_OK) { return err; } } @@ -773,7 +859,7 @@ esp_err_t Storage::eraseNamespace(uint8_t nsIndex) } -esp_err_t Storage::getItemDataSize(uint8_t nsIndex, ItemType datatype, const char* key, size_t& dataSize) +esp_err_t Storage::findKey(const uint8_t nsIndex, const char* key, ItemType* datatype) { if (mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; @@ -781,13 +867,33 @@ esp_err_t Storage::getItemDataSize(uint8_t nsIndex, ItemType datatype, const cha Item item; Page* findPage = nullptr; - auto err = findItem(nsIndex, datatype, key, findPage, item); + auto err = findItem(nsIndex, ItemType::ANY, key, findPage, item); if (err != ESP_OK) { - if (datatype != ItemType::BLOB) { + return err; + } + + if(datatype != nullptr) { + *datatype = item.datatype; + } + + return err; +} + +esp_err_t Storage::getItemDataSize(uint8_t nsIndex, ItemType datatype, const char* key, size_t& dataSize) +{ + if(mState != StorageState::ACTIVE) { + return ESP_ERR_NVS_NOT_INITIALIZED; + } + + Item item; + Page* findPage = nullptr; + auto err = findItem(nsIndex, datatype, key, findPage, item); + if(err != ESP_OK) { + if(datatype != ItemType::BLOB) { return err; } err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); - if (err != ESP_OK) { + if(err != ESP_OK) { return err; } dataSize = item.blobIndex.dataSize; @@ -800,7 +906,7 @@ esp_err_t Storage::getItemDataSize(uint8_t nsIndex, ItemType datatype, const cha void Storage::debugDump() { - for (auto p = mPageManager.begin(); p != mPageManager.end(); ++p) { + for(auto p = mPageManager.begin(); p != mPageManager.end(); ++p) { p->debugDump(); } } @@ -810,15 +916,15 @@ void Storage::debugCheck() { std::map keys; - for (auto p = mPageManager.begin(); p != mPageManager.end(); ++p) { + for(auto p = mPageManager.begin(); p != mPageManager.end(); ++p) { size_t itemIndex = 0; size_t usedCount = 0; Item item; - while (p->findItem(Page::NS_ANY, ItemType::ANY, nullptr, itemIndex, item) == ESP_OK) { + while(p->findItem(Page::NS_ANY, ItemType::ANY, nullptr, itemIndex, item) == ESP_OK) { std::stringstream keyrepr; keyrepr << static_cast(item.nsIndex) << "_" << static_cast(item.datatype) << "_" << item.key <<"_"<(item.chunkIndex); std::string keystr = keyrepr.str(); - if (keys.find(keystr) != std::end(keys)) { + if(keys.find(keystr) != std::end(keys)) { printf("Duplicate key: %s\n", keystr.c_str()); debugDump(); assert(0); @@ -842,24 +948,24 @@ esp_err_t Storage::calcEntriesInNamespace(uint8_t nsIndex, size_t& usedEntries) { usedEntries = 0; - if (mState != StorageState::ACTIVE) { + if(mState != StorageState::ACTIVE) { return ESP_ERR_NVS_NOT_INITIALIZED; } - for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { size_t itemIndex = 0; Item item; - while (true) { + while(true) { auto err = it->findItem(nsIndex, ItemType::ANY, nullptr, itemIndex, item); - if (err == ESP_ERR_NVS_NOT_FOUND) { + if(err == ESP_ERR_NVS_NOT_FOUND) { break; } - else if (err != ESP_OK) { + else if(err != ESP_OK) { return err; } usedEntries += item.span; itemIndex += item.span; - if (itemIndex >= it->ENTRY_COUNT) break; + if(itemIndex >= it->ENTRY_COUNT) break; } } return ESP_OK; @@ -871,7 +977,7 @@ void Storage::fillEntryInfo(Item &item, nvs_entry_info_t &info) strncpy(info.key, item.key, sizeof(info.key) - 1); info.key[sizeof(info.key) - 1] = '\0'; - for (auto &name : mNamespaces) { + for(auto &name : mNamespaces) { if(item.nsIndex == name.mIndex) { strlcpy(info.namespace_name, name.mName, sizeof(info.namespace_name)); break; @@ -885,7 +991,7 @@ bool Storage::findEntry(nvs_opaque_iterator_t* it, const char* namespace_name) it->nsIndex = Page::NS_ANY; it->page = mPageManager.begin(); - if (namespace_name != nullptr) { + if(namespace_name != nullptr) { if(createOrOpenNamespace(namespace_name, false, it->nsIndex) != ESP_OK) { return false; } @@ -913,7 +1019,7 @@ bool Storage::nextEntry(nvs_opaque_iterator_t* it) Item item; esp_err_t err; - for (auto page = it->page; page != mPageManager.end(); ++page) { + for(auto page = it->page; page != mPageManager.end(); ++page) { do { err = page->findItem(it->nsIndex, (ItemType)it->type, nullptr, it->entryIndex, item); it->entryIndex += item.span; @@ -922,7 +1028,7 @@ bool Storage::nextEntry(nvs_opaque_iterator_t* it) it->page = page; return true; } - } while (err != ESP_ERR_NVS_NOT_FOUND); + } while(err != ESP_ERR_NVS_NOT_FOUND); it->entryIndex = 0; } diff --git a/components/nvs_flash/src/nvs_storage.hpp b/components/nvs_flash/src/nvs_storage.hpp index 6dbb6b1007..eab1b9dfaf 100644 --- a/components/nvs_flash/src/nvs_storage.hpp +++ b/components/nvs_flash/src/nvs_storage.hpp @@ -74,6 +74,8 @@ public: esp_err_t readItem(uint8_t nsIndex, ItemType datatype, const char* key, void* data, size_t dataSize); + esp_err_t findKey(const uint8_t nsIndex, const char* key, ItemType* datatype); + esp_err_t getItemDataSize(uint8_t nsIndex, ItemType datatype, const char* key, size_t& dataSize); esp_err_t eraseItem(uint8_t nsIndex, ItemType datatype, const char* key); diff --git a/docs/en/api-reference/storage/nvs_flash.rst b/docs/en/api-reference/storage/nvs_flash.rst index b54715d939..fbfd461044 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 d3d33002eb..49d3f690f8 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`` 等其他类型数据的支持。 -键必须唯一。为现有的键写入新的值可能产生如下结果： +键必须唯一。为现有的键写入新值时，会将旧的值及数据类型更新为写入操作指定的值和数据类型。 -- 如果新旧值数据类型相同，则更新值； -- 如果新旧值数据类型不同，则返回错误。 - -读取值时也会执行数据类型检查。如果读取操作的数据类型与该值的数据类型不匹配，则返回错误。 +读取值时会执行数据类型检查。如果读取操作预期的数据类型与对应键的数据类型不匹配，则返回错误。 命名空间 diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 9e68f680cb..2964fa50d6 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -700,9 +700,6 @@ components/nvs_flash/include/nvs_flash.h components/nvs_flash/include/nvs_handle.hpp components/nvs_flash/src/nvs_cxx_api.cpp components/nvs_flash/src/nvs_encrypted_partition.hpp -components/nvs_flash/src/nvs_handle_locked.cpp -components/nvs_flash/src/nvs_handle_locked.hpp -components/nvs_flash/src/nvs_handle_simple.cpp components/nvs_flash/src/nvs_item_hash_list.cpp components/nvs_flash/src/nvs_pagemanager.hpp components/nvs_flash/src/nvs_partition.cpp From 683212f41e3f2ad5f45ab81b570bc3ac87516f99 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Fri, 9 May 2025 21:42:25 +0200 Subject: [PATCH 2/6] refactor(nvs_flash): Improved Blob performance The findItem method was improved to use a hash list in RAM when searching for BLOB data chunks The findItem method was extended with a parameter that returns the position of an item on the page, if it is found The algorithm for matching existing variable-length data (such as strings and BLOBs) with new values was enhanced by comparing the CRC32 of the data chunks before reading the data from flash --- components/nvs_flash/src/nvs_page.cpp | 87 +++++++--- components/nvs_flash/src/nvs_page.hpp | 2 + components/nvs_flash/src/nvs_storage.cpp | 205 +++++++++++++++-------- components/nvs_flash/src/nvs_storage.hpp | 2 +- components/nvs_flash/src/nvs_types.cpp | 5 +- components/nvs_flash/src/nvs_types.hpp | 2 +- 6 files changed, 209 insertions(+), 94 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 2465ded216..0ef29204e6 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -246,6 +246,43 @@ esp_err_t Page::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, c return ESP_OK; } +// Reads the data entries of the variable length item. +// The metadata entry is already read in the item object. +// index is the index of the metadata entry on the page. +// data is pointer to the buffer where the data will be copied to. It has to be at least +// item.varLength.dataSize bytes long. +// The function returns ESP_OK if the data was read successfully, or an error code if there was an error. +esp_err_t Page::readVariableLengthItemData(const Item& item, const size_t index, void* data) +{ + if (mState == PageState::INVALID) { + return ESP_ERR_NVS_INVALID_STATE; + } + + esp_err_t rc; + uint8_t* dst = reinterpret_cast(data); + size_t left = item.varLength.dataSize; + for (size_t i = index + 1; i < index + item.span; ++i) { + Item ditem; + rc = readEntry(i, ditem); + if (rc != ESP_OK) { + return rc; + } + size_t willCopy = ENTRY_SIZE; + willCopy = (left < willCopy) ? left : willCopy; + memcpy(dst, ditem.rawData, willCopy); + left -= willCopy; + dst += willCopy; + } + if (Item::calculateCrc32(reinterpret_cast(data), item.varLength.dataSize) != item.varLength.dataCrc32) { + rc = eraseEntryAndSpan(index); + if (rc != ESP_OK) { + return rc; + } + return ESP_ERR_NVS_NOT_FOUND; + } + return ESP_OK; +} + esp_err_t Page::readItem(uint8_t nsIndex, ItemType datatype, const char* key, void* data, size_t dataSize, uint8_t chunkIdx, VerOffset chunkStart) { size_t index = 0; @@ -273,28 +310,7 @@ esp_err_t Page::readItem(uint8_t nsIndex, ItemType datatype, const char* key, vo return ESP_ERR_NVS_INVALID_LENGTH; } - uint8_t* dst = reinterpret_cast(data); - size_t left = item.varLength.dataSize; - for (size_t i = index + 1; i < index + item.span; ++i) { - Item ditem; - rc = readEntry(i, ditem); - if (rc != ESP_OK) { - return rc; - } - size_t willCopy = ENTRY_SIZE; - willCopy = (left < willCopy) ? left : willCopy; - memcpy(dst, ditem.rawData, willCopy); - left -= willCopy; - dst += willCopy; - } - if (Item::calculateCrc32(reinterpret_cast(data), item.varLength.dataSize) != item.varLength.dataCrc32) { - rc = eraseEntryAndSpan(index); - if (rc != ESP_OK) { - return rc; - } - return ESP_ERR_NVS_NOT_FOUND; - } - return ESP_OK; + return readVariableLengthItemData(item, index, data); } esp_err_t Page::cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize, uint8_t chunkIdx, VerOffset chunkStart) @@ -326,9 +342,23 @@ esp_err_t Page::cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, con return ESP_ERR_NVS_INVALID_LENGTH; } + // We have metadata of the variable length data chunk. It contains the length of the data and the crc32. + // As a first step we can calculate the crc32 of the data buffer to be compared with the crc32 of the item in the flash. + // If they are not equal, immediately return ESP_ERR_NVS_CONTENT_DIFFERS. + // If they are equal, to avoid crc32 collision false positive, we will read the data from the flash entry by entry and compare + // it with the respective chunk of input data buffer. The crc32 of the data read from the flash will be calculated on the fly. + // At the end, we will compare the crc32 of the data read from the flash with the crc32 of the metadata item in the flash to make sure + // that the data in the flash is not corrupted. + if (Item::calculateCrc32(reinterpret_cast(data), item.varLength.dataSize) != item.varLength.dataCrc32) { + return ESP_ERR_NVS_CONTENT_DIFFERS; + } + const uint8_t* dst = reinterpret_cast(data); size_t left = item.varLength.dataSize; - for (size_t i = index + 1; i < index + item.span; ++i) { + uint32_t accumulatedCRC32; + size_t initial_index = index + 1; + + for (size_t i = initial_index; i < index + item.span; ++i) { Item ditem; rc = readEntry(i, ditem); if (rc != ESP_OK) { @@ -339,11 +369,18 @@ esp_err_t Page::cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, con if (memcmp(dst, ditem.rawData, willCopy)) { return ESP_ERR_NVS_CONTENT_DIFFERS; } + + // Calculate the crc32 of the actual ditem.rawData buffer. Do not pass accumulatedCRC32 in the first call. + // In the first call, calculateCrc32 will use its default. In the subsequent calls, accumulatedCRC32 is the crc32 of the previous buffer. + accumulatedCRC32 = Item::calculateCrc32(ditem.rawData, willCopy, (i == initial_index) ? nullptr : &accumulatedCRC32); + left -= willCopy; dst += willCopy; } - if (Item::calculateCrc32(reinterpret_cast(data), item.varLength.dataSize) != item.varLength.dataCrc32) { - return ESP_ERR_NVS_NOT_FOUND; + // Check if the CRC32 calculated on the fly matches the variable length data CRC32 indicated in the metadata entry. + // If they are not equal, it means the data in the flash is corrupt, we will return ESP_ERR_NVS_CONTENT_DIFFERS. + if (accumulatedCRC32 != item.varLength.dataCrc32) { + return ESP_ERR_NVS_CONTENT_DIFFERS; } return ESP_OK; diff --git a/components/nvs_flash/src/nvs_page.hpp b/components/nvs_flash/src/nvs_page.hpp index 4aff32ad01..2acaa64e83 100644 --- a/components/nvs_flash/src/nvs_page.hpp +++ b/components/nvs_flash/src/nvs_page.hpp @@ -88,6 +88,8 @@ public: esp_err_t writeItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize, uint8_t chunkIdx = CHUNK_ANY); + esp_err_t readVariableLengthItemData(const Item& item, const size_t index, void* data); + esp_err_t readItem(uint8_t nsIndex, ItemType datatype, const char* key, void* data, size_t dataSize, uint8_t chunkIdx = CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY); esp_err_t cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize, uint8_t chunkIdx = CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY); diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index a4974563c1..32d3ac311a 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -241,13 +241,16 @@ bool Storage::isValid() const return mState == StorageState::ACTIVE; } -esp_err_t Storage::findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item, uint8_t chunkIdx, VerOffset chunkStart) +esp_err_t Storage::findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item, uint8_t chunkIdx, VerOffset chunkStart, size_t* itemIndex) { for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { - size_t itemIndex = 0; - auto err = it->findItem(nsIndex, datatype, key, itemIndex, item, chunkIdx, chunkStart); + size_t tmpItemIndex = 0; + auto err = it->findItem(nsIndex, datatype, key, tmpItemIndex, item, chunkIdx, chunkStart); if(err == ESP_OK) { page = it; + if(itemIndex) { + *itemIndex = tmpItemIndex; + } return ESP_OK; } } @@ -360,7 +363,7 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo } // datatype BLOB is written as BLOB_INDEX and BLOB_DATA and is searched for previous value as BLOB_INDEX and/or BLOB -// datatype BLOB_IDX and BLOB_DATA are not supported as input parameters, the layer above should always use BLOB +// datatype BLOB_INDEX and BLOB_DATA are not supported as input parameters, the layer above should always use BLOB esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize) { if(mState != StorageState::ACTIVE) { @@ -369,13 +372,13 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key // pointer to the page where the existing item was found Page* findPage = nullptr; + // index of the item in the page where the existing item was found + size_t itemIndex = 0; // page sequence number helping to detect whether the page with old value was relocated during the new write uint32_t findPageSeqNumber = UINT32_MAX; // indicates the datatype representation match between the old value and the new one bool matchedTypePageFound = false; - - // contains the item with the old value, if found Item item; esp_err_t err = ESP_OK; @@ -385,23 +388,35 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if(datatype == ItemType::BLOB) { // Specific lookup if performed for input datatype BLOB. The searched datatype for exact match is BLOB_INDEX. // The BLOB_INDEX is used to store the metadata of the (originally typed) BLOB data in current V2 implementation. - err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); + err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); if(err == ESP_OK && findPage != nullptr) { matchedTypePageFound = true; } } else { // Handle all other data types than BLOB - err = findItem(nsIndex, datatype, key, findPage, item); + err = findItem(nsIndex, datatype, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); if(err == ESP_OK && findPage != nullptr) { matchedTypePageFound = true; + } + } +#ifndef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY + // If the item was not found under assumed datatype, try to find it as ANY. + if(findPage == nullptr) { + + // We should not find BLOB_DATA chunks as CHUNK_ANY is never used by the BLOB_DATA. + err = findItem(nsIndex, nvs::ItemType::ANY, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); + if(err == ESP_OK && findPage != nullptr) { // keep the sequence number of the page where the item was found for later check of relocation err = findPage->getSeqNumber(findPageSeqNumber); if(err != ESP_OK) { return err; } + // item was found with the same key and namespace index but data type is different + matchedTypePageFound = false; } } +#endif #ifndef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY // If the item was not found, try to find it with any other datatype. Omit the BLOB_DATA and handle BLOB with respect @@ -478,7 +493,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } - if(err != ESP_OK) { + if (err != ESP_OK) { return err; } } else { @@ -560,13 +575,14 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if(wasRelocated) { // The page was relocated. We have to find the old value again from the beginning. // As the item was already found before relocation, we can use the exact datatype from item. - err = findItem(nsIndex, item.datatype, key, findPage, item); + err = findItem(nsIndex, item.datatype, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); if(err != ESP_OK) { return err; } } + // Page containing the old value is now refreshed. We can erase the old value. - err = findPage->eraseItem(nsIndex, item.datatype, key); + err = findPage->eraseEntryAndSpan(itemIndex); if(err == ESP_ERR_FLASH_OP_FAIL) { return ESP_ERR_NVS_REMOVE_FAILED; } @@ -635,8 +651,10 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat { Item item; Page* findPage = nullptr; + size_t itemIndex = 0; - /* First read the blob index */ + + // First read the blob index auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); if(err != ESP_OK) { return err; @@ -648,24 +666,28 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat NVS_ASSERT_OR_RETURN(dataSize == item.blobIndex.dataSize, ESP_FAIL); - /* Now read corresponding chunks */ + // Now read related blob data chunks + // Remember the itemIndex as it is used to fast locate the entry in the page for(uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { - err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum); + err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum, nvs::VerOffset::VER_ANY, &itemIndex); if(err != ESP_OK) { if(err == ESP_ERR_NVS_NOT_FOUND) { break; } return err; } + + // Check if the blob data chunk length indicated for actual item still fits into the total length of the buffer if(item.varLength.dataSize > dataSize - offset) { - /* The size of the entry in the index is inconsistent with the sum of the sizes of chunks */ err = ESP_ERR_NVS_INVALID_LENGTH; break; } - err = findPage->readItem(nsIndex, ItemType::BLOB_DATA, key, static_cast(data) + offset, item.varLength.dataSize, static_cast (chunkStart) + chunkNum); + + err = findPage->readVariableLengthItemData(item, itemIndex, static_cast(data) + offset); if(err != ESP_OK) { return err; } + NVS_ASSERT_OR_RETURN(static_cast (chunkStart) + chunkNum == item.chunkIndex, ESP_FAIL); offset += item.varLength.dataSize; @@ -685,8 +707,9 @@ esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void { Item item; Page* findPage = nullptr; + size_t itemIndex = 0; - /* First read the blob index */ + // First read the blob index auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); if(err != ESP_OK) { return err; @@ -701,15 +724,28 @@ esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void return ESP_ERR_NVS_CONTENT_DIFFERS; } - /* Now read corresponding chunks */ + // Now read corresponding chunks for(uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) { - err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum); + err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast (chunkStart) + chunkNum, nvs::VerOffset::VER_ANY, &itemIndex); if(err != ESP_OK) { if(err == ESP_ERR_NVS_NOT_FOUND) { break; } return err; } + + if(item.varLength.dataSize > dataSize - offset) { + // The size of the entry in the index is bigger than the size of the remaining data to be compared + return ESP_ERR_NVS_CONTENT_DIFFERS; + } + + // calculate crc32 of the incoming data window related to the BLOB_DATA chunk and compare it with the crc32 from the BLOB_DATA metadata entry + // Different crc32 indicates data mismatch. + // If crc32 matches, we have to compare the data in the chunk with the buffer data to exclude crc32 collision. + if (Item::calculateCrc32(reinterpret_cast(data), item.varLength.dataSize) != item.varLength.dataCrc32) { + return ESP_ERR_NVS_CONTENT_DIFFERS; + } + err = findPage->cmpItem(nsIndex, ItemType::BLOB_DATA, key, static_cast(data) + offset, item.varLength.dataSize, static_cast (chunkStart) + chunkNum); if(err != ESP_OK) { return err; @@ -753,13 +789,18 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse } Item item; Page* findPage = nullptr; + size_t itemIndex = 0; + uint8_t chunkCount = 0; - auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart); + auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart, &itemIndex); if(err != ESP_OK) { return err; } + + chunkCount = item.blobIndex.chunkCount; + // Erase the index first and make children blobs orphan - err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart); + err = findPage->eraseEntryAndSpan(itemIndex); if(err != ESP_OK) { return err; } @@ -767,48 +808,76 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse // If caller requires delete of VER_ANY // We may face dirty NVS partition and version duplicates can be there // Make second attempt to delete index and ignore eventual not found - if(chunkStart == VerOffset::VER_ANY) - { - err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart); + if(chunkStart == VerOffset::VER_ANY) { + // Specific case called during initialisation of the storage + // We need to delete all chunks with the same key and namespace index + + // If there exists another BLOB_IDX with the same key and namespace index, delete it + // Ignore potential error if the item is not found + err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart, &itemIndex); if(err == ESP_OK) { - err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart); + err = findPage->eraseEntryAndSpan(itemIndex); if(err != ESP_OK) { return err; } } else if(err != ESP_ERR_NVS_NOT_FOUND) { return err; } - } - // setup limits for chunkIndex-es to be deleted - uint8_t minChunkIndex = (uint8_t) VerOffset::VER_0_OFFSET; - uint8_t maxChunkIndex = (uint8_t) VerOffset::VER_ANY; + // To delete all chunks, we will visit every page and delete all chunks regardless of chunkIndex + // This approach cannot use the hash list as the chunkIndex is not known. + for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + // reset itemIndex to zero for each page to search from the beginning + itemIndex = 0; + do { + // (Re)Try to find the item at the position starting at the itemIndex + err = it->findItem(nsIndex, ItemType::BLOB_DATA, key, itemIndex, item); - if(chunkStart == VerOffset::VER_0_OFFSET) { - maxChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET; - } else if(chunkStart == VerOffset::VER_1_OFFSET) { - minChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET; - } - - for(auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { - size_t itemIndex = 0; - do { - err = it->findItem(nsIndex, ItemType::BLOB_DATA, key, itemIndex, item); - if(err == ESP_ERR_NVS_NOT_FOUND) { - break; - } else if(err == ESP_OK) { - // check if item.chunkIndex is within the version range indicated by chunkStart, if so, delete it - if((item.chunkIndex >= minChunkIndex) && (item.chunkIndex < maxChunkIndex)) { + // If the item is not found, we can break the actual loop and continue with the next page + if(err == ESP_ERR_NVS_NOT_FOUND) { + break; + } else if(err == ESP_OK) { err = it->eraseEntryAndSpan(itemIndex); - } - // continue findItem until end of page - itemIndex += item.span; - } + // advance itemIndex to the next potential entry on the page + // findItem checks the consistency of the entry metadata so we can safely assume the span is non-zero + itemIndex += item.span; + } + if(err != ESP_OK) { + return err; + } + // Continue the loop until all items on the current page are found and erased + } while(err == ESP_OK && itemIndex < Page::ENTRY_COUNT); + } + + } else { + // Most common condition + // The caller has specified the chunk version, delete all chunks within the chunk index range indicated by the BLOB_IDX entry + // The loop will iterate the chunk index, page will be found and chunk index will be erased + // This approach uses the hash list to find the item on the page, so it is efficient. + uint8_t minChunkIndex = (uint8_t) VerOffset::VER_ANY; + uint8_t maxChunkIndex = (uint8_t) VerOffset::VER_ANY; + + if(chunkStart == VerOffset::VER_0_OFFSET) { + minChunkIndex = (uint8_t) VerOffset::VER_0_OFFSET; + maxChunkIndex = minChunkIndex + chunkCount; + } else if(chunkStart == VerOffset::VER_1_OFFSET) { + minChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET; + maxChunkIndex = minChunkIndex + chunkCount; + } + + for(uint8_t chunkIndex = minChunkIndex; chunkIndex < maxChunkIndex; chunkIndex++) { + err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, chunkIndex, nvs::VerOffset::VER_ANY, &itemIndex); if(err != ESP_OK) { return err; } - } while(err == ESP_OK && itemIndex < Page::ENTRY_COUNT); + + // Erase the entry + err = findPage->eraseEntryAndSpan(itemIndex); + if(err != ESP_OK) { + return err; + } + } } return ESP_OK; @@ -820,22 +889,21 @@ esp_err_t Storage::eraseItem(uint8_t nsIndex, ItemType datatype, const char* key return ESP_ERR_NVS_NOT_INITIALIZED; } - if(datatype == ItemType::BLOB) { - return eraseMultiPageBlob(nsIndex, key); - } - Item item; Page* findPage = nullptr; - auto err = findItem(nsIndex, datatype, key, findPage, item); + esp_err_t err = ESP_OK; + size_t itemIndex = 0; + + err = findItem(nsIndex, datatype, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); if(err != ESP_OK) { return err; } - - if(item.datatype == ItemType::BLOB_DATA || item.datatype == ItemType::BLOB_IDX) { - return eraseMultiPageBlob(nsIndex, key); + // If the item found is BLOB_IDX, the eraseMultiPageBlob is used to erase the whole multi-page blob. + if (item.datatype == ItemType::BLOB_IDX) { + return eraseMultiPageBlob(nsIndex, key, item.blobIndex.chunkStart); } - return findPage->eraseItem(nsIndex, datatype, key); + return findPage->eraseEntryAndSpan(itemIndex); } esp_err_t Storage::eraseNamespace(uint8_t nsIndex) @@ -887,19 +955,24 @@ esp_err_t Storage::getItemDataSize(uint8_t nsIndex, ItemType datatype, const cha Item item; Page* findPage = nullptr; - auto err = findItem(nsIndex, datatype, key, findPage, item); - if(err != ESP_OK) { - if(datatype != ItemType::BLOB) { - return err; - } + esp_err_t err = ESP_OK; + + // If requested datatype is BLOB, first try to find the item with datatype BLOB_IDX - new format + // If not found, try to find the item with datatype BLOB - old format. + if(datatype == ItemType::BLOB) { err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item); - if(err != ESP_OK) { + if(err == ESP_OK) { + dataSize = item.blobIndex.dataSize; + return err; + } else if(err != ESP_ERR_NVS_NOT_FOUND) { return err; } - dataSize = item.blobIndex.dataSize; - return ESP_OK; } + err = findItem(nsIndex, datatype, key, findPage, item); + if(err != ESP_OK) { + return err; + } dataSize = item.varLength.dataSize; return ESP_OK; } diff --git a/components/nvs_flash/src/nvs_storage.hpp b/components/nvs_flash/src/nvs_storage.hpp index eab1b9dfaf..b2376e6fa3 100644 --- a/components/nvs_flash/src/nvs_storage.hpp +++ b/components/nvs_flash/src/nvs_storage.hpp @@ -151,7 +151,7 @@ protected: void fillEntryInfo(Item &item, nvs_entry_info_t &info); - esp_err_t findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item, uint8_t chunkIdx = Page::CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY); + esp_err_t findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item, uint8_t chunkIdx = Page::CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY, size_t* itemIndex = NULL); protected: Partition *mPartition; diff --git a/components/nvs_flash/src/nvs_types.cpp b/components/nvs_flash/src/nvs_types.cpp index 24073fc306..81379c8591 100644 --- a/components/nvs_flash/src/nvs_types.cpp +++ b/components/nvs_flash/src/nvs_types.cpp @@ -33,9 +33,12 @@ uint32_t Item::calculateCrc32WithoutValue() const return result; } -uint32_t Item::calculateCrc32(const uint8_t* data, size_t size) +uint32_t Item::calculateCrc32(const uint8_t* data, size_t size, uint32_t* initial_crc32) { uint32_t result = 0xffffffff; + if(initial_crc32) { + result = *initial_crc32; + } result = esp_rom_crc32_le(result, data, size); return result; } diff --git a/components/nvs_flash/src/nvs_types.hpp b/components/nvs_flash/src/nvs_types.hpp index 5eda9abcd9..5cb3056a45 100644 --- a/components/nvs_flash/src/nvs_types.hpp +++ b/components/nvs_flash/src/nvs_types.hpp @@ -93,7 +93,7 @@ public: uint32_t calculateCrc32() const; uint32_t calculateCrc32WithoutValue() const; - static uint32_t calculateCrc32(const uint8_t* data, size_t size); + static uint32_t calculateCrc32(const uint8_t* data, size_t size, uint32_t* initial_crc32 = nullptr); void getKey(char* dst, size_t dstSize) { From 95e169b5f2c55c50612be7abdc97ea99e18645ff Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 27 Nov 2023 14:33:38 +0100 Subject: [PATCH 3/6] ci(nvs_flash): upgrade to Catch2 as a component, fix warnings --- .../host_test/nvs_host_test/CMakeLists.txt | 4 +--- .../host_test/nvs_host_test/main/CMakeLists.txt | 12 +++++++++--- .../nvs_host_test/main/idf_component.yml | 2 ++ .../host_test/nvs_host_test/main/main.cpp | 7 ------- .../host_test/nvs_host_test/main/test_nvs.cpp | 15 ++++----------- .../nvs_host_test/main/test_nvs_cxx_api.cpp | 2 +- .../nvs_host_test/main/test_nvs_handle.cpp | 2 +- .../main/test_nvs_initialization.cpp | 4 +--- .../nvs_host_test/main/test_nvs_storage.cpp | 2 +- .../nvs_host_test/main/test_partition_manager.cpp | 2 +- 10 files changed, 21 insertions(+), 31 deletions(-) create mode 100644 components/nvs_flash/host_test/nvs_host_test/main/idf_component.yml delete mode 100644 components/nvs_flash/host_test/nvs_host_test/main/main.cpp diff --git a/components/nvs_flash/host_test/nvs_host_test/CMakeLists.txt b/components/nvs_flash/host_test/nvs_host_test/CMakeLists.txt index cd3f3ebece..658d311f49 100644 --- a/components/nvs_flash/host_test/nvs_host_test/CMakeLists.txt +++ b/components/nvs_flash/host_test/nvs_host_test/CMakeLists.txt @@ -2,9 +2,7 @@ cmake_minimum_required(VERSION 3.16) include($ENV{IDF_PATH}/tools/cmake/project.cmake) set(COMPONENTS main) -# Freertos is included via common components. However, CATCH isn't compatible with the FreeRTOS component yet, hence -# using the FreeRTOS mock component. -# target. +# This test app doesn't require FreeRTOS, using mock instead list(APPEND EXTRA_COMPONENT_DIRS "$ENV{IDF_PATH}/tools/mocks/freertos/") project(nvs_host_test) diff --git a/components/nvs_flash/host_test/nvs_host_test/main/CMakeLists.txt b/components/nvs_flash/host_test/nvs_host_test/main/CMakeLists.txt index 05663cb008..534f968c53 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/CMakeLists.txt +++ b/components/nvs_flash/host_test/nvs_host_test/main/CMakeLists.txt @@ -1,5 +1,4 @@ -idf_component_register(SRCS "main.cpp" - "test_nvs.cpp" +idf_component_register(SRCS "test_nvs.cpp" "test_partition_manager.cpp" "test_nvs_cxx_api.cpp" "test_nvs_handle.cpp" @@ -9,6 +8,13 @@ idf_component_register(SRCS "main.cpp" "../../../src" "../../../private_include" "../../../../mbedtls/mbedtls/include" - "../../../../../tools/catch" WHOLE_ARCHIVE REQUIRES nvs_flash) + +if(CMAKE_C_COMPILER_ID MATCHES "Clang") + target_compile_options(${COMPONENT_LIB} PRIVATE -std=gnu++20) +endif() + +# Currently 'main' for IDF_TARGET=linux is defined in freertos component. +# Since we are using a freertos mock here, need to let Catch2 provide 'main'. +target_link_libraries(${COMPONENT_LIB} PRIVATE Catch2WithMain) diff --git a/components/nvs_flash/host_test/nvs_host_test/main/idf_component.yml b/components/nvs_flash/host_test/nvs_host_test/main/idf_component.yml new file mode 100644 index 0000000000..f7982136b9 --- /dev/null +++ b/components/nvs_flash/host_test/nvs_host_test/main/idf_component.yml @@ -0,0 +1,2 @@ +dependencies: + espressif/catch2: "^3.4.0" diff --git a/components/nvs_flash/host_test/nvs_host_test/main/main.cpp b/components/nvs_flash/host_test/nvs_host_test/main/main.cpp deleted file mode 100644 index dddf85b1d5..0000000000 --- a/components/nvs_flash/host_test/nvs_host_test/main/main.cpp +++ /dev/null @@ -1,7 +0,0 @@ -/* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD - * - * SPDX-License-Identifier: Apache-2.0 - */ -#define CATCH_CONFIG_MAIN -#include "catch.hpp" 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 1da5e8ca80..184e216b3c 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 @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include "nvs.hpp" #include "sdkconfig.h" #include "nvs_partition_manager.hpp" @@ -381,8 +381,7 @@ TEST_CASE("storage can find items on second page if first is not fully written a { PartitionEmulationFixture f(0, 3); nvs::Storage storage(f.part()); - CHECK(storage.init(0, 3) == ESP_OK); - int bar = 0; + TEST_ESP_OK(storage.init(0, 3)); uint8_t bigdata[(nvs::Page::CHUNK_MAX_SIZE - nvs::Page::ENTRY_SIZE) / 2] = {0}; // write one big chunk of data ESP_ERROR_CHECK(storage.writeItem(0, nvs::ItemType::BLOB, "1", bigdata, sizeof(bigdata))); @@ -632,8 +631,6 @@ TEST_CASE("deinit partition doesn't affect other partition's open handles", "[nv const char *OTHER_PARTITION_NAME = "other_part"; PartitionEmulationFixture f(0, 10); PartitionEmulationFixture f_other(0, 10, OTHER_PARTITION_NAME); - const char *str = "value 0123456789abcdef0123456789abcdef"; - const uint8_t blob[8] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}; nvs_handle_t handle_1; const uint32_t NVS_FLASH_SECTOR = 6; @@ -693,7 +690,6 @@ TEST_CASE("nvs_entry_info fails with ESP_ERR_INVALID_ARG if a parameter is NULL" TEST_CASE("nvs_entry_info doesn't change iterator on parameter error", "[nvs]") { nvs_iterator_t it = reinterpret_cast(0xbeef); - nvs_entry_info_t info; REQUIRE(nvs_entry_info(it, nullptr) == ESP_ERR_INVALID_ARG); CHECK(it == reinterpret_cast(0xbeef)); @@ -972,8 +968,8 @@ TEST_CASE("wifi test", "[nvs]") TEST_ESP_OK(nvs_set_u8(net80211_handle, "wifi.opmode", opmode)); uint8_t country = 0; - TEST_ESP_ERR(nvs_get_u8(net80211_handle, "wifi.country", &opmode), ESP_ERR_NVS_NOT_FOUND); - TEST_ESP_OK(nvs_set_u8(net80211_handle, "wifi.country", opmode)); + TEST_ESP_ERR(nvs_get_u8(net80211_handle, "wifi.country", &country), ESP_ERR_NVS_NOT_FOUND); + TEST_ESP_OK(nvs_set_u8(net80211_handle, "wifi.country", country)); char ssid[36]; size_t size = sizeof(ssid); @@ -1239,7 +1235,6 @@ public: case nvs::ItemType::SZ: { char buf[strBufLen]; - size_t len = strBufLen; size_t strLen = gen() % (strBufLen - 1); std::generate_n(buf, strLen, [&]() -> char { @@ -1751,8 +1746,6 @@ TEST_CASE("Check that orphaned blobs are erased during init", "[nvs]") { const size_t blob_size = nvs::Page::CHUNK_MAX_SIZE * 3 ; uint8_t blob[blob_size] = {0x11}; - uint8_t blob2[blob_size] = {0x22}; - uint8_t blob3[blob_size] = {0x33}; PartitionEmulationFixture f(0, 5); nvs::Storage storage(f.part()); diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_cxx_api.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_cxx_api.cpp index 84070fa6d1..d4ebef3a35 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_cxx_api.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_cxx_api.cpp @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include #include #include "nvs_handle_simple.hpp" diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_handle.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_handle.cpp index 6c6e35e1d1..dae567d446 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_handle.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_handle.cpp @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include #include #include "nvs_handle_simple.hpp" diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_initialization.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_initialization.cpp index 2482129535..cd52f19a81 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_initialization.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_initialization.cpp @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include "nvs.hpp" #include "nvs_partition_manager.hpp" #include "nvs_partition.hpp" @@ -12,8 +12,6 @@ TEST_CASE("nvs_flash_init_partition_ptr fails due to nullptr arg", "[nvs_custom_part]") { - const uint32_t NVS_FLASH_SECTOR = 6; - const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 3; uint8_t *p_part_desc_addr_start; CHECK(esp_partition_file_mmap((const uint8_t **)&p_part_desc_addr_start) == ESP_OK); diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_storage.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_storage.cpp index 7e19ce7d5d..769c661260 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_storage.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_nvs_storage.cpp @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include #include "nvs_storage.hpp" #include "nvs_partition_manager.hpp" diff --git a/components/nvs_flash/host_test/nvs_host_test/main/test_partition_manager.cpp b/components/nvs_flash/host_test/nvs_host_test/main/test_partition_manager.cpp index 2a67579a87..fa134ea226 100644 --- a/components/nvs_flash/host_test/nvs_host_test/main/test_partition_manager.cpp +++ b/components/nvs_flash/host_test/nvs_host_test/main/test_partition_manager.cpp @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "catch.hpp" +#include #include #include #include "nvs_handle_simple.hpp" From 7027c06b0ab4b8e5cd5818d44d3366ae10f9e559 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Mon, 23 Jun 2025 09:38:21 +0200 Subject: [PATCH 4/6] fix(nvs_flash): Host test adopted to the new clang initializer rules --- .../host_test/nvs_host_test/main/test_nvs.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 184e216b3c..5b8bfb6f73 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 @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -591,7 +591,7 @@ TEST_CASE("nvs api tests", "[nvs]") nvs_handle_t handle_2; TEST_ESP_OK(nvs_open("namespace2", NVS_READWRITE, &handle_2)); TEST_ESP_OK(nvs_set_i32(handle_2, "foo", 0x3456789a)); - const char *str = "value 0123456789abcdef0123456789abcdef"; + char str[] = "value 0123456789abcdef0123456789abcdef"; TEST_ESP_OK(nvs_set_str(handle_2, "key", str)); int32_t v1; @@ -602,7 +602,7 @@ TEST_CASE("nvs api tests", "[nvs]") TEST_ESP_OK(nvs_get_i32(handle_2, "foo", &v2)); CHECK(0x3456789a == v2); - char buf[strlen(str) + 1]; + char buf[sizeof(str)]; size_t buf_len = sizeof(buf); size_t buf_len_needed; @@ -1171,12 +1171,13 @@ public: blobBufLen = largeBlobLen ; } - uint8_t buf[blobBufLen]; + uint8_t* buf = new uint8_t[blobBufLen]; memset(buf, 0, blobBufLen); size_t len = blobBufLen; auto err = nvs_get_blob(handle, keys[index], buf, &len); if (err == ESP_ERR_FLASH_OP_FAIL) { + delete [] buf; return err; } if (!written[index]) { @@ -1185,6 +1186,7 @@ public: REQUIRE(err == ESP_OK); REQUIRE(memcmp(buf, reinterpret_cast(values[index]), blobBufLen) == 0); } + delete [] buf; break; } @@ -1265,7 +1267,7 @@ public: } else { blobBufLen = largeBlobLen ; } - uint8_t buf[blobBufLen]; + uint8_t* buf = new uint8_t[blobBufLen]; memset(buf, 0, blobBufLen); size_t blobLen = gen() % blobBufLen; std::generate_n(buf, blobLen, [&]() -> uint8_t { @@ -1274,16 +1276,19 @@ public: auto err = nvs_set_blob(handle, keys[index], buf, blobLen); if (err == ESP_ERR_FLASH_OP_FAIL) { + delete [] buf; return err; } if (err == ESP_ERR_NVS_REMOVE_FAILED) { written[index] = true; memcpy(reinterpret_cast(values[index]), buf, blobBufLen); + delete [] buf; return ESP_ERR_FLASH_OP_FAIL; } REQUIRE(err == ESP_OK); written[index] = true; memcpy(reinterpret_cast(values[index]), buf, blobBufLen); + delete [] buf; break; } From dbd0bc3f4afce589300d6e5c69aef9f32a851f9f Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Wed, 3 Sep 2025 16:59:01 +0200 Subject: [PATCH 5/6] fix(nvs_flash): Host test adopted to the actual release 5.1 - added 2 configurations in order host tests to be executed in CI - removed test cases not supported by the actual linux partition emulation --- .../host_test/nvs_host_test/main/test_nvs.cpp | 60 ++++++++++++++++++- .../nvs_host_test/pytest_nvs_host_linux.py | 3 + 2 files changed, 62 insertions(+), 1 deletion(-) 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 5b8bfb6f73..16cb9084f4 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 @@ -503,7 +503,7 @@ TEST_CASE("erase operations are distributed among sectors", "[nvs]") } /* Check that erase counts are distributed between the remaining sectors */ - const size_t max_erase_cnt = write_ops / nvs::Page::ENTRY_COUNT / (sectors - static_sectors) + 1; + TEMPORARILY_DISABLED(const size_t max_erase_cnt = write_ops / nvs::Page::ENTRY_COUNT / (sectors - static_sectors) + 1;) for (size_t i = 0; i < sectors; ++i) { TEMPORARILY_DISABLED( auto erase_cnt = f.emu.getSectorEraseCount(i); @@ -2046,3 +2046,61 @@ TEST_CASE("Recovery from power-off during modification of blob present in old-fo TEST_ESP_OK(nvs_flash_deinit_partition(f.part()->get_partition_name())); } + +TEST_CASE("Page handles invalid CRC of variable length items", "[nvs][cur]") +{ + PartitionEmulationFixture f(0, 4); + { + nvs::Page p; + TEST_ESP_OK(p.load(f.part(), 0)); + char buf[128] = {0}; + TEST_ESP_OK(p.writeItem(1, nvs::ItemType::BLOB, "1", buf, sizeof(buf))); + } + // corrupt header of the item (64 is the offset of the first item in page) + uint32_t overwrite_buf = 0; + TEST_ESP_OK(esp_partition_write(&f.esp_partition, 64, &overwrite_buf, 4)); + // load page again + { + nvs::Page p1; + TEST_ESP_OK(p1.load(f.part(), 0)); + } +} + +TEST_CASE("namespace name is deep copy", "[nvs]") +{ + char ns_name[16]; + strcpy(ns_name, "const_name"); + + nvs_handle_t handle_1; + nvs_handle_t handle_2; + const uint32_t NVS_FLASH_SECTOR = 6; + const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 3; + + PartitionEmulationFixture f(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN); + + TEMPORARILY_DISABLED(f.emu.setBounds(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN);) + + TEST_ESP_OK( nvs::NVSPartitionManager::get_instance()->init_custom(f.part(), + NVS_FLASH_SECTOR, + NVS_FLASH_SECTOR_COUNT_MIN)); + + TEST_ESP_OK(nvs_open("const_name", NVS_READWRITE, &handle_1)); + strcpy(ns_name, "just_kidding"); + + CHECK(nvs_open("just_kidding", NVS_READONLY, &handle_2) == ESP_ERR_NVS_NOT_FOUND); + + nvs_close(handle_1); + nvs_close(handle_2); + + nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME); +} + +/* Add new tests above */ +/* This test has to be the final one */ + +TEST_CASE("dump all performance data", "[nvs]") +{ + std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl; + std::cout << s_perf.str() << std::endl; + std::cout << "====================" << std::endl; +} diff --git a/components/nvs_flash/host_test/nvs_host_test/pytest_nvs_host_linux.py b/components/nvs_flash/host_test/nvs_host_test/pytest_nvs_host_linux.py index bc3828ada2..e57657aebf 100644 --- a/components/nvs_flash/host_test/nvs_host_test/pytest_nvs_host_linux.py +++ b/components/nvs_flash/host_test/nvs_host_test/pytest_nvs_host_linux.py @@ -6,5 +6,8 @@ from pytest_embedded import Dut @pytest.mark.linux @pytest.mark.host_test +@pytest.mark.parametrize('config', [ + 'default_set_key', 'legacy_set_key' +], indirect=True) def test_nvs_host_linux(dut: Dut) -> None: dut.expect_exact('All tests passed', timeout=60) From 13cc380dbf67cf5ffdb59368f8ca47b274e64114 Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Thu, 4 Sep 2025 09:49:18 +0200 Subject: [PATCH 6/6] fix(nvs_flash): Fixed overwrite of V1 BLOB when legacy compatibility mode is in place --- components/nvs_flash/src/nvs_storage.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 32d3ac311a..88c39834f1 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -392,6 +392,27 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if(err == ESP_OK && findPage != nullptr) { matchedTypePageFound = true; } +#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY + // In legacy mode, we also try to find the item as BLOB if not found as BLOB_INDEX. + // In this mode, it is possible to have multiple active values under the same (logical) key. + // For BLOBs (which may have different physical representations in V1 it is BLOB, in V2 it is BLOB_INDEX) it in turn means + // that we have to check both datatypes to find the old value. + // The general case for compatibility flag disabled is below and handles all datatypes including BLOB. + // To save some cycles, we do not compile both findItem calls in this case. + if(err == ESP_ERR_NVS_NOT_FOUND) { + // If not found as BLOB_INDEX, try to find it as BLOB (legacy support). + err = findItem(nsIndex, ItemType::BLOB, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex); + if(err == ESP_OK && findPage != nullptr) { + matchedTypePageFound = false; // datatype does not match, we cannot extract chunkStart from the item + + // keep the sequence number of the page where the item was found for later check of relocation + err = findPage->getSeqNumber(findPageSeqNumber); + if(err != ESP_OK) { + return err; + } + } + } +#endif } else { // Handle all other data types than BLOB err = findItem(nsIndex, datatype, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex);