Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvs: Check if an item is modified before writing out an identical copy (IDFGH-977) #3310

Merged
merged 1 commit into from
May 21, 2019
Merged

nvs: Check if an item is modified before writing out an identical copy (IDFGH-977) #3310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/nvs_flash/include/nvs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ typedef uint32_t nvs_handle;
#define ESP_ERR_NVS_KEYS_NOT_INITIALIZED (ESP_ERR_NVS_BASE + 0x16) /*!< NVS key partition is uninitialized */
#define ESP_ERR_NVS_CORRUPT_KEY_PART (ESP_ERR_NVS_BASE + 0x17) /*!< NVS key partition is corrupt */

#define ESP_ERR_NVS_CONTENT_DIFFERS (ESP_ERR_NVS_BASE + 0x18) /*!< Internal error; never returned by nvs API functions. NVS key is different in comparison */

#define NVS_DEFAULT_PART_NAME "nvs" /*!< Default partition name of the NVS partition in the partition table */
/**
Expand Down
52 changes: 52 additions & 0 deletions components/nvs_flash/src/nvs_page.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,58 @@ esp_err_t Page::readItem(uint8_t nsIndex, ItemType datatype, const char* key, vo
return ESP_OK;
}

esp_err_t Page::cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize, uint8_t chunkIdx, VerOffset chunkStart)
{
size_t index = 0;
Item item;

if (mState == PageState::INVALID) {
return ESP_ERR_NVS_INVALID_STATE;
}

esp_err_t rc = findItem(nsIndex, datatype, key, index, item, chunkIdx, chunkStart);
if (rc != ESP_OK) {
return rc;
}

if (!isVariableLengthType(datatype)) {
if (dataSize != getAlignmentForType(datatype)) {
return ESP_ERR_NVS_TYPE_MISMATCH;
}

if (memcmp(data, item.data, dataSize)) {
return ESP_ERR_NVS_CONTENT_DIFFERS;
}
return ESP_OK;
}

if (dataSize < static_cast<size_t>(item.varLength.dataSize)) {
return ESP_ERR_NVS_INVALID_LENGTH;
}

const uint8_t* dst = reinterpret_cast<const uint8_t*>(data);
size_t left = item.varLength.dataSize;
for (size_t i = index + 1; i < index + item.span; ++i) {
tim-nordell-nimbelink marked this conversation as resolved.
Show resolved Hide resolved
Item ditem;
rc = readEntry(i, ditem);
if (rc != ESP_OK) {
return rc;
}
size_t willCopy = ENTRY_SIZE;
willCopy = (left < willCopy)?left:willCopy;
if (memcmp(dst, ditem.rawData, willCopy)) {
return ESP_ERR_NVS_CONTENT_DIFFERS;
}
left -= willCopy;
dst += willCopy;
}
if (Item::calculateCrc32(reinterpret_cast<const uint8_t*>(data), item.varLength.dataSize) != item.varLength.dataCrc32) {
return ESP_ERR_NVS_NOT_FOUND;
}

return ESP_OK;
}

esp_err_t Page::eraseItem(uint8_t nsIndex, ItemType datatype, const char* key, uint8_t chunkIdx, VerOffset chunkStart)
{
size_t index = 0;
Expand Down
8 changes: 8 additions & 0 deletions components/nvs_flash/src/nvs_page.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class Page : public intrusive_list_node<Page>

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);

esp_err_t eraseItem(uint8_t nsIndex, ItemType datatype, const char* key, uint8_t chunkIdx = CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY);

esp_err_t findItem(uint8_t nsIndex, ItemType datatype, const char* key, uint8_t chunkIdx = CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY);
Expand All @@ -112,6 +114,12 @@ class Page : public intrusive_list_node<Page>
return readItem(nsIndex, itemTypeOf(value), key, &value, sizeof(value));
}

template<typename T>
esp_err_t cmpItem(uint8_t nsIndex, const char* key, const T& value)
{
return cmpItem(nsIndex, itemTypeOf(value), key, &value, sizeof(value));
}

template<typename T>
esp_err_t eraseItem(uint8_t nsIndex, const char* key)
{
Expand Down
56 changes: 56 additions & 0 deletions components/nvs_flash/src/nvs_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,13 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
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 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) {
return ESP_OK;
}

if (findPage->state() == Page::PageState::UNINITIALIZED ||
findPage->state() == Page::PageState::INVALID) {
ESP_ERROR_CHECK(findItem(nsIndex, datatype, key, findPage, item));
Expand Down Expand Up @@ -311,6 +318,13 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
}
}
} else {
// Do a sanity check that the item in question is actually being modified.
// If it isn't, it is cheaper to purposefully not write out new data.
// since it may invoke an erasure of flash.
if (findPage != nullptr &&
findPage->cmpItem(nsIndex, datatype, key, data, dataSize) == ESP_OK) {
return ESP_OK;
}

Page& page = getCurrentPage();
err = page.writeItem(nsIndex, datatype, key, data, dataSize);
Expand Down Expand Up @@ -443,6 +457,48 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat
return err;
}

esp_err_t Storage::cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void* data, size_t dataSize)
{
Item item;
Page* findPage = nullptr;

/* First read the blob index */
auto err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item);
if (err != ESP_OK) {
return err;
}

uint8_t chunkCount = item.blobIndex.chunkCount;
VerOffset chunkStart = item.blobIndex.chunkStart;
size_t readSize = item.blobIndex.dataSize;
size_t offset = 0;

if (dataSize != readSize) {
return ESP_ERR_NVS_CONTENT_DIFFERS;
}

