Minor release: v2.10 #1050
Merged
Minor release: v2.10 #1050
Conversation
This file contains 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
- block_cycles is signed and should use PRId32 - flags is signed (which is a bit weird) and should be cast for %x Unfortunately exactly what PRI* expands to is dependant on both the compiler and the underlying architecture, so I don't think it's possible for us to catch these mistakes with CI... Found by stefano-zanotti
Turns out major versions break things. Old behavior: Artifacts with same name are merged New behavior: Artifacts with same name error Using a pattern and merging on download should fix this at least on the job-side. Though I do wonder if we'll start running into artifact limit issues with the new way artifacts are handled...
Looks like cross-workflow downloads has finally been added to the standard download-artifact action, so we might as well switch to it to reduce dependencies. dawidd6's version was also missing the merge-multiple feature which is necessary to work around breaking changes in download-artifact's v4 bump. Weirdly it needs GITHUB_TOKEN for some reason? Not sure why this couldn't be implicit.
With GitHub forcibly deprecating old versions of actions, pinning the minor/patch version is more likely to cause breakage than not.
Update github actions to the latest versions
With the existing method, (-DLFS_MALLOC=my_malloc) users often had to use compiler options like -include, which was not so portable. This change introduces another way to provide partial overrides of lfs_util.h using a user-provided header.
Original tests provided by m-kostrzewa, these identify signed overflow (undefined behavior) when compiled with -fsanitize=undefined.
In the previous implementation of lfs_file_seek, we calculated the new offset using signed arithmetic before checking for possible overflow/underflow conditions. This results in undefined behavior in C. Fortunately for us, littlefs is now limited to 31-bit file sizes for API reasons, so we don't have to be too clever here. Doing the arithmetic with unsigned integers and just checking if we're in a valid range afterwards should work. Found by m-kostrzewa and lucic71
- Added METADATA_MAX to test_runner. - Added METADATA_MAX to bench_runner. - Added a simple metadata_max test to test_superblocks, for lack of better location. There have been several issues floating around related to metadata_max and LFS_ERR_NOSPC which makes me think there's a bug in our metadata_max logic. metadata_max was a quick patch and is relatively untested, so an undetected bug isn't too surprising. This commit adds at least some testing over metadata_max. Sure enough, the new test_superblocks_metadata_max test reveals a curious LFS_ERR_NAMETOOLONG error that shouldn't be there. More investigation needed.
The inconsistency here between the use of block_size vs metadata_max was suspicious. Turns out there's a bug when metadata_max == prog_size. We correctly use metadata_max for the block_size/2 check, but we weren't using it for the block_size-40 check. The second check seems unnecessary after the first, but it protects against running out of space in a commit for commit-related metadata (checksums, tail pointers, etc) when we can't program half-blocks. Turns out this is also needed when limiting metadata_max to a single prog, otherwise we risk erroring with LFS_ERR_NOSPC early. Found by ajheck, dpkristensen, NLLK, and likely others.
Like the read/prog/block_size checks, these are just asserts. If these invariants are broken the filesystem will break in surprising ways.
These two small libraries provide examples of error-correction compatible with littlefs (or any filesystem really). It would be nice to eventually provide these as drop-in solutions, but right now it's not really possible without breaking changes to littlefs's block device API. In the meantime, ramcrc32bd and ramrsbd at least provide example implementations that can be adapted to users' own block devices.
This should be a superset of the previous test_paths test suite, while covering a couple more things (more APIs, more path synonyms, utf8, non-printable ascii, non-utf8, etc). Not yet tested are some corner cases with known bugs, mainly around trailing slashes.
As expected these are failing and will need some work to pass. The issue with lfs_file_open allowing trailing slashes was found by rob-zeno, and the issue with lfs_mkdir disallowing trailing slashes was found by XinStellaris, PoppaChubby, pavel-kirienko, inf265, Xywzel, steverpalmer, and likely others.
- lfs_mkdir now accepts trailing slashes:
- before: lfs_mkdir("a/") => LFS_ERR_NOENT
- after: lfs_mkdir("a/") => 0
- lfs_stat, lfs_getattr, etc, now reject trailing slashes if the file is
not a directory:
- before: lfs_stat("reg_a/") => 0
- after: lfs_stat("reg_a/") => LFS_ERR_NOTDIR
Note trailing slashes are accepted if the file is a directory:
- before: lfs_stat("dir_a/") => 0
- after: lfs_stat("dir_a/") => 0
- lfs_file_open now returns LFS_ERR_NOTDIR if the file exists but the
path contains trailing slashes:
- before: lfs_file_open("reg_a/") => LFS_ERR_NOENT
- after: lfs_file_open("reg_a/") => LFS_ERR_NOTDIR
To make these work, the internal lfs_dir_find API required some
interesting changes:
- lfs_dir_find no longer sets id=0x3ff on not finding a parent entry in
the path. Instead, lfs_path_islast can be used to determine if the
modified path references a parent entry or child entry based on the
remainder of the path string.
Note this is only necessary for functions that create new entries
(lfs_mkdir, lfs_rename, lfs_file_open).
- Trailing slashes mean we can no longer rely on the modified path being
NULL-terminated. lfs_path_namelen provides an alternative to strlen
that stops at slash or NULL.
- lfs_path_isdir also tells you if the modified path must reference a
dir (contains trailing slashes). I considered handling this entirely
in lfs_dir_find, but the behavior of entry-creating functions is too
nuanced.
At least lfs_dir_find returns LFS_ERR_NOTDIR if the file exists on
disk.
Like strlen, lfs_path_namelen/islast/isdir are all O(n) where n is the
name length. This isn't great, but if you're using filenames large
enough for this to actually matter... uh... open an issue on GitHub and
we might improve this in the future.
---
There are a couple POSIX incompatibilities that I think are not
worth fixing:
- Root modifications return EINVAL instead of EBUSY:
- littlefs: remove("/") => EINVAL
- POSIX: remove("/") => EBUSY
Reason: This would be the only use of EBUSY in the system.
- We accept modifications of directories with trailing dots:
- littlefs: remove("a/.") => 0
- POSIX: remove("a/.") => EBUSY
Reason: Not worth implementing.
- We do not check for existence of directories followed by dotdots:
- littlefs: stat("a/missing/..") => 0
- POSIX: stat("a/missing/..") => ENOENT
Reason: Difficult to implement non-recursively.
- We accept modifications of directories with trailing dotdots:
- littlefs: rename("a/b/..", "c") => 0
- POSIX: rename("a/b/..", "c") => EBUSY
Reason: Not worth implementing.
These are at least now documented in tests/test_paths.toml, which isn't
the greatest location, but it's at least something until a better
document is created.
Note that these don't really belong in SPEC.md because path parsing is
a function of the driver and has no impact on disk.
This changes the behavior of paths that attempt to navigate above root
to now return LFS_ERR_INVAL:
- before: lfs_stat("/../a") => 0
- after: lfs_stat("/../a") => LFS_ERR_INVAL
This is a bit of an opinionated change while making other path
resolution tweaks.
In terms of POSIX-compatibility, it's a bit unclear exactly what dotdots
above the root should do.
POSIX notes:
> As a special case, in the root directory, dot-dot may refer to the
> root directory itself.
But the word choice of "may" implies it is up to the implementation.
I originally implement this as a root-loop simply because that is what
my Linux machine does, but I now think that's not the best option. Since
we're making other path-related tweaks, we might as well try to adopt
behavior that is, in my opinion, safer and less... weird...
This should also help make paths more consistent with future theoretical
openat-list APIs, where saturating at the current directory is sort of
the least expected behavior.
Unlike normal files, dots (".") should not change the depth when
attempting to skip dotdot ("..") entries.
A weird nuance in the path parser, but at least it had a relatively easy
fix.
Added test_paths_dot_dotdots to prevent a regression.
Before this, the empty path ("") was treated as an alias for the root.
This was unintentional and just a side-effect of how the path parser
worked.
Now, the empty path should always result in LFS_ERR_INVAL:
- before: lfs_stat("") => 0
- after: lfs_stat("") => LFS_ERR_INVAL
These flags change the behavior of open quite significantly. It's useful to cover these in our path tests so the behavior is locked down.
- test_paths_noent_trailing_slashes - test_paths_noent_trailing_dots - test_paths_noent_trailing_dotdots These managed to slip through our path testing but should be tested, if anything just to know exactly what errors these return.
- before: lfs_file_open("missing/") => LFS_ERR_ISDIR
- after: lfs_file_open("missing/") => LFS_ERR_NOTDIR
As noted by bmcdonnell-fb, returning LFS_ERR_ISDIR here was inconsistent
with the case where the file exists:
case before after
lfs_file_open("dir_a") => LFS_ERR_ISDIR LFS_ERR_ISDIR
lfs_file_open("dir_a/") => LFS_ERR_ISDIR LFS_ERR_ISDIR
lfs_file_open("reg_a/") => LFS_ERR_NOTDIR LFS_ERR_NOTDIR
lfs_file_open("missing_a/") => LFS_ERR_ISDIR LFS_ERR_NOTDIR
Note this is consistent with the behavior of lfs_stat:
lfs_file_open("reg_a/") => LFS_ERR_NOTDIR
lfs_stat("reg_a/") => LFS_ERR_NOTDIR
And the only other function that can "create" files, lfs_rename:
lfs_file_open("missing_a/") => LFS_ERR_NOTDIR
lfs_rename("reg_a", "missing_a/") => LFS_ERR_NOTDIR
There is some ongoing discussion about if these should return NOTDIR,
ISDIR, or INVAL, but this is at least an improvement over the
rename/open mismatch.
Write the detect cycles function as a function to optimize code
Add an alternative way to override LFS_MALLOC etc
Fix some more LFS_TRACE format specifiers
Fix seek undefined behavior on signed integer overflow
Fix metadata_max==prog_size commit->end calculation
Add links to ramcrc32bd and ramrsbd
paths: Revisit path parsing, fix trailing slash behavior
Tests passed ✓, Code: 17128 B (+0.4%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
|
|
EDIT: Updated with draft of release notes |
Looks like I missed a line during refactoring, resulted in only x86 sizes being reported in GitHub statuses. If we wanted to limited these to one architecture, thumb would have probably been a better pick.
|
EDIT: Adding #1053 to fix a minor CI issue |
Tests passed ✓, Code: 17128 B (+0.4%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
|
|
EDIT: Last minute addition of #1052 |
Tests passed ✓, Code: 17128 B (+0.4%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
|
Tests passed ✓, Code: 17128 B (+0.4%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In hindsight this should have probably been split into multiple releases. Attempting to merge unrelated PRs into the same minor release led to some issues being unresolved much longer than necessary.
Main new features include LFS_DEFINES (#1004) by @yamt as an easier way to override lfs_utils.h, and a rework of the internal path parser to better align with POSIX/user expectations.
Thanks to everyone who contributed!
Bringing in:
Draft of release notes follows:
This holiday themed release brings in some minor, but highly requested, features.
Potentially breaking changes:
Path parsing has been reworked internally to better align with POSIX/user expectations in a number of corner cases. This changes behavior of paths with trailing slashes, navigating above root, and empty paths.
See path changes below for a full list of what's changed.
What's new?
Path parsing has been reworked internally to better align with POSIX/user expectations in a number of corner cases (#1046)
See path changes below for more details.
Thanks to @yamt, LFS_DEFINES now provides an easier alternative to LFS_CONFIG for partial util overrides (#1004)
This is useful for when you want to override some parts of
lfs_utils.h, but keep most of the existing definitions:# in compile flags -DLFS_DEFINES=my_utils.h@wdfk-prog found some nice code savings by deduplicating
lfs_tortoise_detectcycles(#1013)Thanks to @wangdongustc, littlefs now asserts if block-device callbacks are NULL (#1052)
Added links to ramcrc32bd and ramrsbd (#1038)
These are two new example block devices that implement error-correction compatible with littlefs (or any filesystem really).
littlefs currently does not provide error-correction or error-detection. These are intended to be a good starting point if this is a requirement for your system.
Fixed metadata compaction bug that may cause early LFS_ERR_NOSPC when using
metadata_max(#1031)Fixed undefined signed overflow in
lfs_file_seekfound by @m-kostrzewa and @lucic71 (#1027)Fixed some LFS_TRACE format specifiers found by @stefano-zanotti (#997)
Thanks to @yamt, fixed an annoying GitHub Actions breakage (#1026)
Path changes
lfs_mkdirnow accepts trailing slashes:lfs_stat,lfs_getattr, etc, now reject trailing slashes if the file is not a directory:Note trailing slashes are accepted if the file is a directory:
lfs_file_opennow returns LFS_ERR_NOTDIR if the path contains trailing slashes and the file is not a directory, or LFS_O_CREAT is specified and the file does not exist:Note trailing slashes return LFS_ERR_ISDIR if the file is a directory:
Note LFS_ERR_NOENT takes priority if LFS_O_CREAT is not specified:
Attempting to navigate above the root directory now returns LFS_ERR_INVAL:
This actually moves away from the behavior of other filesystems, but towards, in my opinion, more reasonable behavior. It should also be more consistent with future theoretical
openat-like APIs.The empty path ("") no longer aliases the root directory, now returns LFS_ERR_INVAL:
POSIX deviations
While this gets us closer to POSIX, there are still some subtle differences in behaviors. These shouldn't affect most users, but are worth documenting. Most of these are due to 1. littlefs not actually creating
.and..entries on disk and 2. not using recursion during path resolution.Root modifications return EINVAL instead of EBUSY:
Reason: This would be the only use of EBUSY in the system.
We accept modifications of directories with trailing dots:
Reason: Not worth implementing.
We do not check for existence of directories followed by dotdots:
Reason: Difficult to implement non-recursively.
We accept modifications of directories with trailing dotdots:
Reason: Not worth implementing.