From a45b12a68b100422eee88e3859c16120cb8e481a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Rohl=C3=ADnek?= Date: Fri, 24 Jan 2025 10:23:00 +0100 Subject: [PATCH 1/2] feat(storage/vfs): improve mountpoint table memory usage --- .../vfs/private_include/esp_vfs_private.h | 17 +++++---- components/vfs/vfs.c | 37 +++++++++++++------ 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/components/vfs/private_include/esp_vfs_private.h b/components/vfs/private_include/esp_vfs_private.h index 5dcca69b55..f2f293c400 100644 --- a/components/vfs/private_include/esp_vfs_private.h +++ b/components/vfs/private_include/esp_vfs_private.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -7,6 +7,7 @@ #include "sdkconfig.h" #include "esp_vfs.h" #include "esp_vfs_common.h" +#include #ifdef __cplusplus extern "C" { @@ -19,12 +20,12 @@ extern "C" { #endif typedef struct vfs_entry_ { - int flags; /*!< ESP_VFS_FLAG_CONTEXT_PTR and/or ESP_VFS_FLAG_READONLY_FS or ESP_VFS_FLAG_DEFAULT */ - const esp_vfs_fs_ops_t *vfs; // contains pointers to VFS functions - char path_prefix[ESP_VFS_PATH_MAX]; // path prefix mapped to this VFS - size_t path_prefix_len; // micro-optimization to avoid doing extra strlen - void* ctx; // optional pointer which can be passed to VFS - int offset; // index of this structure in s_vfs array + int flags; /*!< ESP_VFS_FLAG_CONTEXT_PTR and/or ESP_VFS_FLAG_READONLY_FS or ESP_VFS_FLAG_DEFAULT */ + const esp_vfs_fs_ops_t *vfs; // contains pointers to VFS functions + void *ctx; // optional pointer which can be passed to VFS + int offset; // index of this structure in s_vfs array + size_t path_prefix_len; // micro-optimization to avoid doing extra strlen + const char path_prefix[] __attribute__ ((counted_by (path_prefix_len))); // path prefix mapped to this VFS } vfs_entry_t; /** @@ -53,7 +54,7 @@ typedef struct vfs_entry_ { * ESP_ERR_NO_MEM if too many VFSes are registered. * ESP_ERR_INVALID_ARG if given an invalid parameter. */ -esp_err_t esp_vfs_register_common(const char *base_path, size_t len, const esp_vfs_t* vfs, void* ctx, int *vfs_index); +esp_err_t esp_vfs_register_common(const char *base_path, size_t len, const esp_vfs_t *vfs, void *ctx, int *vfs_index); /** * Get vfs fd with given path. diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index 49581c7136..b756026ce4 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -395,20 +395,30 @@ fail: return ESP_ERR_NO_MEM; } -static esp_err_t esp_vfs_register_fs_common(const char* base_path, size_t len, const esp_vfs_fs_ops_t* vfs, int flags, void* ctx, int *vfs_index) +static esp_err_t esp_vfs_register_fs_common( + const char *base_path, + size_t base_path_len, + const esp_vfs_fs_ops_t *vfs, + int flags, + void *ctx, + int *vfs_index) { + if (s_vfs_count >= VFS_MAX_COUNT) { + return ESP_ERR_NO_MEM; + } + if (vfs == NULL) { ESP_LOGE(TAG, "VFS is NULL"); return ESP_ERR_INVALID_ARG; } - if (len != LEN_PATH_PREFIX_IGNORED) { + if (base_path_len != LEN_PATH_PREFIX_IGNORED) { /* empty prefix is allowed, "/" is not allowed */ - if ((len == 1) || (len > ESP_VFS_PATH_MAX)) { + if ((base_path_len == 1) || (base_path_len > ESP_VFS_PATH_MAX)) { return ESP_ERR_INVALID_ARG; } /* prefix has to start with "/" and not end with "/" */ - if (len >= 2 && ((base_path[0] != '/') || (base_path[len - 1] == '/'))) { + if (base_path_len >= 2 && ((base_path[0] != '/') || (base_path[base_path_len - 1] == '/'))) { return ESP_ERR_INVALID_ARG; } } @@ -426,23 +436,26 @@ static esp_err_t esp_vfs_register_fs_common(const char* base_path, size_t len, c s_vfs_count++; } - vfs_entry_t *entry = (vfs_entry_t*) heap_caps_malloc(sizeof(vfs_entry_t), VFS_MALLOC_FLAGS); + size_t alloc_size = sizeof(vfs_entry_t) + + (base_path_len == LEN_PATH_PREFIX_IGNORED ? 0 : base_path_len + 1); + + vfs_entry_t *entry = (vfs_entry_t*) heap_caps_malloc(alloc_size, VFS_MALLOC_FLAGS); if (entry == NULL) { return ESP_ERR_NO_MEM; } s_vfs[index] = entry; - if (len != LEN_PATH_PREFIX_IGNORED) { - strcpy(entry->path_prefix, base_path); // we have already verified argument length - } else { - bzero(entry->path_prefix, sizeof(entry->path_prefix)); - } - entry->path_prefix_len = len; + + entry->path_prefix_len = base_path_len; entry->vfs = vfs; entry->ctx = ctx; entry->offset = index; entry->flags = flags; + if (base_path_len != LEN_PATH_PREFIX_IGNORED) { + memcpy((char *)(entry->path_prefix), base_path, base_path_len + 1); + } + if (vfs_index) { *vfs_index = index; } From 67638981ecda4f7b5e5fc73456e50000fc0d638b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Rohl=C3=ADnek?= Date: Thu, 30 Jan 2025 09:52:37 +0100 Subject: [PATCH 2/2] feat(storage/vfs): cleanup path prefix handling --- .../vfs/private_include/esp_vfs_private.h | 10 +-- components/vfs/vfs.c | 64 +++++++++++++------ 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/components/vfs/private_include/esp_vfs_private.h b/components/vfs/private_include/esp_vfs_private.h index f2f293c400..146e17de44 100644 --- a/components/vfs/private_include/esp_vfs_private.h +++ b/components/vfs/private_include/esp_vfs_private.h @@ -21,11 +21,11 @@ extern "C" { typedef struct vfs_entry_ { int flags; /*!< ESP_VFS_FLAG_CONTEXT_PTR and/or ESP_VFS_FLAG_READONLY_FS or ESP_VFS_FLAG_DEFAULT */ - const esp_vfs_fs_ops_t *vfs; // contains pointers to VFS functions - void *ctx; // optional pointer which can be passed to VFS - int offset; // index of this structure in s_vfs array - size_t path_prefix_len; // micro-optimization to avoid doing extra strlen - const char path_prefix[] __attribute__ ((counted_by (path_prefix_len))); // path prefix mapped to this VFS + const esp_vfs_fs_ops_t *vfs; /*!< contains pointers to VFS functions */ + void *ctx; /*!< optional pointer which can be passed to VFS */ + int offset; /*!< index of this structure in s_vfs array */ + size_t path_prefix_len; /*!< micro-optimization to avoid doing extra strlen, contains length of the string, not the size of path_prefix array */ + const char path_prefix[]; /*!< path prefix mapped to this VFS */ } vfs_entry_t; /** diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index b756026ce4..bcd5efa3d5 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -395,9 +395,15 @@ fail: return ESP_ERR_NO_MEM; } +static bool is_path_prefix_valid(const char *path, size_t length) { + return (length >= 2) + && (length <= ESP_VFS_PATH_MAX) + && (path[0] == '/') + && (path[length - 1] != '/'); +} + static esp_err_t esp_vfs_register_fs_common( const char *base_path, - size_t base_path_len, const esp_vfs_fs_ops_t *vfs, int flags, void *ctx, @@ -412,13 +418,15 @@ static esp_err_t esp_vfs_register_fs_common( return ESP_ERR_INVALID_ARG; } - if (base_path_len != LEN_PATH_PREFIX_IGNORED) { - /* empty prefix is allowed, "/" is not allowed */ - if ((base_path_len == 1) || (base_path_len > ESP_VFS_PATH_MAX)) { - return ESP_ERR_INVALID_ARG; - } - /* prefix has to start with "/" and not end with "/" */ - if (base_path_len >= 2 && ((base_path[0] != '/') || (base_path[base_path_len - 1] == '/'))) { + size_t base_path_len = 0; + const char *_base_path = ""; + + if (base_path != NULL) { + base_path_len = strlen(base_path); + _base_path = base_path; + + if (base_path_len != 0 && !is_path_prefix_valid(base_path, base_path_len)) { + ESP_LOGE(TAG, "Invalid path prefix"); return ESP_ERR_INVALID_ARG; } } @@ -436,25 +444,20 @@ static esp_err_t esp_vfs_register_fs_common( s_vfs_count++; } - size_t alloc_size = sizeof(vfs_entry_t) - + (base_path_len == LEN_PATH_PREFIX_IGNORED ? 0 : base_path_len + 1); - - vfs_entry_t *entry = (vfs_entry_t*) heap_caps_malloc(alloc_size, VFS_MALLOC_FLAGS); + vfs_entry_t *entry = heap_caps_malloc(sizeof(vfs_entry_t) + base_path_len + 1, VFS_MALLOC_FLAGS); if (entry == NULL) { return ESP_ERR_NO_MEM; } s_vfs[index] = entry; - entry->path_prefix_len = base_path_len; + entry->path_prefix_len = base_path == NULL ? LEN_PATH_PREFIX_IGNORED : base_path_len; entry->vfs = vfs; entry->ctx = ctx; entry->offset = index; entry->flags = flags; - if (base_path_len != LEN_PATH_PREFIX_IGNORED) { - memcpy((char *)(entry->path_prefix), base_path, base_path_len + 1); - } + memcpy((char *)(entry->path_prefix), _base_path, base_path_len + 1); if (vfs_index) { *vfs_index = index; @@ -470,8 +473,13 @@ esp_err_t esp_vfs_register_fs(const char* base_path, const esp_vfs_fs_ops_t* vfs return ESP_ERR_INVALID_ARG; } + if (base_path == NULL) { + ESP_LOGE(TAG, "base_path cannot be null"); + return ESP_ERR_INVALID_ARG; + } + if ((flags & ESP_VFS_FLAG_STATIC)) { - return esp_vfs_register_fs_common(base_path, strlen(base_path), vfs, flags, ctx, NULL); + return esp_vfs_register_fs_common(base_path, vfs, flags, ctx, NULL); } esp_vfs_fs_ops_t *_vfs = esp_vfs_duplicate_fs_ops(vfs); @@ -479,7 +487,7 @@ esp_err_t esp_vfs_register_fs(const char* base_path, const esp_vfs_fs_ops_t* vfs return ESP_ERR_NO_MEM; } - esp_err_t ret = esp_vfs_register_fs_common(base_path, strlen(base_path), _vfs, flags, ctx, NULL); + esp_err_t ret = esp_vfs_register_fs_common(base_path, _vfs, flags, ctx, NULL); if (ret != ESP_OK) { esp_vfs_free_fs_ops(_vfs); return ret; @@ -500,13 +508,29 @@ esp_err_t esp_vfs_register_common(const char* base_path, size_t len, const esp_v return ESP_ERR_INVALID_ARG; } + if (len != LEN_PATH_PREFIX_IGNORED) { + if (base_path == NULL) { + ESP_LOGE(TAG, "Path prefix cannot be NULL"); + return ESP_ERR_INVALID_ARG; + } + + // This is for backwards compatibility + if (strlen(base_path) != len) { + ESP_LOGE(TAG, "Mismatch between real and given path prefix lengths."); + return ESP_ERR_INVALID_ARG; + } + } else { + // New API expects NULL in this case + base_path = NULL; + } + esp_vfs_fs_ops_t *_vfs = NULL; esp_err_t ret = esp_vfs_make_fs_ops(vfs, &_vfs); if (ret != ESP_OK) { return ret; } - ret = esp_vfs_register_fs_common(base_path, len, _vfs, vfs->flags, ctx, vfs_index); + ret = esp_vfs_register_fs_common(base_path, _vfs, vfs->flags, ctx, vfs_index); if (ret != ESP_OK) { esp_vfs_free_fs_ops(_vfs); return ret; @@ -564,7 +588,7 @@ esp_err_t esp_vfs_register_fs_with_id(const esp_vfs_fs_ops_t *vfs, int flags, vo } *vfs_id = -1; - return esp_vfs_register_fs_common("", LEN_PATH_PREFIX_IGNORED, vfs, flags, ctx, vfs_id); + return esp_vfs_register_fs_common(NULL, vfs, flags, ctx, vfs_id); } esp_err_t esp_vfs_register_with_id(const esp_vfs_t *vfs, void *ctx, esp_vfs_id_t *vfs_id)