joltwallet / esp_littlefs

LittleFS port for ESP-IDF
MIT License
254 stars 95 forks source link

littlefs does not support O_APPEND? #154

Closed alonbl closed 10 months ago

alonbl commented 10 months ago

Hi,

I am unsure where the exact issue is, I believe this is in the littlefs.

Provided the following code in a modified example of esp-idf:

    int fd;
    fd = open("/littlefs/fcntl.txt", O_CREAT | O_WRONLY | O_TRUNC, 0777);
    ESP_LOGI(TAG, "WRITE: %d", write(fd, "test1\n", 6));
    close(fd);
    fd = open("/littlefs/fcntl.txt", O_CREAT | O_WRONLY | O_APPEND, 0777);
    ESP_LOGI(TAG, "LSEEK-CUR: %ld", lseek(fd, 0, SEEK_CUR));
    ESP_LOGI(TAG, "WRITE: %d", write(fd, "test2\n", 6));
    ESP_LOGI(TAG, "LSEEK-CUR: %ld", lseek(fd, 0, SEEK_CUR));
    close(fd);

The output is:

I (352) esp_littlefs: WRITE: 6
I (352) esp_littlefs: LSEEK-CUR: 0
I (352) esp_littlefs: WRITE: 6
I (352) esp_littlefs: LSEEK-CUR: 6

As you can see the O_APPEND flag was provided and have no affect, content was overridden.

The expected output (tested on Linux):

I (352) esp_littlefs: WRITE: 6
I (352) esp_littlefs: LSEEK-CUR: 0
I (352) esp_littlefs: WRITE: 6
I (352) esp_littlefs: LSEEK-CUR: 12

The O_APPEND should cause seek to the end of file before each write.

Looking at the littlefs code the only reference of append is the following:

    if (m == O_APPEND) {ESP_LOGV(ESP_LITTLEFS_TAG, "O_APPEND"); lfs_flags |= LFS_O_APPEND;}

No use of LFS_O_APPEND anywhere.

Workaround (as long as we do not support concurrent access:

    fd = open("/littlefs/fcntl.txt", O_CREAT | O_WRONLY | O_TRUNC, 0777);
    lseek(fd, 0, SEEK_END);

Thanks,

BrianPugh commented 10 months ago

I wonder why I wrote if (m == O_APPEND) instead of if (m & O_APPEND). Investigating to see if there was a reason.

alonbl commented 10 months ago

You are right, the two lines are suspicious:

    if (m == O_APPEND) {ESP_LOGV(ESP_LITTLEFS_TAG, "O_APPEND"); lfs_flags |= LFS_O_APPEND;}
    if (m == O_RDONLY) {ESP_LOGV(ESP_LITTLEFS_TAG, "O_RDONLY"); lfs_flags |= LFS_O_RDONLY;}
BrianPugh commented 10 months ago

The naive solution is just to replace those two == with &, just trying to make sure that

  1. It's proper.
  2. What the original intent of explicitly checking for those flags.
BrianPugh commented 10 months ago

so the naive solution doesn't work, looking at other filesystem implementations, they do explicit checking as well. Diving a little deeper.

BrianPugh commented 10 months ago

oh derp, O_RDONLY is 0, so that makes sense why we're explicitly checking that.

BrianPugh commented 10 months ago

Fixed by #156 and in release v1.10.2. Thanks for opening the issue!

alonbl commented 10 months ago

Thank you!