Prefer alloc::io than no_std_io2 - #3722
Conversation
|
/aster-code-review |
|
✅ On it! I'll review the PR diff, running the PR's own Track progress here: https://github.com/asterinas/asterinas/actions/runs/31935628510 I'll post the results back on this PR when the run finishes. |
There was a problem hiding this comment.
Note
These comments were generated by AI using the aster-code-review skill. Please take them seriously——but also with a grain of salt. You can run aster-code-review locally to iterate on your changes (review -> fix -> re-review) and catch issues before they reach manual review.
The change successfully removes the direct no_std_io2 dependency and keeps the
replacement alloc::io usage compiling for cpio-decoder and aster-core on
x86_64-unknown-none. I did not find runtime correctness, security, hardware,
or documentation defects in the reviewed diff.
The remaining issue is commit hygiene: the final cleanup commit combines several
unrelated edits under one generic subject, which makes the series harder to
review and bisect.
Findings not attachable to this PR's diff
Note
These couldn't be pinned to a changed line
— usually they sit on lines this PR didn't change (likely pre-existing issues),
or concern a commit message.
Listed here so they aren't lost.
commit cc45285ef message — atomic-commits (minor): cc45285ef groups unrelated cleanups: it narrows FileLike::access_mode, changes inotify log/error wording and local naming, removes #[inline] from check_length, adds whitespace in init process setup, and rewrites a local VmarMapOptions binding. That makes the commit hard to review or bisect as one logical change.
Fix. Split this into one commit per logical cleanup, or fold each small cleanup into the commit that introduced the nearby code it adjusts.
tatetian
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the contribution!
Our toolchain version is now
nightly-2026-07-21.This is a nice number, which means that rust-lang/rust#158544 has been merged (2026-07-18), so that
alloc::io::Readis available to use.asterinas/kernel/core/src/fs/initramfs.rs
Line 20 in 4373656
Therefore, there is no need to rely on
no_std_io2. I propose removing it to eliminate another external crate.We can use
alloc::io::Readinstead. See the changes in this first commit.(Note:
libflatestill depends onno_std_io2. I don't think it is controversial to clean up our dependencies first, though.)I also noticed that
InodeWriterhas only one user. Given that I cannot think of a second user and it does not rely on any inode's implementation details, I think it is better to just move it to where it is used. This is done in the second commit.The third commit performs some random style cleanups. The correctness should be obvious.