Draft: changes and bug fixes while using this in esp-idf. POSIX compliant readdir()#6
Open
Vincent-Dalstra wants to merge 17 commits into
Open
Draft: changes and bug fixes while using this in esp-idf. POSIX compliant readdir()#6Vincent-Dalstra wants to merge 17 commits into
Vincent-Dalstra wants to merge 17 commits into
Conversation
This works when managed by the registry, but not as a git submodule, so it needs to be disabled while developing.
This makes Unit-tests better, because they can allocate/cleanup every single time. However, it fails with ESP_ERR_INVALID_STATE each time. Could this be a clue to why things are going wrong?
Bad sizeof operator. A classic c bug! I added a runtime check that will catch that error if it were to resurface again. It's also preferable to the previous behaviour of 'truncate silently if the path is too long'. Give people errors when they do things wrong!
I had introduced this bug while adding an error message. I thought I was helping, but did not consider the && in the if() statement. Split the if(&&) statement into two if() statements, to make the control flow clearer - and prevent the above from happening.
When using this component to substitute for the esp-idf's fatfs component, I discovered various bugs and non-POSIX-compliant behaviour that resulted in unit tests failing. - Not handling trailing '/' in paths properly. - Incorrect results from readdir() when the directory is modified. - Setting errno when an operatioon succeeds. - Setting ENFILE instead of EISDIR during unlink(). These errors were fixed using Test-Driven-Development, meaning there is excellent test coverage. Some of the original commit messages are kept below: --- POSIX compliant readdir() behaviour when files are deleted When a file is added or deleted from a directory after opendir(), POSIX leaves unspecified whether the file will be returned from a subsequent readdir() call. That only applies to the the files that were added and removed, though. All other files should be returned if-and-only-if they were not already returned. However, because the array of entries is shifted back when an element is removed, removing a file behind the iterator would result in a file being skipped! To reliably detect changes, a copy of the directory handle is made during opendir(), and entry pointer compared during each iteration to see if anything was removed. Use correct errno value for unlink() Previously, when unlink() was called on a directory it would return ENFILE, which I think was intended to represent "not a file", but which really means "run out of file descriptors". Now returns the correct value of EISDIR. Check for errors during ramfs_rmtree() Ignore trailing slash's in opendir(), mkdir() etc... Fix opendir() returning NULL for empty directories. Fix erroneously setting errno in soem situations. Do not allow trailing slash when path refers to a file
THis was done to make the Unit-test app simpler; to speed up tests, the the file system is generally left mounted after each test. But the ramfs unit tests use a different path, and so try to setup a second filesystem, exceeding the previous maximum of just 1. Furthermore, it's only 24 bytes of static memory per additional slot, so there's very little harm in increasing it a bit.
Remove lines with commented-out dependencies. Add a note about how dependencies are small on purpose.
Owner
|
Wow cool, thank you for your efforts. I will make time to review your changes this week! Sorry for the delayed response. |
Author
|
No worries. I was considering making something like this, so this saved me a lot of effort. I think my unit tests are about 10x faster, at a rough guess. |
Replace assert() statements with the corresponding TEST_ASSERT_NULL or TEST_ASSERT_NOT_NULL or TEST_ASSERT_EQUAL statements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The changes I've made work, and are all unit-tested, but it's a bit messy: I should tidy up the commits more, and some of the code could be made more efficient (readdir(), specifically, when it hits the 'unhappy path').
It could be some time before I get it done, though, so I thought I may as well share the code as it is now. Could save someone some effort.
Do not merge this as it is. Revert 03d9707 first, at the very least.
Context: I was running lots of unit tests that interacted with a file system, using fatfs on an eMMC card - that's about as good as it gets, but its still much slower than memory. So using something like ramfs seems perfect! Unfortunately, swapping out fatfs for ramfs caused many tests to start failing.
These changes are the result of me (1) creating test cases that reveal bugs, or where behaviour does not adhere to the POSIX standards, and (2) fixing those bugs.
esp-idf version = v5.4.1
Main changes: