VFS: use O_APPEND flag of open() correctly

Closes https://github.com/espressif/esp-idf/pull/1455

diff --git a/components/fatfs/src/vfs_fat.c b/components/fatfs/src/vfs_fat.c
index 556492e3ed928298e065bc4cdbf1aa337e860003..1e059e134e7fb4f767c0988b7dcb49259c19e4d4 100644 (file)
--- 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 c344e577648022807dd09ebddd805890741d5a99..d5fb18003813bfc564cc6d411d14dada984440ae 100644 (file)
--- 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 (file)
index 0000000..d12d334
--- /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 <unistd.h>
+#include <fcntl.h>
+#include <string.h>
+#include <sys/stat.h>
+#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));
+}