Merge branch 'bugfix/spiffs_obj_name_len_check' into 'master'

SPIFFS: fix issues with formatting and page size limit

See merge request idf/esp-idf!1866

components/spiffs/Kconfig

@@ -5,16 +5,14 @@ config SPIFFS_MAX_PARTITIONS
     default 3
     range 1 10
     help
-        Define maximum number of partitions 
+        Define maximum number of partitions that can be mounted.
-        that can be mounted.
 
 menu "SPIFFS Cache Configuration"
 config SPIFFS_CACHE
     bool "Enable SPIFFS Cache"
     default "y"
     help
-        Enables/disable memory read 
+        Enables/disable memory read caching of nucleus file system 
-        caching of nucleus file system 
         operations.
 
 config SPIFFS_CACHE_WR
@@ -22,16 +20,14 @@ config SPIFFS_CACHE_WR
     default "y"
     depends on SPIFFS_CACHE
     help
-        Enables memory write caching for 
+        Enables memory write caching for file descriptors in hydrogen.
-        file descriptors in hydrogen.
 
 config SPIFFS_CACHE_STATS
     bool "Enable SPIFFS Cache Statistics"
     default "n"
     depends on SPIFFS_CACHE
     help
-        Enable/disable statistics on caching. 
+        Enable/disable statistics on caching. Debug/test purpose only.
-        Debug/test purpose only.
 
 endmenu
 
@@ -39,44 +35,54 @@ config SPIFFS_PAGE_CHECK
     bool "Enable SPIFFS Page Check"
     default "y"
     help
-        Always check header of each 
+        Always check header of each accessed page to ensure consistent state.
-        accessed page to ensure consistent state.
+        If enabled it will increase number of reads from flash, especially
-        If enabled it will increase number 
+        if cache is disabled.
-        of reads, will increase flash.
 
 config SPIFFS_GC_MAX_RUNS
     int "Set Maximum GC Runs"
     default 10
     range 1 255
     help
-        Define maximum number of gc runs to 
+        Define maximum number of GC runs to perform to reach desired free pages.
-        perform to reach desired free pages.
 
 config SPIFFS_GC_STATS
     bool "Enable SPIFFS GC Statistics"
     default "n"
     help
-        Enable/disable statistics on gc. 
+        Enable/disable statistics on gc. Debug/test purpose only.
-        Debug/test purpose only.
+
+config SPIFFS_PAGE_SIZE
+	int "SPIFFS logical page size"
+	default 256
+	range 256 1024
+	help
+		Logical page size of SPIFFS partition, in bytes. Must be multiple
+		of flash page size (which is usually 256 bytes).
+		Larger page sizes reduce overhead when storing large files, and
+		improve filesystem performance when reading large files.
+		Smaller page sizes reduce overhead when storing small (< page size)
+		files.
 
 config SPIFFS_OBJ_NAME_LEN
     int "Set SPIFFS Maximum Name Length"
     default 32
     range 1 256
     help
-        Object name maximum length. Note that this length 
+        Object name maximum length. Note that this length include the 
-        include the zero-termination character, 
+        zero-termination character, meaning maximum string of characters
-        meaning maximum string of characters can at most be 
+        can at most be SPIFFS_OBJ_NAME_LEN - 1.
-        SPIFFS_OBJ_NAME_LEN - 1.
+        
+        SPIFFS_OBJ_NAME_LEN + SPIFFS_META_LENGTH should not exceed
+        SPIFFS_PAGE_SIZE - 64.
 
 config SPIFFS_USE_MAGIC
     bool "Enable SPIFFS Filesystem Magic"
     default "y"
     help
         Enable this to have an identifiable spiffs filesystem. 
-        This will look for a magic in all sectors 
+        This will look for a magic in all sectors to determine if this
-        to determine if this is a valid spiffs system 
+        is a valid spiffs system or not at mount time.
-        or not on mount point.
 
 config SPIFFS_USE_MAGIC_LENGTH
     bool "Enable SPIFFS Filesystem Length Magic"
@@ -96,6 +102,9 @@ config SPIFFS_META_LENGTH
         These bytes can be used in an application-specific manner.
         Set this to at least 4 bytes to enable support for saving file
         modification time.
+        
+        SPIFFS_OBJ_NAME_LEN + SPIFFS_META_LENGTH should not exceed
+        SPIFFS_PAGE_SIZE - 64.
 
 config SPIFFS_USE_MTIME
     bool "Save file modification time"
@@ -113,45 +122,39 @@ config SPIFFS_DBG
     bool "Enable general SPIFFS debug"
     default "n"
     help
