Allow moving file handles #1124
Conversation
Tests passed ✓, Code: 17160 B (+0.3%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
geky
added
enhancement
needs minor version
|
|
||
| lfs_file_close(&lfs, &file) => 0; | ||
|
|
||
| lfs_file_ishandleopen(&lfs, &file) => LFS_ERR_BADF; |
There was a problem hiding this comment.
I hadn't thought about the return value of lfs_file_ishandleopen, LFS_ERR_BADF is a good choice. It's also the first use of this error code in the codebase.
Humorously, LFS_ERR_BADF is removed in v3-alpha, but we can bring it back for this purpose.
LFS_ERR_BADF also makes sense for a theoretical LFS_UNTRUSTED_USER build, where file operations error if the file is not open. Some users have asked for such a feature, but it's been low priority. (I'm also hesitant because it isn't bulletproof, we can't provide the same guardrails around if a filesystem is mounted, for example.)
There was a problem hiding this comment.
Humorously, LFS_ERR_BADF is removed in v3-alpha, but we can bring it back for this purpose.
That is hilarious! I was also debating LFS_ERR_INVAL, but LFS_ERR_BADF just felt right.
|
Thanks for the PR. And including tests! I have just have some style related nits. And another request: Are you able to I can clean up these nits/commits on my end, but it messes with the commit author. Though these are low-priority due to the ongoing feature freeze (#1111). |
Add lfs_file_movehandle to allow moving file handles in the rare case it is needed. Add lfs_file_ishandleopen to allow checking if a file handle is open.
jacobschloss
force-pushed
the
lfs_file_movehandle
branch
from
2156c07 to
c8d01e1
Compare
|
I gave the edits a shot |
Tests passed ✓, Code: 17160 B, Stack: 1448 B, Structs: 812 B
|
|
Looks great! Sorry about the late response, but thanks for updating/amending based on feedback. Will bring this into the next minor release in both littlefs2 and littlefs3, once the feature freeze is lifted. |
Edit for style
jacobschloss
force-pushed
the
lfs_file_movehandle
branch
from
c8d01e1 to
056ba42
Compare
Tests passed ✓, Code: 17160 B (+0.3%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
|
Looks good here! Thanks for the extra changes. One of these days we'll have actual checks to catch style issues. Will bring this into the next minor release in both littlefs2 and littlefs3, once the feature freeze is lifted. |
Provide a lfs_file_movehandle and lfs_file_ishandleopen function to allow moving file handles.
While users should move file handles rarely, being able to move them allows for better integration with C++ RAII wrappers, which users would like to be able to move around -- even if only once in order to place them into a std::optional. Other options could involve using heap or a special arena allocator, but this is nice and simple. Providing a move function also indirectly hints that the lfs_file_t can't just be memcpy'd, and documents that it is expensive and if you are doing it a lot you might need to do something different.
I have a small amount of test code for this I could put somewhere that spins through opening and moving a few file handles.
Previous discussion at #765.