Add RawMessageProvider trait and expand SendError/ReceiveError variants#756
Add RawMessageProvider trait and expand SendError/ReceiveError variants#756
Conversation
Add SendError::Io(i32) and ReceiveError::{ProtocolError, Eof} variants.
Introduce RawMessageProvider trait for direct guest-broker byte-stream
messaging (bypassing IP stack), with default stubs. Add Provider
supertrait bound. Implement (empty defaults) for all platform crates.
Update net/phy.rs to match on new error variants.
|
🤖 SemverChecks 🤖 Click for details |
jaybosamiya-ms
added
the
expmt:shadow-kiln
jaybosamiya-ms
left a comment
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
Currently, neither the implementations on the platform sides, nor the implementations on the shim sides of how this interface will be used have shown up. So it is a little hard to actually confirm if this is the right design. As a concrete alternative design, maybe IpInterfaceProvider should be changed to be a StreamProvider that has send/recv but takes an extra StreamType::IP or StreamType::Raw? I am not sure whether the current design or that design is the nicer one, and it is hard to tell without seeing more.
Nonetheless for this interface this cleanup can be done in the future too, so I don't see any particular reason to block it on figuring the above bits up.
I would like to see the nicer version of the i32 though, since that is unacceptable to merge, so currently marking it as "request changes" but it is quite close to an "approve" right now.
| /// The underlying device returned an I/O error. The packet was not sent. | ||
| #[error("I/O error on send: errno {0}")] | ||
| Io(i32), |
There was a problem hiding this comment.
As mentioned on #743, hardcoding an i32 here is wrong; pick actually relevant errors
| /// When available, this provides a fast path for protocols like 9P that would | ||
| /// otherwise pay the overhead of traversing two smoltcp stacks. |
There was a problem hiding this comment.
Nit: "network stacks" not "smoltcp stacks". Doc strings should not promise unnecessary internal details.
| /// The default implementation returns [`ReceiveError::WouldBlock`] / | ||
| /// [`SendError::Unavailable`], indicating the channel is not available. |
There was a problem hiding this comment.
I think the default should be Unavailable for both, because otherwise a shim has no way to know if it tries to recv first whether it is just that something has not shown up (so it should try again) or whether it is never going to become available.
2 participants
Summary
RawMessageProvidertrait for direct guest-host byte channels that bypass the IP stack. Default impl returnsErr(Unavailable).Providersupertrait bound+ RawMessageProvider.SendErrorwithIo(i32)andUnavailablevariants; expandReceiveErrorwithProtocolErrorandEofvariants. Update doc strings for shared usage acrossIPInterfaceProviderandRawMessageProvider.Split from #743.