From 08e41973f0f0041d77da775e5e899e4a7b3a45b8 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 15 Aug 2022 23:12:20 +0200 Subject: [PATCH 1/6] nvs_flash: update intrusive_list for compatibility with C++17 std::iterator is deprecated since C++17, the code produces a warning when compiled with clang and libc++. --- components/nvs_flash/src/intrusive_list.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/components/nvs_flash/src/intrusive_list.h b/components/nvs_flash/src/intrusive_list.h index 798b718a66..9a8c6f228e 100644 --- a/components/nvs_flash/src/intrusive_list.h +++ b/components/nvs_flash/src/intrusive_list.h @@ -30,9 +30,14 @@ class intrusive_list public: - class iterator : public std::iterator + class iterator { public: + using iterator_category = std::forward_iterator_tag; + using value_type = T; + using difference_type = ptrdiff_t; + using pointer = T*; + using reference = T&; iterator() : mPos(nullptr) {} From bfb6f318728f8aff44070fbfc86a4da87f712199 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 15 Aug 2022 23:26:29 +0200 Subject: [PATCH 2/6] nvs_flash: remove unnecessary check that the data pointer is in DROM spi_flash driver knows how to write data from cache (DROM or PSRAM) into flash, so the extra check in nvs_flash is unnecessary. Besides, the hardcoded address limit (0x3ff00000) is wrong for some of the newer chips. --- components/nvs_flash/src/nvs_page.cpp | 28 +-------------------------- 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index dbae73060e..36cde3db25 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -128,37 +128,11 @@ esp_err_t Page::writeEntryData(const uint8_t* data, size_t size) NVS_ASSERT_OR_RETURN(mFirstUsedEntry != INVALID_ENTRY, ESP_FAIL); const uint16_t count = size / ENTRY_SIZE; - const uint8_t* buf = data; - -#if !defined LINUX_TARGET - // TODO: check whether still necessary with esp_partition* API - /* On the ESP32, data can come from DROM, which is not accessible by spi_flash_write - * function. To work around this, we copy the data to heap if it came from DROM. - * Hopefully this won't happen very often in practice. For data from DRAM, we should - * still be able to write it to flash directly. - * TODO: figure out how to make this platform-specific check nicer (probably by introducing - * a platform-specific flash layer). - */ - if ((uint32_t) data < 0x3ff00000) { - buf = (uint8_t*) malloc(size); - if (!buf) { - return ESP_ERR_NO_MEM; - } - memcpy((void*)buf, data, size); - } -#endif // ! LINUX_TARGET - uint32_t phyAddr; esp_err_t rc = getEntryAddress(mNextFreeEntry, &phyAddr); if (rc == ESP_OK) { - rc = mPartition->write(phyAddr, buf, size); + rc = mPartition->write(phyAddr, data, size); } - -#if !defined LINUX_TARGET - if (buf != data) { - free((void*)buf); - } -#endif // ! LINUX_TARGET if (rc != ESP_OK) { mState = PageState::INVALID; return rc; From 02661081be8f1d2c14d33fa0a760d70a57359ea9 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 15 Aug 2022 23:27:26 +0200 Subject: [PATCH 3/6] nvs_flash: make nvs_flash_init usable for Linux target Since the partition API is now supported for Linux target builds, this function can now be used. --- components/nvs_flash/src/nvs_api.cpp | 4 ++-- components/nvs_flash/test_nvs_host/Makefile | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/components/nvs_flash/src/nvs_api.cpp b/components/nvs_flash/src/nvs_api.cpp index 8d63c25469..6d7f546050 100644 --- a/components/nvs_flash/src/nvs_api.cpp +++ b/components/nvs_flash/src/nvs_api.cpp @@ -116,7 +116,7 @@ extern "C" esp_err_t nvs_flash_init_partition_ptr(const esp_partition_t *partiti return init_res; } -#ifndef LINUX_TARGET +#ifndef LINUX_HOST_LEGACY_TEST extern "C" esp_err_t nvs_flash_init_partition(const char *part_name) { esp_err_t lock_result = Lock::init(); @@ -239,7 +239,7 @@ extern "C" esp_err_t nvs_flash_erase(void) { return nvs_flash_erase_partition(NVS_DEFAULT_PART_NAME); } -#endif // ! LINUX_TARGET +#endif // LINUX_HOST_LEGACY_TEST extern "C" esp_err_t nvs_flash_deinit_partition(const char* partition_name) { diff --git a/components/nvs_flash/test_nvs_host/Makefile b/components/nvs_flash/test_nvs_host/Makefile index 10669bfb6f..60bd9e802a 100644 --- a/components/nvs_flash/test_nvs_host/Makefile +++ b/components/nvs_flash/test_nvs_host/Makefile @@ -39,8 +39,8 @@ COMPILER := gcc endif CPPFLAGS += -I../private_include -I../include -I../src -I../../esp_rom/include -I../../esp_rom/include/linux -I../../log/include -I./ -I../../esp_common/include -I../../esp32/include -I ../../mbedtls/mbedtls/include -I ../../spi_flash/include -I ../../hal/include -I ../../xtensa/include -I ../../../tools/catch -fprofile-arcs -ftest-coverage -g2 -ggdb -CFLAGS += -fprofile-arcs -ftest-coverage -DLINUX_TARGET -CXXFLAGS += -std=c++11 -Wall -Werror -DLINUX_TARGET +CFLAGS += -fprofile-arcs -ftest-coverage -DLINUX_TARGET -DLINUX_HOST_LEGACY_TEST +CXXFLAGS += -std=c++11 -Wall -Werror -DLINUX_TARGET -DLINUX_HOST_LEGACY_TEST LDFLAGS += -lstdc++ -Wall -fprofile-arcs -ftest-coverage ifeq ($(COMPILER),clang) From bee241b0e2cb93b62cf6fe29315dce2bd57bb62b Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 15 Aug 2022 23:28:07 +0200 Subject: [PATCH 4/6] nvs_flash: remove leftover ESP_ERROR_CHECKS Similar to the asserts, these are replaced by error checks. --- components/nvs_flash/src/nvs_pagemanager.cpp | 5 ++++- components/nvs_flash/src/nvs_storage.cpp | 10 ++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/components/nvs_flash/src/nvs_pagemanager.cpp b/components/nvs_flash/src/nvs_pagemanager.cpp index cdc6b221dd..062f627743 100644 --- a/components/nvs_flash/src/nvs_pagemanager.cpp +++ b/components/nvs_flash/src/nvs_pagemanager.cpp @@ -47,7 +47,10 @@ esp_err_t PageManager::load(Partition *partition, uint32_t baseSector, uint32_t return activatePage(); } else { uint32_t lastSeqNo; - ESP_ERROR_CHECK( mPageList.back().getSeqNumber(lastSeqNo) ); + auto err = mPageList.back().getSeqNumber(lastSeqNo); + if (err != ESP_OK) { + return err; + } mSeqNumber = lastSeqNo + 1; } diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 1cf3b94859..17cc5648a1 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -305,7 +305,10 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if (findPage->state() == Page::PageState::UNINITIALIZED || findPage->state() == Page::PageState::INVALID) { - ESP_ERROR_CHECK(findItem(nsIndex, datatype, key, findPage, item)); + err = findItem(nsIndex, datatype, key, findPage, item); + if (err != ESP_OK) { + return err; + } } /* Get the version of the previous index with same */ prevStart = item.blobIndex.chunkStart; @@ -383,7 +386,10 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key if (findPage) { if (findPage->state() == Page::PageState::UNINITIALIZED || findPage->state() == Page::PageState::INVALID) { - ESP_ERROR_CHECK(findItem(nsIndex, datatype, key, findPage, item)); + err = findItem(nsIndex, datatype, key, findPage, item); + if (err != ESP_OK) { + return err; + } } err = findPage->eraseItem(nsIndex, datatype, key); if (err == ESP_ERR_FLASH_OP_FAIL) { From 7a1e19edf1f4261070f312a974458a2eba8bae42 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 15 Aug 2022 23:29:25 +0200 Subject: [PATCH 5/6] nvs_flash: replace strncpy + manual null termination with strlcpy Since libbsd is now a build dependency on Linux, strncpy can be replaced with the safer and less verbose strlcpy. --- components/nvs_flash/src/nvs_storage.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 17cc5648a1..03172f1a96 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -4,6 +4,10 @@ * SPDX-License-Identifier: Apache-2.0 */ #include "nvs_storage.hpp" +#if __has_include() +// for strlcpy +#include +#endif #ifndef ESP_PLATFORM // We need NO_DEBUG_STORAGE here since the integration tests on the host add some debug code. @@ -755,11 +759,7 @@ void Storage::fillEntryInfo(Item &item, nvs_entry_info_t &info) for (auto &name : mNamespaces) { if(item.nsIndex == name.mIndex) { -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wstringop-truncation" - strncpy(info.namespace_name, name.mName, sizeof(info.namespace_name) - 1); -#pragma GCC diagnostic pop - info.namespace_name[sizeof(info.namespace_name) -1] = '\0'; + strlcpy(info.namespace_name, name.mName, sizeof(info.namespace_name)); break; } } From 4e0fa26e640873e3993728948faf8f2de557de58 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 17 Aug 2022 12:20:25 +0200 Subject: [PATCH 6/6] nvs_flash: link the host test against libbsd when building on Linux ...and clean up the linking rule --- components/nvs_flash/test_nvs_host/Makefile | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/components/nvs_flash/test_nvs_host/Makefile b/components/nvs_flash/test_nvs_host/Makefile index 60bd9e802a..647c3d1491 100644 --- a/components/nvs_flash/test_nvs_host/Makefile +++ b/components/nvs_flash/test_nvs_host/Makefile @@ -43,6 +43,10 @@ CFLAGS += -fprofile-arcs -ftest-coverage -DLINUX_TARGET -DLINUX_HOST_LEGACY_TEST CXXFLAGS += -std=c++11 -Wall -Werror -DLINUX_TARGET -DLINUX_HOST_LEGACY_TEST LDFLAGS += -lstdc++ -Wall -fprofile-arcs -ftest-coverage +ifeq ($(shell uname -s),Linux) +LDFLAGS += -lbsd +endif + ifeq ($(COMPILER),clang) CFLAGS += -fsanitize=address CXXFLAGS += -fsanitize=address @@ -53,13 +57,16 @@ OBJ_FILES = $(SOURCE_FILES:.cpp=.o) OBJ_FILES_C = $(SOURCE_FILES_C:.c=.o) COVERAGE_FILES = $(OBJ_FILES:.o=.gc*) +MBEDTLS_LIB := ../../mbedtls/mbedtls/library/libmbedcrypto.a $(OBJ_FILES): %.o: %.cpp $(OBJ_FILES_C): %.c: %.c -$(TEST_PROGRAM): clean-coverage $(OBJ_FILES) $(OBJ_FILES_C) +$(MBEDTLS_LIB): $(MAKE) -C ../../mbedtls/mbedtls/ lib - g++ $(LDFLAGS) -o $(TEST_PROGRAM) $(OBJ_FILES) $(OBJ_FILES_C) ../../mbedtls/mbedtls/library/libmbedcrypto.a + +$(TEST_PROGRAM): $(OBJ_FILES) $(OBJ_FILES_C) $(MBEDTLS_LIB) | clean-coverage + g++ -o $@ $^ $(LDFLAGS) $(OUTPUT_DIR): mkdir -p $(OUTPUT_DIR)