Fix return value of lfs_rename()

littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers

BSD 3-Clause "New" or "Revised" License

4.9k stars 771 forks source link

Fix return value of lfs_rename() #917

Closed tomscii closed 5 months ago

tomscii commented 6 months ago

When lfs_rename() is called trying to rename (move) a file to an existing directory, LFS_ERR_ISDIR is (correctly) returned. However, in the opposite case, if one tries to rename (move) a directory to a path currently occupied by a regular file, LFS_ERR_NOTDIR should be returned (since the error is that the destination is NOT a directory), but in reality, LFS_ERR_ISDIR is returned in this case as well.

This commit fixes the code so that in the latter case, LFS_ERR_NOTDIR is returned.

geky-bot commented 6 months ago

Tests passed ✓, Code: 16836 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16836 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2357/2535 lines (-0.1%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1202/1530 branches (-0.1%) | | Threadsafe | 17704 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16900 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18516 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17492 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

geky commented 5 months ago

Hi @tomscii, thanks for this. This does look more correct in terms of following POSIX.

The only concern is if this will break any existing code. Code may be relying on LFS_ERR_NOTDIR for both cases.

I'll plan on bringing this in on the next minor release. If it's an issue, we'll learn, otherwise I think the strategy may be to allow error codes to change to match POSIX on minor releases.

geky commented 5 months ago

I've gone ahead and updating this PR for two minor reasons:

Updated to match style, mainly double-indention (8-spaces) in expressions. I realize this isn't really documented anywhere.
Added tests to ensure that the rename type mismatch returns this error as expected.

Let me know if anything looks concerning.

geky-bot commented 5 months ago

Tests passed ✓, Code: 16836 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16836 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2359/2535 lines (+0.0%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1205/1530 branches (+0.1%) | | Threadsafe | 17704 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16900 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18516 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17492 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

tomscii commented 5 months ago

@geky Looks great, thank you!

geky commented 5 months ago

This is on master now, thanks for the PR!

tomscii commented 5 months ago

This is on master now, thanks for the PR!

Excellent! Thanks for all your hard work maintaining LittleFS!