Can FS/src/vfs_api.cpp VFSImpl::rmdir use actual rmdir instead of or in addition to unlink?

[lorol]

Hardware:

Board: ESP32 Dev Module
Core Installation/update date: b92c58d 2020-05-31
IDE name: Arduino IDE
Flash Frequency: 40Mhz
PSRAM enabled: no
Upload Speed: 115200
Computer OS: Windows 10

Description:

The sketch tests LittleFS, based on https://github.com/joltwallet/esp_littlefs
Proposed as: #4096 until the LittleFS gets implemented as official IDF-ESP component or otherwise.
Everything works except FS.rmdir(path) - which always fails
As found by @joltwallet, the implemented in esp_littlefs.c rmdir is not called, rather unlink is called from vfs_api.cpp
https://github.com/espressif/arduino-esp32/blob/master/libraries/FS/src/vfs_api.cpp#L203

Sketch:

https://github.com/lorol/LITTLEFS/blob/master/examples/LittleFS_test/LittleFS_test.ino

Debug Messages:

Serial Output:

...
  FILE: /mydir/hello2.txt	SIZE: 6
Deleting file: /mydir/hello2.txt
- file deleted
Removing Dir: /mydir
rmdir failed
Listing directory: /
  DIR : /mydir
...

[lbernstone]

Why does unlink fail? Can you directly unlink a directory outside of the FS methods? What is the return code from the rmdir?

[lorol]

@lbernstone
unlink fails because I copied the code from:
https://github.com/joltwallet/esp_littlefs/blob/master/src/esp_littlefs.c#L1148
which disallows unlink of folder there and is has own rmdir implementation further in code but it is not used from FS.
If I comment it, of course it works:
https://github.com/lorol/LITTLEFS/blob/master/src/esp_littlefs.c#L1176
Basically esp_littlefs.c separates the unlink() for files and rmdir() for folders and doesn't have remove()
The question is rather what is the best recommended fix?
For me, it works fine with allowing unilk for dir's at esp_littlefs.c
I'd like to make sure it is correct and also to report/discuss this with the author of esp_littlefs.c
Thanks for your time.

[lbernstone]

I assume @me-no-dev used unlink there due to an issue with rmdir in one of the other libraries.... Maybe that could be revisited at some point.
I'm not sure if it will cause ugly conflicts, but see if you are able to redefine the function somewhere in your code with:
#define vfs_littlefs_rmdir vfs_littlefs_unlink

[lorol]

No problem, I have a workaround.
Up to you and the team to revisit as long as it is on your list.
Should we keep this open?

[lbernstone]

Yes, leave it open until it is fixed. Did you try putting the define into your code so the source doesn't need to be changed?

[lorol]

Hi, with define only cannot be fixed. vfs_littlefs_unlink is originally made for files only.
But if I remove this checking (allow folders too, as i showed with commented lines) it works. A re-define is not needed.

[lorol]

@lbernstone
I will test few other things and update.

Off topic, what is the story of fatfs - do you have by chance any "mkfatfs" - like tool build for win that actually works and images from it mount to esp32 or esp8266? Couldn't find anything "ready for use" ...

[lbernstone]

https://github.com/marcmerlin/esp32_fatfsimage

[lorol]

Any guidance how to build on Win32 or ready binary?
I spent a lot of time.

[lbernstone]

Nope. I have tried to revisit it and the code is so ancient it doesn't compile properly any more. Maybe if you go back to IDF 2.x. It would be nice to make it a standalone, but my makefile skills are not good enough. Take this convo over to gitter if you want to chat.

[lorol]

Ok, back to the topic if esp-core devs want to review. Summary:

arduino-esp FS vfs_api.cpp - both methods use unlink()

bool VFSImpl::remove(const char* path) 
bool VFSImpl::rmdir(const char *path)

IDF esp_littlefs (a candidate for official LittleFS IDF component) has separate functions for removing files and folders:

static int vfs_littlefs_unlink(void* ctx, const char *path) 
static int vfs_littlefs_rmdir(void* ctx, const char* name)

arduino-esp wrapper LITTLEFS based on esp_littlefs.c, the rmdir fails because vfs_api.cpp calls vfs_littlefs_unlink
Workaround - a modified esp_littlefs.c to allow vfs_littlefs_unlink to delete both files and folders

[BrianPugh]

Just weighing in (author of esp-idf littlefs port), I believe it's improper than the current vfs implementation calls unlink on a directory.

See GNU's stance on this:

On some systems unlink cannot be used to delete the name of a directory, or at least can only be used this way by a privileged user. To avoid such problems, use rmdir to delete directories. (On GNU/Linux and GNU/Hurd systems unlink can never delete the name of a directory.)

So I could allow unlink to operate on directories, but I don't think its proper.

The lfs call I make, lfs_remove, works on both directories and files, so that itself is not a big deal. However, in order for it to properly delete a directory, the directory must be empty. So if i were to allow unlink to operate on directories, should I also add code that recursively deletes all files/directories within that directory? Or just return an error if the directory is not empty?

[lbernstone]

@me-no-dev Is there a particular reason you used unlink instead of rmdir in FS?

[lorol]

@BrianPugh thanks for explaining your valid points!
I believe it should not automatically delete all files/directories within that directory. IMO this is very aggressive.
LittleFS on ESP8266 have reproducing the SPIFFS behavior: The folders tree up to to / is silently deleted on deletion of last remaining file in this tree. But this obviously is because SPIFFS has no folders just flat file names can contain "/" and nothing else can be done.
This shouldn't be reproduced here on LittleFS i think. If someone needs it, it always can be done at application level.

[lbernstone]

Ok. I think this was added in to deal with SPIFFS causing issues with rmdir. I'll see if there is some way to fix this.