>> 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 for ChosenSocket<'a> where T: AsRef, { fn as_ref(&self) -> &UnixSeqpacket { self.socket } } Importantly, there would be no public constructor for this type. That way, when you called do_request, the ChosenSocket value would be compile-time proof that you had previously called VmRequest::socket. It could then do a runtime check that the request that the ChosenSocket was contructed with matches the request that it is now being called with. The advantage to this approach would be no annoying callback, but the disadvantage would be that it would be possible to pass a ChosenSocket for a different request, and only have that be checked at runtime. It would still be better than the current situation of no checking, though, and passing in the wrong ChosenSocket would probably be a difficult thing to do accidentally. (3) Make each each VmRequest case a newtype, like so: #[derive(MsgOnSocket, Debug)] pub enum VmRequest { Exit(Exit), Suspend(Suspend), ... } This would basically be a workaround for the fact that enum variants in Rust aren't proper types. If they were, though (which we can emulate through this approach), we could have some trait with an associated socket type for executing each type of request. Both 1 and 3 would provide full compile-time type safety, at the expense of being a bit more horrible than 2. I think I'm leaning towards 1, but I don't have strong feelings about any of them. Having thought about this now though, in response to your question (thanks!), I definitely think that we should do something here better than what I presented in these patches. I'd be very curious to hear your thoughts. You might also be interested in implementing one of them. Would be a good starter crosvm exercise. It's probably the type of thing where you could be pretty confident you'd got it right if it compiled, but I could offer some guidance on testing it as well, if you wanted. If not, though, please give me your thoughts on approach anyway, and I'll implement whichever one we think is best myself at some point. :) > 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!