-        Enabling this option will print 
+        Enabling this option will print general debug mesages to the console.
-        general debug mesages to the console
 
 config SPIFFS_API_DBG
     bool "Enable SPIFFS API debug"
     default "n"
     help
-        Enabling this option will print 
+        Enabling this option will print API debug mesages to the console.
-        API debug mesages to the console
 
 config SPIFFS_GC_DBG
     bool "Enable SPIFFS Garbage Cleaner debug"
     default "n"
     help
-        Enabling this option will print 
+        Enabling this option will print GC debug mesages to the console.
-        GC debug mesages to the console
 
 config SPIFFS_CACHE_DBG
     bool "Enable SPIFFS Cache debug"
     default "n"
     depends on SPIFFS_CACHE
     help
-        Enabling this option will print 
+        Enabling this option will print cache debug mesages to the console.
-        Cache debug mesages to the console
 
 config SPIFFS_CHECK_DBG
     bool "Enable SPIFFS Filesystem Check debug"
     default "n"
     help
-        Enabling this option will print 
+        Enabling this option will print Filesystem Check debug mesages
-        Filesystem Check debug mesages 
+        to the console.
-        to the console
 
 config SPIFFS_TEST_VISUALISATION
     bool "Enable SPIFFS Filesystem Visualization"
     default "n"
     help
-        Enable this option to enable SPIFFS_vis function 
+        Enable this option to enable SPIFFS_vis function in the API.
-        in the api.
 
 endmenu
 

components/spiffs/esp_spiffs.c

@@ -222,6 +222,14 @@ static esp_err_t esp_spiffs_init(const esp_vfs_spiffs_conf_t* conf)
         return ESP_ERR_INVALID_STATE;
     }
 
