Occasionally, it is useful to be able to use UnixSeqpacket as a type that can represent any kind of MsgSocket. For example, to keep some MsgSockets of different types in a Vec. In this case, it may be known what type of messages should be sent over a socket, even though that may not be represantable in the type system.
To accomodate this situation, this patch introduces send_msg_on_socket and recv_msg_on_socket methods on UnixSeqpacket, that can be used to send or receive any kind of MsgOnSocket. The caller is obviously responsible for ensuring that the messages being sent are of the type expected by the socket.
This lack of type safety for message types is not ideal, and so MsgSender and MsgReceiver should still be preferred wherever possible.
Is there anyway this can or will be rectified?
I started out writing about why I didn't think this was something that
necessarily needed to be rectified, but in the course of writing that up
came up with a couple of potential solutions, so maybe it should be
after all. :P
Did you see the way it was used in the second patch? We put all the
sockets into a Vec<&UnixSeqpacket>, ask a function which is the right
socket to send the message to, and then send the message to that
socket. It's not really possible to do that sort of thing while
preserving type information, because you can't really have a collection
containing members of different types.
The reason this is done at all is to avoid duplicating a match statement
between two different functions (because I think that would be likely to
go out of sync). This does mean that it's the responsibility of the
caller of VmRequest::execute to pass in the right socket, though, which
is not ideal.
So here are some options I came up with:
(1) Since VmRequest::socket is only ever really going to be used to
inspect the socket value before then passing it along to
VmRequest::execute, we could drop VmRequest::socket, move the match
statement back into VmRequest::execute, but have it take a callback
parameter, which would be called with the chosen socket as a
&UnixSeqpacket. That would mean we wouldn't have to assume the
correct type of socket in VmRequest::execute, but the caller could
still see which was chosen. The problem is that in run_control, we
use the chosen socket to decide whether to proceed with the request
at all, or to do something else (queue it for later). So then the
closure would have to return a bool or something indicating whether
to actually do the request or not. All feasible, but starts to
sound a little awkward. We might have to change the name of
do_request considering that it now might or might not do a request
depending on what the callback told it.
(2) Keep VmRequest::socket, but have it return a type like this:
pub struct ChosenSocket<'a> {
request: &'a VmRequest,
socket: &'a UnixSeqpacket,
}
impl<'a> AsRef<UnixSeqpacket> for ChosenSocket<'a>
where
T: AsRef
Otherwise, I don't see anything obviously wrong, but I'm also not very familiar with sockets.
Reviewed-by: Cole Helbling
Thank you so much for the review! I wasn't expecting a review for this patch (haven't really got one for crosvm stuff before), so I didn't wait very long before I pushed it. The review is still very very much appreciated, though -- as I hope my wall of text above demonstrates, your review is definitely going to lead to better quality code. But, because I wasn't expecting a review and pushed already, it's too late to include your review in the commit message. :( I'm going to try to figure out how to use git notes (git-notes(1)) this morning, which I think should let me annotate the commit with your review after the fact, such that it shows up in the log message and everything. I'll definitely wait longer to push crosvm patches in future, now that I know somebody might be looking at them!