diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index e483c596..845dd091 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.cpp +++ b/components/nvs_flash/src/nvs_item_hash_list.cpp @@ -60,7 +60,7 @@ void HashList::insert(const Item& item, size_t index) newBlock->mCount++; } -void HashList::erase(size_t index) +void HashList::erase(size_t index, bool itemShouldExist) { for (auto it = mBlockList.begin(); it != mBlockList.end();) { bool haveEntries = false; @@ -82,7 +82,9 @@ void HashList::erase(size_t index) ++it; } } - assert(false && "item should have been present in cache"); + if (itemShouldExist) { + assert(false && "item should have been present in cache"); + } } size_t HashList::find(size_t start, const Item& item) diff --git a/components/nvs_flash/src/nvs_item_hash_list.hpp b/components/nvs_flash/src/nvs_item_hash_list.hpp index 3f8dcc85..e759cd81 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.hpp +++ b/components/nvs_flash/src/nvs_item_hash_list.hpp @@ -29,7 +29,7 @@ public: ~HashList(); void insert(const Item& item, size_t index); - void erase(const size_t index); + void erase(const size_t index, bool itemShouldExist=true); size_t find(size_t start, const Item& item); void clear(); diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 2a17cff2..efadc185 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -318,7 +318,6 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) { auto state = mEntryTable.get(index); assert(state == EntryState::WRITTEN || state == EntryState::EMPTY); - mHashList.erase(index); size_t span = 1; if (state == EntryState::WRITTEN) { @@ -328,6 +327,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } if (item.calculateCrc32() != item.crc32) { + mHashList.erase(index, false); rc = alterEntryState(index, EntryState::ERASED); --mUsedEntryCount; ++mErasedEntryCount; @@ -335,6 +335,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } } else { + mHashList.erase(index); span = item.span; for (ptrdiff_t i = index + span - 1; i >= static_cast(index); --i) { if (mEntryTable.get(i) == EntryState::WRITTEN) { @@ -385,7 +386,7 @@ void Page::updateFirstUsedEntry(size_t index, size_t span) } } -esp_err_t Page::moveItem(Page& other) +esp_err_t Page::copyItems(Page& other) { if (mFirstUsedEntry == INVALID_ENTRY) { return ESP_ERR_NVS_NOT_FOUND; @@ -399,29 +400,41 @@ esp_err_t Page::moveItem(Page& other) } Item entry; - auto err = readEntry(mFirstUsedEntry, entry); - if (err != ESP_OK) { - return err; - } - other.mHashList.insert(entry, other.mNextFreeEntry); - err = other.writeEntry(entry); - if (err != ESP_OK) { - return err; - } + size_t readEntryIndex = mFirstUsedEntry; - size_t span = entry.span; - size_t end = mFirstUsedEntry + span; + while (readEntryIndex < ENTRY_COUNT) { - assert(mFirstUsedEntry != INVALID_ENTRY || span == 1); + if (mEntryTable.get(readEntryIndex) != EntryState::WRITTEN) { + assert(readEntryIndex != mFirstUsedEntry); + readEntryIndex++; + continue; + } + auto err = readEntry(readEntryIndex, entry); + if (err != ESP_OK) { + return err; + } - for (size_t i = mFirstUsedEntry + 1; i < end; ++i) { - readEntry(i, entry); + other.mHashList.insert(entry, other.mNextFreeEntry); err = other.writeEntry(entry); if (err != ESP_OK) { return err; } + size_t span = entry.span; + size_t end = readEntryIndex + span; + + assert(end <= ENTRY_COUNT); + + for (size_t i = readEntryIndex + 1; i < end; ++i) { + readEntry(i, entry); + err = other.writeEntry(entry); + if (err != ESP_OK) { + return err; + } + } + readEntryIndex = end; + } - return eraseEntryAndSpan(mFirstUsedEntry); + return ESP_OK; } esp_err_t Page::mLoadEntryTable() @@ -515,11 +528,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - - // search for potential duplicate item - size_t duplicateIndex = mHashList.find(0, item); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -529,6 +537,10 @@ esp_err_t Page::mLoadEntryTable() continue; } + mHashList.insert(item, i); + + // search for potential duplicate item + size_t duplicateIndex = mHashList.find(0, item); if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { span = item.span; @@ -580,8 +592,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -590,10 +600,20 @@ esp_err_t Page::mLoadEntryTable() } continue; } - assert(item.span > 0); + mHashList.insert(item, i); size_t span = item.span; + + if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { + for (size_t j = i + 1; j < i + span; ++j) { + if (mEntryTable.get(j) != EntryState::WRITTEN) { + eraseEntryAndSpan(i); + break; + } + } + } + i += span - 1; } @@ -730,7 +750,11 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si auto crc32 = item.calculateCrc32(); if (item.crc32 != crc32) { - eraseEntryAndSpan(i); + rc = eraseEntryAndSpan(i); + if (rc != ESP_OK) { + mState = PageState::INVALID; + return rc; + } continue; } diff --git a/components/nvs_flash/src/nvs_page.hpp b/components/nvs_flash/src/nvs_page.hpp index 7731e403..413da458 100644 --- a/components/nvs_flash/src/nvs_page.hpp +++ b/components/nvs_flash/src/nvs_page.hpp @@ -127,7 +127,7 @@ public: esp_err_t markFreeing(); - esp_err_t moveItem(Page& other); + esp_err_t copyItems(Page& other); esp_err_t erase(); diff --git a/components/nvs_flash/src/nvs_pagemanager.cpp b/components/nvs_flash/src/nvs_pagemanager.cpp index 943f54f2..31240d98 100644 --- a/components/nvs_flash/src/nvs_pagemanager.cpp +++ b/components/nvs_flash/src/nvs_pagemanager.cpp @@ -67,7 +67,9 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) if (lastItemIndex != SIZE_MAX) { auto last = PageManager::TPageListIterator(&lastPage); for (auto it = begin(); it != last; ++it) { - if (it->eraseItem(item.nsIndex, item.datatype, item.key) == ESP_OK) { + + if ((it->state() != Page::PageState::FREEING) && + (it->eraseItem(item.nsIndex, item.datatype, item.key) == ESP_OK)) { break; } } @@ -77,23 +79,26 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) for (auto it = begin(); it!= end(); ++it) { if (it->state() == Page::PageState::FREEING) { Page* newPage = &mPageList.back(); - if (newPage->state() != Page::PageState::ACTIVE) { - auto err = activatePage(); + if (newPage->state() == Page::PageState::ACTIVE) { + auto err = newPage->erase(); if (err != ESP_OK) { return err; } - newPage = &mPageList.back(); + mPageList.erase(newPage); + mFreePageList.push_back(newPage); } - while (true) { - auto err = it->moveItem(*newPage); - if (err == ESP_ERR_NVS_NOT_FOUND) { - break; - } else if (err != ESP_OK) { - return err; - } + auto err = activatePage(); + if (err != ESP_OK) { + return err; + } + newPage = &mPageList.back(); + + err = it->copyItems(*newPage); + if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { + return err; } - auto err = it->erase(); + err = it->erase(); if (err != ESP_OK) { return err; } @@ -109,7 +114,7 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) if (mFreePageList.size() == 0) { return ESP_ERR_NVS_NO_FREE_PAGES; } - + return ESP_OK; } @@ -125,17 +130,18 @@ esp_err_t PageManager::requestNewPage() } // find the page with the higest number of erased items - TPageListIterator maxErasedItemsPageIt; - size_t maxErasedItems = 0; + TPageListIterator maxUnusedItemsPageIt; + size_t maxUnusedItems = 0; for (auto it = begin(); it != end(); ++it) { - auto erased = it->getErasedEntryCount(); - if (erased > maxErasedItems) { - maxErasedItemsPageIt = it; - maxErasedItems = erased; + + auto unused = Page::ENTRY_COUNT - it->getUsedEntryCount(); + if (unused > maxUnusedItems) { + maxUnusedItemsPageIt = it; + maxUnusedItems = unused; } } - if (maxErasedItems == 0) { + if (maxUnusedItems == 0) { return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } @@ -146,7 +152,8 @@ esp_err_t PageManager::requestNewPage() Page* newPage = &mPageList.back(); - Page* erasedPage = maxErasedItemsPageIt; + Page* erasedPage = maxUnusedItemsPageIt; + #ifndef NDEBUG size_t usedEntries = erasedPage->getUsedEntryCount(); #endif @@ -154,13 +161,9 @@ esp_err_t PageManager::requestNewPage() if (err != ESP_OK) { return err; } - while (true) { - err = erasedPage->moveItem(*newPage); - if (err == ESP_ERR_NVS_NOT_FOUND) { - break; - } else if (err != ESP_OK) { - return err; - } + err = erasedPage->copyItems(*newPage); + if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { + return err; } err = erasedPage->erase(); @@ -171,8 +174,8 @@ esp_err_t PageManager::requestNewPage() #ifndef NDEBUG assert(usedEntries == newPage->getUsedEntryCount()); #endif - - mPageList.erase(maxErasedItemsPageIt); + + mPageList.erase(maxUnusedItemsPageIt); mFreePageList.push_back(erasedPage); return ESP_OK; diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index f419256c..c23f5d66 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -132,14 +132,14 @@ TEST_CASE("when writing and erasing, used/erased counts are updated correctly", CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.getUsedEntryCount() == Page::ENTRY_COUNT - 1); CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.eraseItem(1, itemTypeOf(), name) == ESP_OK); } CHECK(page.getUsedEntryCount() == 1); @@ -153,7 +153,7 @@ TEST_CASE("when page is full, adding an element fails", "[nvs]") CHECK(page.load(0) == ESP_OK); for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.writeItem(1, "foo", 64UL) == ESP_ERR_NVS_PAGE_FULL); @@ -259,6 +259,25 @@ TEST_CASE("Page validates blob size", "[nvs]") TEST_ESP_OK(page.writeItem(1, ItemType::BLOB, "2", buf, Page::BLOB_MAX_SIZE)); } +TEST_CASE("Page handles invalid CRC of variable length items", "[nvs][cur]") +{ + SpiFlashEmulator emu(4); + { + Page page; + TEST_ESP_OK(page.load(0)); + char buf[128] = {0}; + TEST_ESP_OK(page.writeItem(1, 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; + emu.write(64, &overwrite_buf, 4); + // load page again + { + Page page; + TEST_ESP_OK(page.load(0)); + } +} + TEST_CASE("can init PageManager in empty flash", "[nvs]") { SpiFlashEmulator emu(4); @@ -1123,7 +1142,7 @@ TEST_CASE("crc errors in item header are handled", "[nvs]") // add more items to make the page full for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char item_name[Item::MAX_KEY_LENGTH + 1]; - snprintf(item_name, sizeof(item_name), "item_%ld", i); + snprintf(item_name, sizeof(item_name), "item_%ld", (long int)i); TEST_ESP_OK(storage.writeItem(1, item_name, static_cast(i))); } @@ -1246,6 +1265,27 @@ TEST_CASE("multiple partitions access check", "[nvs]") CHECK(v2 == 0xcafebabe); } +TEST_CASE("nvs page selection takes into account free entries also not just erased entries", "[nvs]") +{ + const size_t blob_size = Page::BLOB_MAX_SIZE; + uint8_t blob[blob_size] = {0}; + SpiFlashEmulator emu(3); + TEST_ESP_OK( nvs_flash_init_custom(NVS_DEFAULT_PART_NAME, 0, 3) ); + nvs_handle handle; + TEST_ESP_OK( nvs_open("test", NVS_READWRITE, &handle) ); + // Fill first page + TEST_ESP_OK( nvs_set_blob(handle, "1a", blob, blob_size/3) ); + TEST_ESP_OK( nvs_set_blob(handle, "1b", blob, blob_size) ); + // Fill second page + TEST_ESP_OK( nvs_set_blob(handle, "2a", blob, blob_size) ); + TEST_ESP_OK( nvs_set_blob(handle, "2b", blob, blob_size) ); + + // The item below should be able to fit the first page. + TEST_ESP_OK( nvs_set_blob(handle, "3a", blob, 4) ); + TEST_ESP_OK( nvs_commit(handle) ); + nvs_close(handle); +} + TEST_CASE("dump all performance data", "[nvs]") { std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl;