fs/fat: fix incorrect representation of SFN files created on Linux#18697
fs/fat: fix incorrect representation of SFN files created on Linux#18697michallenc wants to merge 1 commit intoapache:masterfrom
Conversation
Linux creates all files as long file name entries even if they fit short 8.3 format. This caused issues when deleting or renaming files as the long file name entry was not recognized and only deleted it's short file name entry. This basically kept breaking the file system as subsequent long file name files were not correctly stored. The typical representation of this issue was long file name being represented as it's short name alias. This commit adjusts the LFN/SFN logic a bit - the code now always fills in long file name and then checks if this could possibly be a short file entry (we still have to do that because Windows stores 8.3 files as short file entry). This ensures compatibility with files created both on Linux and Windows. Signed-off-by: Michal Lenc <michallenc@seznam.cz>
michallenc
requested review from
acassis,
anchao,
cederom,
fdcavalcanti,
gustavonihei,
jerpelea,
linguini1,
lupyuen,
pkarashchenko,
tmedicci and
xiaoxiang781216
|
It would be lovely if you could test this in your environment as well to ensure I didn't break anything. |
|
@michallenc nice fix! Strange nobody faced it before. Do you know if the same issue happens if using Windows/MacOS ? |
It doesn't happen when using Windows because they store file with short name as a short entry so NuttX correctly removes it. Not sure about MacOS, don't have one to try. |
github-actions
bot
added
Area: File System
|
I can check on FreeBSD.. what are test steps? I assume this is about FAT on the NuttX side? :-) |
Yes, it's about FAT on the NuttX side. Testing is pretty simple
I have this simple C code that does the rename on NuttX for you. #include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/file.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <syslog.h>
#include <unistd.h>
int main(int argc, char *argv[]) {
openlog("sdcard", LOG_PERROR, LOG_USER);
int fd = open("/sdcard/config.ini", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
if (fd < 0) {
syslog(LOG_ERR, "Failed to create SD card %d\n", errno);
return -1;
}
close(fd);
char newpath[64];
uint32_t rnd = argc > 1 ? atol(argv[1]) : 0x12345678;
sprintf(newpath, "%s_%08" PRIx32 ".ini", "/sdcard/config", rnd);
rename("sdcard/config.ini", newpath);
return 0;
}This happens ONLY IF your computer creates the initial But I am more concerned whether I break anything on NuttX FAT side - hopefully not, I tested file creation, rename, delete and it all seems to work without issues. |
|
Okay, my box does not create anything on its own for sure, I even need to mount things by hand :D What board:config can you recommend? I guess we need usbmsc? Or something with SD card reader? :-P |
|
There was a problem hiding this comment.
Thank you @michallenc good catch! :-)
If there is anyone using FAT insensively more testing is welcome :-)
I did call for testing on the mailing list :-)
| { | ||
| /* Get short file name for given path */ | ||
|
|
||
| char *name = kmm_zalloc(DIR_MAXFNAME); |
There was a problem hiding this comment.
could we avoid malloc here
There was a problem hiding this comment.
No because of FAR const char *node = *path; line in fat_parsesfname. And we need to keep the possibility to edit path pointer because calling fat_parsesfname when CONFIG_FAT_LFN=n needs to move it after / terminator.
It could be done, but would require larger rewrite of these functions.
There was a problem hiding this comment.
DIR_MAXFNAME is 11 why not define local array directly
I formatted the card many times and tried both FAT16 (that's the default for my SD card since it only has about 400 MBs) and FAT32, there is no difference in how the file are created - it should just affect the size of the allocation table. I think this is more of a implementation simplification on Linux side and our wrong expectation that it has to be short file entry if the file name fits into 8+3 scheme. Regarding your dump, what it look like if you find the file entry? You should be able to grep 'CONFIG` or whatever name you used - just try it with freshly formatted SD card, there might be left old deleted entries otherwise. |
4 participants
Summary
Linux creates all files as long file name entries even if they fit short 8.3 format. This caused issues when deleting or renaming files as the long file name entry was not recognized and only deleted it's short file name entry. This basically kept breaking the file system as subsequent long file name files were not correctly stored. The typical representation of this issue was long file name being represented as it's short name alias.
This commit adjusts the LFN/SFN logic a bit - the code now always fills in long file name and then checks if this could possibly be a short file entry (we still have to do that because Windows stores 8.3 files as short file entry). This ensures compatibility with files created both on Linux and Windows.
The following log is a FAT entry on Linux. You can see short file name
config.iniis still written as a long file entry.The issue on NuttX is easily reproducible by renaming
config.inifile to long nameconfig_12345678.ini. The first rename is ok (though the erase of the old entry already not correct), but subsequent rename back toconfig.inion Linux and another rename to long name breaks the system -lscommand now doesn't showconfig_12345678.inias it should, but it's short file aliasCONFIG~1.INI- and it's not justls, callingopenonconfig_12345678.inidoesn't work either.Impact
fs/fat: Fix long/short file (re)name issues caused by incorrect representation of SFN files created on Linux.
Testing
Here are the logs after the fix - both Linux and Windows created files are correctly renamed now.
Linux
Windows
I kept the original design that NuttX creates a file with short name as a short file entry - it seems like a better strategy for us since we are embedded and one would expect we wouldn't waste memory unless really have to.