/* Now read corresponding chunks */
for (uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) {
err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast<uint8_t> (chunkStart) + chunkNum);
if (err != ESP_OK) {
if (err == ESP_ERR_NVS_NOT_FOUND) {
break;
}
return err;
}
err = findPage->cmpItem(nsIndex, ItemType::BLOB_DATA, key, static_cast<const uint8_t*>(data) + offset, item.varLength.dataSize, static_cast<uint8_t> (chunkStart) + chunkNum);
if (err != ESP_OK) {
return err;
}
assert(static_cast<uint8_t> (chunkStart) + chunkNum == item.chunkIndex);
offset += item.varLength.dataSize;
}
if (err == ESP_OK) {
assert(offset == dataSize);
}
return err;
}

esp_err_t Storage::readItem(uint8_t nsIndex, ItemType datatype, const char* key, void* data, size_t dataSize)
{
if (mState != StorageState::ACTIVE) {
Expand Down
2 changes: 2 additions & 0 deletions components/nvs_flash/src/nvs_storage.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ class Storage : public intrusive_list_node<Storage>

esp_err_t readMultiPageBlob(uint8_t nsIndex, const char* key, void* data, size_t dataSize);

esp_err_t cmpMultiPageBlob(uint8_t nsIndex, const char* key, const void* data, size_t dataSize);

esp_err_t eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffset chunkStart = VerOffset::VER_ANY);

void debugDump();
Expand Down
62 changes: 57 additions & 5 deletions components/nvs_flash/test_nvs_host/test_nvs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,8 @@ TEST_CASE("storage doesn't add duplicates within one page", "[nvs]")
emu.setBounds(4, 8);
CHECK(storage.init(4, 4) == ESP_OK);
int bar = 0;
CHECK(storage.writeItem(1, "bar", bar) == ESP_OK);
CHECK(storage.writeItem(1, "bar", bar) == ESP_OK);
CHECK(storage.writeItem(1, "bar", ++bar) == ESP_OK);
CHECK(storage.writeItem(1, "bar", ++bar) == ESP_OK);

Page page;
page.load(4);
Expand All @@ -404,11 +404,11 @@ TEST_CASE("storage doesn't add duplicates within multiple pages", "[nvs]")
emu.setBounds(4, 8);
CHECK(storage.init(4, 4) == ESP_OK);
int bar = 0;
CHECK(storage.writeItem(1, "bar", bar) == ESP_OK);
CHECK(storage.writeItem(1, "bar", ++bar) == ESP_OK);
for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) {
CHECK(storage.writeItem(1, "foo", static_cast<int>(bar)) == ESP_OK);
CHECK(storage.writeItem(1, "foo", static_cast<int>(++bar)) == ESP_OK);
}
CHECK(storage.writeItem(1, "bar", bar) == ESP_OK);
CHECK(storage.writeItem(1, "bar", ++bar) == ESP_OK);

Page page;
page.load(4);
Expand Down Expand Up @@ -750,6 +750,58 @@ TEST_CASE("wifi test", "[nvs]")

}

TEST_CASE("writing the identical content does not write or erase", "[nvs]")
{
SpiFlashEmulator emu(20);

const uint32_t NVS_FLASH_SECTOR = 5;
const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 10;
emu.setBounds(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN);
TEST_ESP_OK(nvs_flash_init_custom(NVS_DEFAULT_PART_NAME, NVS_FLASH_SECTOR, NVS_FLASH_SECTOR_COUNT_MIN));

nvs_handle misc_handle;
TEST_ESP_OK(nvs_open("test", NVS_READWRITE, &misc_handle));

// Test writing a u8 twice, then changing it
nvs_set_u8(misc_handle, "test_u8", 8);
emu.clearStats();
nvs_set_u8(misc_handle, "test_u8", 8);
CHECK(emu.getWriteOps() == 0);
CHECK(emu.getEraseOps() == 0);
CHECK(emu.getReadOps() != 0);
emu.clearStats();
nvs_set_u8(misc_handle, "test_u8", 9);
CHECK(emu.getWriteOps() != 0);
CHECK(emu.getReadOps() != 0);

// Test writing a string twice, then changing it
static const char *test[2] = {"Hello world.", "Hello world!"};
nvs_set_str(misc_handle, "test_str", test[0]);
emu.clearStats();
nvs_set_str(misc_handle, "test_str", test[0]);
CHECK(emu.getWriteOps() == 0);
CHECK(emu.getEraseOps() == 0);
CHECK(emu.getReadOps() != 0);
emu.clearStats();
nvs_set_str(misc_handle, "test_str", test[1]);
CHECK(emu.getWriteOps() != 0);
CHECK(emu.getReadOps() != 0);

// Test writing a multi-page blob, then changing it
uint8_t blob[Page::CHUNK_MAX_SIZE * 3] = {0};
memset(blob, 1, sizeof(blob));
nvs_set_blob(misc_handle, "test_blob", blob, sizeof(blob));
emu.clearStats();
nvs_set_blob(misc_handle, "test_blob", blob, sizeof(blob));
CHECK(emu.getWriteOps() == 0);
CHECK(emu.getEraseOps() == 0);
CHECK(emu.getReadOps() != 0);
blob[sizeof(blob) - 1]++;
emu.clearStats();
nvs_set_blob(misc_handle, "test_blob", blob, sizeof(blob));
CHECK(emu.getWriteOps() != 0);
CHECK(emu.getReadOps() != 0);
}

TEST_CASE("can init storage from flash with random contents", "[nvs]")
{
Expand Down