From 733ff15719ec30e3fbb76aa5475ccfa2acc208af Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Thu, 10 May 2018 14:26:47 +0200 Subject: [PATCH] VFS: use O_APPEND flag of open() correctly Closes https://github.com/espressif/esp-idf/pull/1455 --- components/fatfs/src/vfs_fat.c | 25 +++++- components/spiffs/esp_spiffs.c | 3 +- components/vfs/test/test_vfs_append.c | 116 ++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 components/vfs/test/test_vfs_append.c diff --git a/components/fatfs/src/vfs_fat.c b/components/fatfs/src/vfs_fat.c index 556492e3ed..1e059e134e 100644 --- a/components/fatfs/src/vfs_fat.c +++ b/components/fatfs/src/vfs_fat.c @@ -32,6 +32,7 @@ typedef struct { FATFS fs; /* fatfs library FS structure */ char tmp_path_buf[FILENAME_MAX+3]; /* temporary buffer used to prepend drive name to the path */ char tmp_path_buf2[FILENAME_MAX+3]; /* as above; used in functions which take two path arguments */ + bool *o_append; /* O_APPEND is stored here for each max_files entries (because O_APPEND is not compatible with FA_OPEN_APPEND) */ FIL files[0]; /* array with max_files entries; must be the final member of the structure */ } vfs_fat_ctx_t; @@ -147,12 +148,18 @@ esp_err_t esp_vfs_fat_register(const char* base_path, const char* fat_drive, siz if (fat_ctx == NULL) { return ESP_ERR_NO_MEM; } + fat_ctx->o_append = malloc(max_files * sizeof(bool)); + if (fat_ctx->o_append == NULL) { + free(fat_ctx); + return ESP_ERR_NO_MEM; + } fat_ctx->max_files = max_files; strlcpy(fat_ctx->fat_drive, fat_drive, sizeof(fat_ctx->fat_drive) - 1); strlcpy(fat_ctx->base_path, base_path, sizeof(fat_ctx->base_path) - 1); esp_err_t err = esp_vfs_register(base_path, &vfs, fat_ctx); if (err != ESP_OK) { + free(fat_ctx->o_append); free(fat_ctx); return err; } @@ -180,6 +187,7 @@ esp_err_t esp_vfs_fat_unregister_path(const char* base_path) return err; } _lock_close(&fat_ctx->lock); + free(fat_ctx->o_append); free(fat_ctx); s_fat_ctxs[ctx] = NULL; return ESP_OK; @@ -306,6 +314,13 @@ static int vfs_fat_open(void* ctx, const char * path, int flags, int mode) fd = -1; goto out; } + // O_APPEND need to be stored because it is not compatible with FA_OPEN_APPEND: + // - FA_OPEN_APPEND means to jump to the end of file only after open() + // - O_APPEND means to jump to the end only before each write() + // Other VFS drivers handles O_APPEND well (to the best of my knowledge), + // therefore this flag is stored here (at this VFS level) in order to save + // memory. + fat_ctx->o_append[fd] = (flags & O_APPEND) == O_APPEND; out: _lock_release(&fat_ctx->lock); return fd; @@ -315,8 +330,16 @@ static ssize_t vfs_fat_write(void* ctx, int fd, const void * data, size_t size) { vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx; FIL* file = &fat_ctx->files[fd]; + FRESULT res; + if (fat_ctx->o_append[fd]) { + if ((res = f_lseek(file, f_size(file))) != FR_OK) { + ESP_LOGD(TAG, "%s: fresult=%d", __func__, res); + errno = fresult_to_errno(res); + return -1; + } + } unsigned written = 0; - FRESULT res = f_write(file, data, size, &written); + res = f_write(file, data, size, &written); if (res != FR_OK) { ESP_LOGD(TAG, "%s: fresult=%d", __func__, res); errno = fresult_to_errno(res); diff --git a/components/spiffs/esp_spiffs.c b/components/spiffs/esp_spiffs.c index c344e57764..d5fb180038 100644 --- a/components/spiffs/esp_spiffs.c +++ b/components/spiffs/esp_spiffs.c @@ -522,7 +522,8 @@ static int spiffs_mode_conv(int m) res |= SPIFFS_O_CREAT | SPIFFS_O_EXCL; } else if ((m & O_CREAT) && (m & O_TRUNC)) { res |= SPIFFS_O_CREAT | SPIFFS_O_TRUNC; - } else if (m & O_APPEND) { + } + if (m & O_APPEND) { res |= SPIFFS_O_CREAT | SPIFFS_O_APPEND; } return res; diff --git a/components/vfs/test/test_vfs_append.c b/components/vfs/test/test_vfs_append.c new file mode 100644 index 0000000000..d12d334e02 --- /dev/null +++ b/components/vfs/test/test_vfs_append.c @@ -0,0 +1,116 @@ +// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include "unity.h" +#include "esp_vfs.h" +#include "esp_vfs_fat.h" +#include "esp_spiffs.h" +#include "wear_levelling.h" + +#define TEST_PARTITION_LABEL "flash_test" + +#define OPEN_MODE 0 +#define MSG1 "Hello" +#define MSG2 " " +#define MSG3 "world!" + +static inline void test_write(int fd, const char *str, const char *msg) +{ + TEST_ASSERT_EQUAL_MESSAGE(strlen(str), write(fd, str, strlen(str)), msg); +} + +static inline void test_read(int fd, const char *str, const char *msg) +{ + char buf[strlen(str)]; + TEST_ASSERT_EQUAL_MESSAGE(strlen(str), read(fd, buf, strlen(str)), msg); + TEST_ASSERT_EQUAL_UINT8_ARRAY_MESSAGE(str, buf, strlen(str), msg); +} + +static inline void test_read_fails(int fd, const char *msg) +{ + char buf; + TEST_ASSERT_EQUAL_MESSAGE(0, read(fd, &buf, 1), msg); +} + +static void test_append(const char *path) +{ + int fd = open(path, O_RDWR | O_APPEND | O_CREAT | O_TRUNC, OPEN_MODE); + TEST_ASSERT_NOT_EQUAL(-1, fd); + + test_write(fd, MSG1, "write MSG1"); + test_read_fails(fd, "read fails MSG1"); + lseek(fd, 0, SEEK_SET); + test_read(fd, MSG1, "read MSG1"); + + lseek(fd, 0, SEEK_SET); + test_write(fd, MSG2, "write MSG2"); + test_read_fails(fd, "read fails MSG2"); //because write moved the pointer + lseek(fd, 0, SEEK_SET); + test_read(fd, MSG1 MSG2, "read MSG1 + MSG2"); + + TEST_ASSERT_NOT_EQUAL(-1, close(fd)); + fd = open(path, O_RDWR | O_APPEND, OPEN_MODE); + TEST_ASSERT_NOT_EQUAL(-1, fd); + + //after reopening the pointer should be at the beginning + test_read(fd, MSG1 MSG2, "read reopening"); + + lseek(fd, strlen(MSG1), SEEK_SET); + test_read(fd, MSG2, "read MSG2"); + lseek(fd, strlen(MSG1), SEEK_SET); + test_write(fd, MSG3, "write MSG3"); + test_read_fails(fd, "read fails MSG3"); //because write moved the pointer + lseek(fd, strlen(MSG1), SEEK_SET); + test_read(fd, MSG2 MSG3, "read MSG2 + MSG3"); + + lseek(fd, 0, SEEK_SET); + test_read(fd, MSG1 MSG2 MSG3, "read MSG1 + MSG2 + MSG3"); + + TEST_ASSERT_NOT_EQUAL(-1, close(fd)); + TEST_ASSERT_NOT_EQUAL(-1, unlink(path)); +} + +TEST_CASE("open() with O_APPEND on FATFS works well", "[vfs][FATFS]") +{ + wl_handle_t test_wl_handle; + + esp_vfs_fat_sdmmc_mount_config_t mount_config = { + .format_if_mount_failed = true, + .max_files = 2 + }; + TEST_ESP_OK(esp_vfs_fat_spiflash_mount("/spiflash", NULL, &mount_config, &test_wl_handle)); + + test_append("/spiflash/file.txt"); + + TEST_ESP_OK(esp_vfs_fat_spiflash_unmount("/spiflash", test_wl_handle)); +} + +TEST_CASE("open() with O_APPEND on SPIFFS works well", "[vfs][spiffs]") +{ + esp_vfs_spiffs_conf_t conf = { + .base_path = "/spiffs", + .partition_label = TEST_PARTITION_LABEL, + .max_files = 2, + .format_if_mount_failed = true + }; + TEST_ESP_OK(esp_vfs_spiffs_register(&conf)); + + test_append("/spiffs/file.txt"); + + TEST_ESP_OK(esp_vfs_spiffs_unregister(TEST_PARTITION_LABEL)); +}