-
Notifications
You must be signed in to change notification settings - Fork 5
feat(filesystem):add async vfs path #545
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
Conversation
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi Imai <[email protected]>
Signed-off-by: Koichi <[email protected]>
…feat/async_vfs_path
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
Signed-off-by: Koichi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adapts the synchronous and asynchronous virtual filesystem paths for no_std environments, replacing SystemTime with a custom Time type and restructuring modules to separate sync (awkernel_lib) and async (awkernel_async_lib) code. Key changes include:
- Migrated
awkernel_libVFS path API to useallocandTimeand exposedPathLikepublicly. - Introduced an async VFS layer in
awkernel_async_libwith updated file-system traits,AsyncVfsPath, and a newasync_copyhelper. - Updated module exports (
pub mod path/file) and added new dependencies (async-trait,async-recursion).
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| awkernel_lib/src/file/vfs/path.rs | Switched imports to alloc, replaced SystemTime with Time, made PathLike public |
| awkernel_lib/src/file/vfs.rs | Re-exported path module |
| awkernel_async_lib/src/lib.rs | Added top-level file module |
| awkernel_async_lib/src/file/path.rs | Rewrote async path logic, updated method signatures, added async_copy, removed many docs |
| awkernel_async_lib/src/file/filesystem.rs | Extended AsyncSeekAndRead with read_to_string, added Debug to AsyncFileSystem |
| awkernel_async_lib/src/file.rs | Introduced filesystem and path modules |
| awkernel_async_lib/Cargo.toml | Added async-trait and async-recursion dependencies |
Comments suppressed due to low confidence (5)
awkernel_async_lib/src/file/path.rs:1
- [nitpick] Most public methods have lost their doc comments; adding concise documentation (including examples) for key APIs like
join,read_dir, andcopy_filewill improve discoverability and maintainability.
//! Async virtual filesystem path
awkernel_async_lib/src/file/filesystem.rs:113
- Adding a
Debugbound toAsyncFileSystemis a breaking change for existing implementations; consider documenting this change or making the bound optional to preserve backward compatibility.
pub trait AsyncFileSystem: Sync + Send + Debug {
awkernel_async_lib/src/file/path.rs:58
- [nitpick] Changing
AsyncVfsPath::newfrom a generic parameter to aBox<dyn AsyncFileSystem>reduces API flexibility; consider providing a generic constructor or helper to allow callers to pass concrete types directly.
pub fn new(filesystem: Box<dyn AsyncFileSystem + Send + Sync>) -> Self {
awkernel_async_lib/src/file/path.rs:613
- The new
async_copyfunction does not have any accompanying tests; adding unit tests (including error paths) will help ensure correctness and prevent regressions.
pub async fn async_copy<R, W>(reader: &mut R, writer: &mut W) -> VfsResult<u64>
awkernel_lib/src/file/vfs/path.rs:11
- The
Vecimport does not appear to be used in this file after trimming its contents; consider removing it to avoid an unused import.
};
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Koichi <[email protected]>
Description
This PR ports AsyncVfsPath for no_std environments. File systems that implement AsyncFileSystem, which was introduced in a previous PR, will be able to perform various file operations using this AsyncVfsPath.
This pull request primarily involves changes due to VfsError becoming VfsError(PR #548), switching from std::time::SystemTime to awkernel_lib::time::Time, and removing unnecessary comments.
std::io::Read read_to_string() is used in read_to_string() method of this AsyncVfsPath. Therefore, I've implemented it myself in this PR, since we didn't have read_to_string() method in our AsyncSeekAndRead trait. It seems like it contains two unnecessary copies, but this is to follow the rule below.
Related links
#548 (delete type parameter E from VfsError)
#539 (introducing AsyncFilesystem)
#511 (main PR)
How was this PR tested?
Notes for reviewers
As a result of these recent deletions, awkernel_lib/src/file/vfs/path.rs is now almost empty, containing only helper functions necessary for AsyncVfsPath and the definition of VfsMetadata. It's highly likely that this content will eventually be moved to awkernel_async_lib/src/file/path.rs, but I'm holding off on that for this PR to avoid making it overly complex.