Conversation
wdcui
left a comment
There was a problem hiding this comment.
I went through the code and left some comments. Thank you for adding 9p support in LiteBox!
litebox/src/fs/layered.rs
Outdated
| match self.lower.file_status(path) { | ||
| Ok(stat) => Ok(stat.file_type), | ||
| Err(FileStatusError::ClosedFd) => unreachable!(), | ||
| Err(FileStatusError::Io) => Err(PathError::NoSuchFileOrDirectory), |
There was a problem hiding this comment.
Io is a pretty general erorr name. Why would it imply NoSuchFileOrDirectory?
There was a problem hiding this comment.
Good catch. PathError does not have Io. I updated the return type.
| #[error("error when truncating: {0}")] | ||
| TruncateError(#[from] TruncateError), | ||
| #[error("I/O error")] | ||
| Io, |
There was a problem hiding this comment.
Should we interpret Io error as network failure instead of the file or directory not exist?
There was a problem hiding this comment.
Yes, I updated the code.
litebox/src/fs/nine_p/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Walk to the parent of a path and return the parent fid and the name |
litebox/src/fs/nine_p/mod.rs
Outdated
| super::FileStatus { | ||
| file_type, | ||
| mode: super::Mode::from_bits_truncate(attr.stat.mode), | ||
| size: usize::try_from(attr.stat.size).expect("file size exceeds usize"), |
There was a problem hiding this comment.
expect() will cause litebox to crash, right? Should we avoid that?
|
|
||
| let bytes_read = self.client.read(fid, read_offset as u64, buf)?; | ||
|
|
||
| // Update offset if not using explicit offset |
There was a problem hiding this comment.
Shouldn't we always update the offset stored in the descriptor table?
There was a problem hiding this comment.
If the offset is provided by user, it means the file offset should remain unchanged.
| /// Tagged 9P message | ||
| #[derive(Clone, Debug)] | ||
| pub(super) struct TaggedFcall<'a> { | ||
| pub(super) tag: u16, |
There was a problem hiding this comment.
Concurrent requests may receive out-of-order responses. Each response should also have a tag that matches its request.
There was a problem hiding this comment.
I added some comments.
litebox/src/fs/nine_p/fcall.rs
Outdated
| encode_fcall(buf, tag, fcall)?; | ||
|
|
||
| // Write the size at the beginning | ||
| let size = u32::try_from(buf.len()).expect("buffer length exceeds u32"); |
There was a problem hiding this comment.
Should we return the error instead of panic?
There was a problem hiding this comment.
I agree. Updated all expect to return an error instead.
litebox/src/fs/nine_p/fcall.rs
Outdated
| } | ||
|
|
||
| // ============================================================================ | ||
| // Encoding functions |
There was a problem hiding this comment.
Can we simplify the encoding/decoding to serde of structs?
There was a problem hiding this comment.
Good point! Encoding and decoding have a lot of repetition. serde has its own binary representation which may not conform to the 9p protocol. I asked agent to use macro to simplify the code.
| //! 9P transport layer abstraction | ||
| //! | ||
| //! This module defines traits for reading and writing 9P protocol messages over | ||
| //! an underlying transport (e.g., TCP socket, virtio-9p, etc.). |
There was a problem hiding this comment.
Where is the network integration?
There was a problem hiding this comment.
The network stack (e.g., TCP) is implemented in Linux shim. I will add it in the next PR.
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// A wrapper around `TcpStream` that implements the litebox 9P transport traits. | ||
| struct TcpTransport { |
There was a problem hiding this comment.
Why do we have both implementations and tests in this file?
There was a problem hiding this comment.
This uses std sockets for testing purposes only. All the following tests set up diod as remote server and thus require sockets for connection. This crate does not have network stack implementation. In the next PR, I will use linux shim to test 9p as well.
litebox/src/fs/nine_p/mod.rs
Outdated
| //! | ||
| //! # Submodules | ||
| //! | ||
| //! The 9P implementation is split into several submodules: | ||
| //! - `fcall` - Protocol message definitions and encoding/decoding | ||
| //! - `transport` - Transport layer traits and message I/O | ||
| //! - `client` - High-level 9P client for protocol operations |
There was a problem hiding this comment.
Minor comment (I have not reviewed the code, just was looking through the docs): we might wish to skip this from the doc-strings and change it to a regular comment. Anything in the doc-strings (here, module-level) effectively behaves like a spec. The details most of these submodules (which are not necessarily public) is not useful/needed to callers of this module.
There was a problem hiding this comment.
Good point! I removed it.
|
🤖 SemverChecks 🤖 Click for details |
This PR adds implementation of 9p fs that uses 9P2000.L protocol. For testing, I used
diodas the remote server. The implementation of 9p client was largely migrated from VSBox.Note that different from other fs, this only support blocking operations as it relies on network. Not sure if it breaks any assumption that LiteBox has.
Currently it only allows issuing one call over network at a time and waiting for it to complete. We should allow concurrent requests. I will fix it in the next PR.