Conversation
Co-authored-by: 朝倉水希 <asakuramizu111@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for vsock (Virtual Socket) devices, specifically virtio-vsock, enabling socket-based communication in virtual machine environments.
Key changes:
- Introduces a new
axdriver_vsockcrate defining common traits and types for vsock drivers - Implements VirtIO socket device driver in
axdriver_virtiowith connection management and event handling - Extends the base device type enumeration to include vsock devices
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| axdriver_vsock/src/lib.rs | Defines core vsock types (VsockAddr, VsockConnId, VsockDriverEvent) and the VsockDriverOps trait interface |
| axdriver_vsock/Cargo.toml | Package metadata and dependencies for the new axdriver_vsock crate |
| axdriver_virtio/src/socket.rs | Implements VirtIoSocketDev wrapper around virtio-drivers' VsockConnectionManager with event conversion logic |
| axdriver_virtio/src/lib.rs | Integrates socket support via feature flag, adds Socket device type mapping, and extends error conversion for socket-specific errors |
| axdriver_virtio/Cargo.toml | Adds socket feature and axdriver_vsock dependency |
| axdriver_base/src/lib.rs | Extends DeviceType enum with Vsock variant |
| Cargo.toml | Adds axdriver_vsock workspace member |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
axdriver_vsock/src/lib.rs
Outdated
| /// Forcibly closes the connection without waiting for the peer. | ||
| fn abort(&mut self, cid: VsockConnId) -> DevResult<()>; | ||
|
|
||
| /// poll event from driver |
There was a problem hiding this comment.
The buf parameter is passed to poll_event but is unused in the Ok(None) branch and the error path. The buffer is only used when converting events in the Ok(Some(event)) case. Consider documenting the purpose of this buffer parameter in the trait definition, as it's not clear why callers need to provide it.
| /// poll event from driver | |
| /// Poll a single event from the driver. | |
| /// | |
| /// The caller provides `buf` as a scratch buffer that implementations may use | |
| /// to decode or temporarily store driver-specific data associated with the | |
| /// returned [`VsockDriverEvent`]. The buffer is typically only used when an | |
| /// event is actually available (`Ok(Some(event))`) and may be ignored when | |
| /// there is no event (`Ok(None)`) or when an error is returned. | |
| /// | |
| /// Implementations must not rely on the contents of `buf` on entry, and | |
| /// callers should ensure it is large enough for any driver-specific payload | |
| /// they expect to handle. |
| #[allow(dead_code)] | ||
| const fn as_dev_err(e: virtio_drivers::Error) -> DevError { | ||
| use virtio_drivers::Error::*; | ||
| use virtio_drivers::{Error::*, device::socket::SocketError::*}; |
There was a problem hiding this comment.
The import of SocketError::* may cause namespace pollution since it brings all socket error variants into scope. This could lead to naming conflicts if other error types have similar variant names. Consider using qualified paths like device::socket::SocketError::ConnectionExists in the match arms instead, or limiting the wildcard import scope.
| Ok(VsockDriverEvent::Received(cid, read)) | ||
| } | ||
| VsockEventType::Disconnected { reason: _ } => Ok(VsockDriverEvent::Disconnected(cid)), | ||
| _ => Ok(VsockDriverEvent::Unknown), |
There was a problem hiding this comment.
The wildcard pattern _ in the match statement catches all unknown event types and returns VsockDriverEvent::Unknown. While this provides a fallback, it may silently ignore newly added event types in future versions of the virtio-drivers crate. Consider logging a warning when an unknown event type is encountered to aid debugging.
Add support for vsock devices, like virtio-vsock.