+    uint32_t flash_page_size = g_rom_flashchip.page_size;
+    uint32_t log_page_size = CONFIG_SPIFFS_PAGE_SIZE;
+    if (log_page_size % flash_page_size != 0) {
+        ESP_LOGE(TAG, "SPIFFS_PAGE_SIZE is not multiple of flash chip page size (%d)",
+                flash_page_size);
+        return ESP_ERR_INVALID_ARG;
+    }
+
     esp_partition_subtype_t subtype = conf->partition_label ?
             ESP_PARTITION_SUBTYPE_ANY : ESP_PARTITION_SUBTYPE_DATA_SPIFFS;
     const esp_partition_t* partition = esp_partition_find_first(ESP_PARTITION_TYPE_DATA, 
@@ -247,7 +255,7 @@ static esp_err_t esp_spiffs_init(const esp_vfs_spiffs_conf_t* conf)
     efs->cfg.hal_read_f        = spiffs_api_read;
     efs->cfg.hal_write_f       = spiffs_api_write;
     efs->cfg.log_block_size    = g_rom_flashchip.sector_size;
-    efs->cfg.log_page_size     = g_rom_flashchip.page_size;
+    efs->cfg.log_page_size     = log_page_size;
     efs->cfg.phys_addr         = 0;
     efs->cfg.phys_erase_block  = g_rom_flashchip.sector_size;
     efs->cfg.phys_size         = partition->size;
@@ -349,8 +357,12 @@ esp_err_t esp_spiffs_info(const char* partition_label, size_t *total_bytes, size
 
 esp_err_t esp_spiffs_format(const char* partition_label)
 {
-    bool mount_on_success = false;
+    bool partition_was_mounted = false;
     int index;
+    /* If the partition is not mounted, need to create SPIFFS structures
+     * and mount the partition, unmount, format, delete SPIFFS structures.
+     * See SPIFFS wiki for the reason why.
+     */
     esp_err_t err = esp_spiffs_by_label(partition_label, &index);
     if (err != ESP_OK) {
         esp_vfs_spiffs_conf_t conf = {
@@ -363,23 +375,28 @@ esp_err_t esp_spiffs_format(const char* partition_label)
             return err;
         }
         err = esp_spiffs_by_label(partition_label, &index);
-        if (err != ESP_OK) {
+        assert(err == ESP_OK && "failed to get index of the partition just mounted");
-            return err;
-        }
-        esp_spiffs_free(&_efs[index]);
-        return ESP_OK;
     } else if (SPIFFS_mounted(_efs[index]->fs)) {
-        SPIFFS_unmount(_efs[index]->fs);
+        partition_was_mounted = true;
-        mount_on_success = true;
     }
+
+    SPIFFS_unmount(_efs[index]->fs);
+
     s32_t res = SPIFFS_format(_efs[index]->fs);
     if (res != SPIFFS_OK) {
         ESP_LOGE(TAG, "format failed, %i", SPIFFS_errno(_efs[index]->fs));
         SPIFFS_clearerr(_efs[index]->fs);
+        /* If the partition was previously mounted, but format failed, don't
+         * try to mount the partition back (it will probably fail). On the
+         * other hand, if it was not mounted, need to clean up.
+         */
+        if (!partition_was_mounted) {
+            esp_spiffs_free(&_efs[index]);
+        }
         return ESP_FAIL;
     }
 
-    if (mount_on_success) {
+    if (partition_was_mounted) {
         res = SPIFFS_mount(_efs[index]->fs, &_efs[index]->cfg, _efs[index]->work,
                             _efs[index]->fds, _efs[index]->fds_sz, _efs[index]->cache,
                             _efs[index]->cache_sz, spiffs_api_check);
@@ -388,6 +405,8 @@ esp_err_t esp_spiffs_format(const char* partition_label)
             SPIFFS_clearerr(_efs[index]->fs);
             return ESP_FAIL;
         }
+    } else {
+        esp_spiffs_free(&_efs[index]);
     }
     return ESP_OK;
 }

components/spiffs/include/spiffs_config.h

@@ -153,12 +153,15 @@ extern void spiffs_api_unlock(struct spiffs_t *fs);
 // changes the on-disk format, so the change is not backward-compatible.
 //
 // Do note: the meta length must never exceed
-// logical_page_size - (SPIFFS_OBJ_NAME_LEN + 64)
+// logical_page_size - (SPIFFS_OBJ_NAME_LEN + SPIFFS_PAGE_EXTRA_SIZE)
 //
 // This is derived from following:
 // logical_page_size - (SPIFFS_OBJ_NAME_LEN + sizeof(spiffs_page_header) +
 // spiffs_object_ix_header fields + at least some LUT entries)
 #define SPIFFS_OBJ_META_LEN             (CONFIG_SPIFFS_META_LENGTH)
+#define SPIFFS_PAGE_EXTRA_SIZE          (64)
+_Static_assert(SPIFFS_OBJ_META_LEN + SPIFFS_OBJ_NAME_LEN + SPIFFS_PAGE_EXTRA_SIZE
+        <= CONFIG_SPIFFS_PAGE_SIZE, "SPIFFS_OBJ_META_LEN or SPIFFS_OBJ_NAME_LEN too long");
 
 // Size of buffer allocated on stack used when copying data.
 // Lower value generates more read/writes. No meaning having it bigger

components/spiffs/test/test_spiffs.c

@@ -407,7 +407,7 @@ static void test_teardown()
     TEST_ESP_OK(esp_vfs_spiffs_unregister(spiffs_test_partition_label));
 }
 
-TEST_CASE("can format partition", "[spiffs]")
+TEST_CASE("can initialize SPIFFS in erased partition", "[spiffs]")
 {
     const esp_partition_t* part = get_test_data_partition();
     TEST_ASSERT_NOT_NULL(part);
@@ -420,6 +420,44 @@ TEST_CASE("can format partition", "[spiffs]")
     test_teardown();
 }
 
+TEST_CASE("can format mounted partition", "[spiffs]")
+{
+    // Mount SPIFFS, create file, format, check that the file does not exist.
+    const esp_partition_t* part = get_test_data_partition();
+    TEST_ASSERT_NOT_NULL(part);
+    test_setup();
+    const char* filename = "/spiffs/hello.txt";
+    test_spiffs_create_file_with_text(filename, spiffs_test_hello_str);
+    esp_spiffs_format(part->label);
+    FILE* f = fopen(filename, "r");
+    TEST_ASSERT_NULL(f);
+    test_teardown();
+}
+
+TEST_CASE("can format unmounted partition", "[spiffs]")
+{
+    // Mount SPIFFS, create file, unmount. Format. Mount again, check that
+    // the file does not exist.
+    const esp_partition_t* part = get_test_data_partition();
+    TEST_ASSERT_NOT_NULL(part);
+    test_setup();
+    const char* filename = "/spiffs/hello.txt";
+    test_spiffs_create_file_with_text(filename, spiffs_test_hello_str);
+    test_teardown();
+    esp_spiffs_format(part->label);
+    // Don't use test_setup here, need to mount without formatting
+    esp_vfs_spiffs_conf_t conf = {
+        .base_path = "/spiffs",
+        .partition_label = spiffs_test_partition_label,
+        .max_files = 5,
+        .format_if_mount_failed = false
+    };
+    TEST_ESP_OK(esp_vfs_spiffs_register(&conf));
+    FILE* f = fopen(filename, "r");
+    TEST_ASSERT_NULL(f);
+    test_teardown();
+}
+
 TEST_CASE("can create and write file", "[spiffs]")
 {
     test_setup();