Merge branch 'bugfix/sync_bugfix_from_esp_idf' into 'master'

Sync bugfix from esp-idf

See merge request sdk/ESP8266_RTOS_SDK!725
This commit is contained in:
Dong Heng
2019-01-22 11:59:48 +08:00
6 changed files with 133 additions and 64 deletions

View File

@@ -60,7 +60,7 @@ void HashList::insert(const Item& item, size_t index)
newBlock->mCount++; newBlock->mCount++;
} }
void HashList::erase(size_t index) void HashList::erase(size_t index, bool itemShouldExist)
{ {
for (auto it = mBlockList.begin(); it != mBlockList.end();) { for (auto it = mBlockList.begin(); it != mBlockList.end();) {
bool haveEntries = false; bool haveEntries = false;
@@ -82,8 +82,10 @@ void HashList::erase(size_t index)
++it; ++it;
} }
} }
if (itemShouldExist) {
assert(false && "item should have been present in cache"); assert(false && "item should have been present in cache");
} }
}
size_t HashList::find(size_t start, const Item& item) size_t HashList::find(size_t start, const Item& item)
{ {

View File

@@ -29,7 +29,7 @@ public:
~HashList(); ~HashList();
void insert(const Item& item, size_t index); 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); size_t find(size_t start, const Item& item);
void clear(); void clear();

View File

@@ -318,7 +318,6 @@ esp_err_t Page::eraseEntryAndSpan(size_t index)
{ {
auto state = mEntryTable.get(index); auto state = mEntryTable.get(index);
assert(state == EntryState::WRITTEN || state == EntryState::EMPTY); assert(state == EntryState::WRITTEN || state == EntryState::EMPTY);
mHashList.erase(index);
size_t span = 1; size_t span = 1;
if (state == EntryState::WRITTEN) { if (state == EntryState::WRITTEN) {
@@ -328,6 +327,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index)
return rc; return rc;
} }
if (item.calculateCrc32() != item.crc32) { if (item.calculateCrc32() != item.crc32) {
mHashList.erase(index, false);
rc = alterEntryState(index, EntryState::ERASED); rc = alterEntryState(index, EntryState::ERASED);
--mUsedEntryCount; --mUsedEntryCount;
++mErasedEntryCount; ++mErasedEntryCount;
@@ -335,6 +335,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index)
return rc; return rc;
} }
} else { } else {
mHashList.erase(index);
span = item.span; span = item.span;
for (ptrdiff_t i = index + span - 1; i >= static_cast<ptrdiff_t>(index); --i) { for (ptrdiff_t i = index + span - 1; i >= static_cast<ptrdiff_t>(index); --i) {
if (mEntryTable.get(i) == EntryState::WRITTEN) { 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) { if (mFirstUsedEntry == INVALID_ENTRY) {
return ESP_ERR_NVS_NOT_FOUND; return ESP_ERR_NVS_NOT_FOUND;
@@ -399,29 +400,41 @@ esp_err_t Page::moveItem(Page& other)
} }
Item entry; Item entry;
auto err = readEntry(mFirstUsedEntry, entry); size_t readEntryIndex = mFirstUsedEntry;
while (readEntryIndex < ENTRY_COUNT) {
if (mEntryTable.get(readEntryIndex) != EntryState::WRITTEN) {
assert(readEntryIndex != mFirstUsedEntry);
readEntryIndex++;
continue;
}
auto err = readEntry(readEntryIndex, entry);
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
other.mHashList.insert(entry, other.mNextFreeEntry); other.mHashList.insert(entry, other.mNextFreeEntry);
err = other.writeEntry(entry); err = other.writeEntry(entry);
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
size_t span = entry.span; size_t span = entry.span;
size_t end = mFirstUsedEntry + span; size_t end = readEntryIndex + span;
assert(mFirstUsedEntry != INVALID_ENTRY || span == 1); assert(end <= ENTRY_COUNT);
for (size_t i = mFirstUsedEntry + 1; i < end; ++i) { for (size_t i = readEntryIndex + 1; i < end; ++i) {
readEntry(i, entry); readEntry(i, entry);
err = other.writeEntry(entry); err = other.writeEntry(entry);
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
} }
return eraseEntryAndSpan(mFirstUsedEntry); readEntryIndex = end;
}
return ESP_OK;
} }
esp_err_t Page::mLoadEntryTable() esp_err_t Page::mLoadEntryTable()
@@ -515,11 +528,6 @@ esp_err_t Page::mLoadEntryTable()
return err; return err;
} }
mHashList.insert(item, i);
// search for potential duplicate item
size_t duplicateIndex = mHashList.find(0, item);
if (item.crc32 != item.calculateCrc32()) { if (item.crc32 != item.calculateCrc32()) {
err = eraseEntryAndSpan(i); err = eraseEntryAndSpan(i);
if (err != ESP_OK) { if (err != ESP_OK) {
@@ -529,6 +537,10 @@ esp_err_t Page::mLoadEntryTable()
continue; 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) { if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) {
span = item.span; span = item.span;
@@ -580,8 +592,6 @@ esp_err_t Page::mLoadEntryTable()
return err; return err;
} }
mHashList.insert(item, i);
if (item.crc32 != item.calculateCrc32()) { if (item.crc32 != item.calculateCrc32()) {
err = eraseEntryAndSpan(i); err = eraseEntryAndSpan(i);
if (err != ESP_OK) { if (err != ESP_OK) {
@@ -590,10 +600,20 @@ esp_err_t Page::mLoadEntryTable()
} }
continue; continue;
} }
assert(item.span > 0); assert(item.span > 0);
mHashList.insert(item, i);
size_t span = item.span; 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; 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(); auto crc32 = item.calculateCrc32();
if (item.crc32 != crc32) { if (item.crc32 != crc32) {
eraseEntryAndSpan(i); rc = eraseEntryAndSpan(i);
if (rc != ESP_OK) {
mState = PageState::INVALID;
return rc;
}
continue; continue;
} }

View File

@@ -127,7 +127,7 @@ public:
esp_err_t markFreeing(); esp_err_t markFreeing();
esp_err_t moveItem(Page& other); esp_err_t copyItems(Page& other);
esp_err_t erase(); esp_err_t erase();

View File

@@ -67,7 +67,9 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount)
if (lastItemIndex != SIZE_MAX) { if (lastItemIndex != SIZE_MAX) {
auto last = PageManager::TPageListIterator(&lastPage); auto last = PageManager::TPageListIterator(&lastPage);
for (auto it = begin(); it != last; ++it) { 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; break;
} }
} }
@@ -77,23 +79,26 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount)
for (auto it = begin(); it!= end(); ++it) { for (auto it = begin(); it!= end(); ++it) {
if (it->state() == Page::PageState::FREEING) { if (it->state() == Page::PageState::FREEING) {
Page* newPage = &mPageList.back(); Page* newPage = &mPageList.back();
if (newPage->state() != Page::PageState::ACTIVE) { if (newPage->state() == Page::PageState::ACTIVE) {
auto err = newPage->erase();
if (err != ESP_OK) {
return err;
}
mPageList.erase(newPage);
mFreePageList.push_back(newPage);
}
auto err = activatePage(); auto err = activatePage();
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
newPage = &mPageList.back(); newPage = &mPageList.back();
}
while (true) { err = it->copyItems(*newPage);
auto err = it->moveItem(*newPage); if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
if (err == ESP_ERR_NVS_NOT_FOUND) {
break;
} else if (err != ESP_OK) {
return err; return err;
} }
}
auto err = it->erase(); err = it->erase();
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
@@ -125,17 +130,18 @@ esp_err_t PageManager::requestNewPage()
} }
// find the page with the higest number of erased items // find the page with the higest number of erased items
TPageListIterator maxErasedItemsPageIt; TPageListIterator maxUnusedItemsPageIt;
size_t maxErasedItems = 0; size_t maxUnusedItems = 0;
for (auto it = begin(); it != end(); ++it) { for (auto it = begin(); it != end(); ++it) {
auto erased = it->getErasedEntryCount();
if (erased > maxErasedItems) { auto unused = Page::ENTRY_COUNT - it->getUsedEntryCount();
maxErasedItemsPageIt = it; if (unused > maxUnusedItems) {
maxErasedItems = erased; maxUnusedItemsPageIt = it;
maxUnusedItems = unused;
} }
} }
if (maxErasedItems == 0) { if (maxUnusedItems == 0) {
return ESP_ERR_NVS_NOT_ENOUGH_SPACE; return ESP_ERR_NVS_NOT_ENOUGH_SPACE;
} }
@@ -146,7 +152,8 @@ esp_err_t PageManager::requestNewPage()
Page* newPage = &mPageList.back(); Page* newPage = &mPageList.back();
Page* erasedPage = maxErasedItemsPageIt; Page* erasedPage = maxUnusedItemsPageIt;
#ifndef NDEBUG #ifndef NDEBUG
size_t usedEntries = erasedPage->getUsedEntryCount(); size_t usedEntries = erasedPage->getUsedEntryCount();
#endif #endif
@@ -154,14 +161,10 @@ esp_err_t PageManager::requestNewPage()
if (err != ESP_OK) { if (err != ESP_OK) {
return err; return err;
} }
while (true) { err = erasedPage->copyItems(*newPage);
err = erasedPage->moveItem(*newPage); if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
if (err == ESP_ERR_NVS_NOT_FOUND) {
break;
} else if (err != ESP_OK) {
return err; return err;
} }
}
err = erasedPage->erase(); err = erasedPage->erase();
if (err != ESP_OK) { if (err != ESP_OK) {
@@ -172,7 +175,7 @@ esp_err_t PageManager::requestNewPage()
assert(usedEntries == newPage->getUsedEntryCount()); assert(usedEntries == newPage->getUsedEntryCount());
#endif #endif
mPageList.erase(maxErasedItemsPageIt); mPageList.erase(maxUnusedItemsPageIt);
mFreePageList.push_back(erasedPage); mFreePageList.push_back(erasedPage);
return ESP_OK; return ESP_OK;

View File

@@ -132,14 +132,14 @@ TEST_CASE("when writing and erasing, used/erased counts are updated correctly",
CHECK(page.getErasedEntryCount() == 1); CHECK(page.getErasedEntryCount() == 1);
for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) {
char name[16]; 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, name, i) == ESP_OK);
} }
CHECK(page.getUsedEntryCount() == Page::ENTRY_COUNT - 1); CHECK(page.getUsedEntryCount() == Page::ENTRY_COUNT - 1);
CHECK(page.getErasedEntryCount() == 1); CHECK(page.getErasedEntryCount() == 1);
for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) {
char name[16]; char name[16];
snprintf(name, sizeof(name), "i%ld", i); snprintf(name, sizeof(name), "i%ld", (long int)i);
CHECK(page.eraseItem(1, itemTypeOf<size_t>(), name) == ESP_OK); CHECK(page.eraseItem(1, itemTypeOf<size_t>(), name) == ESP_OK);
} }
CHECK(page.getUsedEntryCount() == 1); 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); CHECK(page.load(0) == ESP_OK);
for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) {
char name[16]; 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, name, i) == ESP_OK);
} }
CHECK(page.writeItem(1, "foo", 64UL) == ESP_ERR_NVS_PAGE_FULL); 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_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]") TEST_CASE("can init PageManager in empty flash", "[nvs]")
{ {
SpiFlashEmulator emu(4); 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 // add more items to make the page full
for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) {
char item_name[Item::MAX_KEY_LENGTH + 1]; 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<uint32_t>(i))); TEST_ESP_OK(storage.writeItem(1, item_name, static_cast<uint32_t>(i)));
} }
@@ -1246,6 +1265,27 @@ TEST_CASE("multiple partitions access check", "[nvs]")
CHECK(v2 == 0xcafebabe); 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]") TEST_CASE("dump all performance data", "[nvs]")
{ {
std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl; std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl;