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.

[lorol]

@lbernstone, did you check if your lbernstone@e151d5b resolves what is discussed here?
I didn't check into code ... but if "mountpoint" is the "base_path" it could be anything so "/spiffs" as criteria is unreliable.
Also, if if "mountpoint" is from partition stuff will not work neither.
Because the proposed LITTLEFS "looks like" SPIFFS for the system - it re-uses same partition scheme and flashing things as SPIFFS, (label, type 0x82, tools etc.) - for ease, similar to esp8266. otherwise it will need a lot more additions and mods of Arduino libs and tools, not necessarily needed.

[lbernstone]

If you use "/spiffs" for the default mountpoint on littlefs, your code will not be accepted. This is just a label applied when you call begin, so it is not going to catch spiffs if someone decides to be "smart". Those people will get a nice stack fault for their efforts. I have made additional changes, look at the actual PR to see what will be committed.

[lorol]

Ok, if I understand properly, on your last idea, if mounting point is named "/spiffs" for any reason, then you just kick and say "no rmdir" :) for you. And if real SPIFFS has other mounting point name ... then rmdir will fail. The assumption is that SPIFFS has only files, so rmdir will not be attempted - did I get it or I am off? Anyway, thank you for your efforts.

[lbernstone]

Yes, rmdir is meaningless (it should be a noop) in spiffs. Files can have "/" in the name, but when you remove the file with the slash, the "directory" goes with it.

[BrianPugh]

@lorol doesn't your littlefs mount to /spiffs, so wouldn't this still cause issues.

[lorol]

I am a bit confused what _mounpoint of the vfs_api.cpp means, Please help to confirm.
I have some fuzzy understanding of: partition table first column names in .csv vs.. basePath vs. _mountpoint

If _mounpoint means (takes value from) basePath below, the then no, the default is /littlefs ... but this is user changeable, which leaves limbo scenarios. Most sketches are lazy of course and use use begin() w/ no parameters.
https://github.com/lorol/LITTLEFS/blob/master/src/LITTLEFS.h#L26

Anyway, with my limited knowledge, I believe the fix at vfs_api.cpp can be done differently than proposed solution by @lbernstone

[lbernstone]

@lorol, the fix is changing unlink to rmdir. That is done. The spiffs change was made to keep it from causing additional issues. Read the topic title.
@BrianPugh, _mountpoint is the location in vfs. @lorol is using /littlefs.

[lorol]

@lbernstone . For me is OK, Hope to be for others as well.
BTW, does the SPIFFS ever return true to isDirectory()? If no, then additional check for mounting point name may not be necessary at all. (IMO)
Thank you for your time and clarifications.

[lorol]

Hi @BrianPugh and @lbernstone
Can you please check here: https://github.com/espressif/arduino-esp32/blob/master/libraries/FS/src/vfs_api.cpp#L57
What a SPIFFS and LittleFS existing file should return accordingly when is tried to open?

I think, LittleFS gets like example: [E][vfs_api.cpp:65] open(): /littlefs/config/wifiSettings.json does not exist
And same code,on SPIFFS does not throw any error.
I believe this is again related to directories difference between both FS and somehow SPIFFS get it working ...

[BrianPugh]

I didn't really look too hard, but does the folder /littlefs/config exist? because with spiffs, the file name is literally interpreted as "config/wifiSettings.json", meanwhile with littlefs it has to transverse the directories in the path.

[lbernstone]

There are no directories in SPIFFS. So, if you open "/test/file" that is the name of the file, and it does not matter if "/test/" exists. Full filesystems should return an error if the path does not exist.
jinx

[lorol]

I didn't really look too hard, but does the folder /littlefs/config exist? because with spiffs, the file name is literally interpreted as "config/wifiSettings.json", meanwhile with littlefs it has to transverse the directories in the path.

You are right to suspect. /littlefs/config was not exiting on my first test. Again because the code I used was made for SPIFFS and these files (paths) can be created later when needed. I had to pre-make the folder. But even after folder exists up to /littlefs/config still throwing the errors :) because these files don't actually exist

[lorol]

There are no directories in SPIFFS. So, if you open "/test/file" that is the name of the file, and it does not matter if "/test/" exists. Full filesystems should return an error if the path does not exist.
jinx

Yes exactly what is happening. Unfortunately a code made for SPIFFS never returns error there. I already got questions from people testing littlefs on esp32 ... why now this error shows ... on my working code for SPIFFS. Guess no right answer on that :)

HM ... so what opendir() is returning on SPIFFS? always true?