Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't assert on attempting to write a file opened RO #831

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shacron
Copy link

@shacron shacron commented May 16, 2023

When lfs_file_write() is called on a file opened as read-only, it soon encounters an assertion in lfs_file_rawwrite() that is unhappy with this state. Instead of aborting, return an error and allow the client to handle it properly. This matches the behavior of write(2), which sets errno to EBADF in this case.

When lfs_file_write() is called on a file opened as read-only, it soon encounters an assertion in lfs_file_rawwrite() that is unhappy with this state. Instead of aborting, return an error and allow the client to handle it properly. This matches the behavior of write(2), which sets errno to EBADF in this case.
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 16568 B (+0.1%), Stack: 1432 B (+0.0%), Structs: 772 B (+0.0%)
Code Stack Structs Coverage
Default 16568 B (+0.1%) 1432 B (+0.0%) 772 B (+0.0%) Lines 2291/2471 lines (-0.0%)
Readonly 5990 B (+0.0%) 448 B (+0.0%) 772 B (+0.0%) Branches 1175/1498 branches (-0.1%)
Threadsafe 17374 B (+0.1%) 1432 B (+0.0%) 780 B (+0.0%) Benchmarks
Migrate 18252 B (+0.1%) 1736 B (+0.0%) 776 B (+0.0%) Readed 29369693876 B (+0.0%)
Error-asserts 17200 B (+0.0%) 1424 B (+0.0%) 772 B (+0.0%) Proged 1482874766 B (+0.0%)
Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented May 17, 2023

Hi @shacron, thanks for creating a PR.

Unfortunately this out of scope for littlefs right now.

The two main common use cases for littlefs as the moment are in OS/RTOSes/frameworks which provide their own high-level filesystem APIs and provide these sort of runtime errors at that level, and low-level microcontroller applications where the filesystem operations are tightly integrated into the application logic.

In these two cases, opening a file readonly and then writing to it is a programming error. It shouldn't happen or should be runtime checked by a higher layer. Making this a runtime error in the littlefs core is redundant and adds code cost. It's also usually easier to debug an assert failure than runtime error in these cases since upper layers can muddy the water as they propagate/mutate error codes.


Though given related discussion, maybe at some point it would be worth adding something like an LFS_UNTRUSTEDUSER configuration that gives the option for these sort of things to be runtime errors.

@shacron
Copy link
Author

shacron commented May 17, 2023

The current behavior is such that if assertions are enabled, the assertion fails and catches the error. If assertions are not enabled, lfs_file_rawwrite() will silently fill the file with zeros and return success. To prevent this, an application or framework provider must store the RW state of the open file. I.e., the API is not safe to use (risking immediate termination or data corruption) without an additional state variable that exists entirely outside the API, and the client must know that they need to maintain it in every usage.

I understand the concerns about code size. In my test the change bloated the code by 4 instructions on arm64 out of 59125 total instructions for lfs.c.

ldrb      w8, [x8]
tbnz      w8, #0x1, 0x9718
mov       w21, #-0x9
b         0x9730

@geky geky added the needs minor version new functionality only allowed in minor versions label Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs minor version new functionality only allowed in minor versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants