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

Assert on NULL IO functions #1052

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

wangdongustc
Copy link
Contributor

@wangdongustc wangdongustc commented Dec 9, 2024

Assert that the IO functions (especially the sync function when using non-caching storage) are provided by the user.

@wangdongustc

This comment was marked as resolved.

@wangdongustc wangdongustc changed the title Assert on NULL sync function Assert on NULL IO function Dec 9, 2024
@wangdongustc wangdongustc changed the title Assert on NULL IO function Assert on NULL sync function Dec 9, 2024
@geky-bot
Copy link
Collaborator

geky-bot commented Dec 9, 2024

Tests passed ✓, Code: 17128 B, Stack: 1440 B, Structs: 812 B
Code Stack Structs Coverage
Default 17128 B 1440 B 812 B Lines 2396/2568 lines
Readonly 6250 B 448 B 812 B Branches 1280/1610 branches
Threadsafe 17976 B 1440 B 820 B Benchmarks
Multiversion 17200 B 1440 B 816 B Readed 29369693876 B
Migrate 18816 B 1736 B 816 B Proged 1482874766 B
Error-asserts 17872 B 1432 B 812 B Erased 1568888832 B

@geky
Copy link
Member

geky commented Dec 9, 2024

Hi @wangdongustc, thanks for creating a PR.

I think this would be more useful in lfs_init, which is where we have other config-related asserts. lfs_init is always called before any filesystem operations.

Are you able to also include asserts on read/prog/erase being non-null? These should all probably follow the same rules.

But only if LFS_READONLY is not defined:

LFS_ASSERT(lfs->cfg->read != NULL);
#ifndef LFS_READONLY
LFS_ASSERT(lfs->cfg->prog != NULL);
LFS_ASSERT(lfs->cfg->erase != NULL);
LFS_ASSERT(lfs->cfg->sync != NULL);
#endif

This gets quite a bit more nuanced than it first appears...


And clever solution for the prettyasserts.py issue, thanks for that.

I had a fix for pointer comparisons on an unrelated branch (b5e264b), but I like your solution more.

(We will probably want to end up with both eventually to allow pointer comparisons without a literal NULL)

@wangdongustc wangdongustc changed the title Assert on NULL sync function Assert on NULL IO function Dec 10, 2024
@wangdongustc wangdongustc changed the title Assert on NULL IO function Assert on NULL IO functions Dec 10, 2024
@wangdongustc
Copy link
Contributor Author

wangdongustc commented Dec 10, 2024

@geky Thanks for the feedback!

Are you able to also include asserts on read/prog/erase being non-null? These should all probably follow the same rules.

Done!

(Actually I did this earlier yesterday. But then I thought, why bother asserting the "read" function? Nobody would expect that lfs will work without a "read" function. So I changed back to only assert the "sync" function later. 🤣

image

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17128 B, Stack: 1440 B, Structs: 812 B
Code Stack Structs Coverage
Default 17128 B 1440 B 812 B Lines 2399/2571 lines
Readonly 6250 B 448 B 812 B Branches 1283/1616 branches
Threadsafe 17976 B 1440 B 820 B Benchmarks
Multiversion 17200 B 1440 B 816 B Readed 29369693876 B
Migrate 18816 B 1736 B 816 B Proged 1482874766 B
Error-asserts 17892 B 1432 B 812 B Erased 1568888832 B

@geky
Copy link
Member

geky commented Dec 10, 2024

But then I thought, why bother asserting the "read" function? Nobody would expect that lfs will work without a "read" function.

Now I'm wondering what it would take to support write-only memory 😬

Thanks for this! Merging!

@geky geky added this to the v2.10 milestone Dec 10, 2024
@geky geky merged commit b8e4433 into littlefs-project:devel Dec 10, 2024
16 checks passed
@bmcdonnell-fb
Copy link

write-only memory 😬

WOM. 😆

@geky geky mentioned this pull request Dec 10, 2024
@geky
Copy link
Member

geky commented Dec 10, 2024

https://web.archive.org/web/20160313132244/https://dl.dropboxusercontent.com/u/85027770/25120SpecTwoSides%20NC.pdf 😄

Apparently someone wrote a kernel driver for it: https://github.com/spacerace/write-only-memory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants