From: Alyssa Ross <hi@alyssa.is>
To: Cole Helbling <cole.e.helbling@outlook.com>
Cc: devel@spectrum-os.org
Subject: Re: [PATCH crosvm 1/2] msg_socket: introduce UnixSeqpacketExt
Date: Tue, 16 Jun 2020 09:32:19 +0000 [thread overview]
Message-ID: <87a713qrto.fsf@alyssa.is> (raw)
In-Reply-To: <CH2PR14MB357902F389083AFA8FA045DDB39D0@CH2PR14MB3579.namprd14.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 6361 bytes --]
>> 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<UnixSeqpacket<T>,
{
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 <cole.e.helbling@outlook.com>
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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
next prev parent reply other threads:[~2020-06-16 9:32 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-14 11:43 [PATCH crosvm 0/2] Fix deadlock on early VmRequest Alyssa Ross
2020-06-14 11:43 ` [PATCH crosvm 1/2] msg_socket: introduce UnixSeqpacketExt Alyssa Ross
2020-06-16 0:17 ` Cole Helbling
2020-06-16 9:32 ` Alyssa Ross [this message]
2020-06-22 22:06 ` Cole Helbling
2020-06-23 2:32 ` Alyssa Ross
2020-06-25 1:54 ` impaqt
2020-07-09 13:24 ` Alyssa Ross
2020-06-14 11:43 ` [PATCH crosvm 2/2] crosvm: fix deadlock on early VmRequest Alyssa Ross
2020-06-16 1:08 ` Cole Helbling
2020-06-16 9:39 ` Alyssa Ross
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a713qrto.fsf@alyssa.is \
--to=hi@alyssa.is \
--cc=cole.e.helbling@outlook.com \
--cc=devel@spectrum-os.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://spectrum-os.org/git/crosvm
https://spectrum-os.org/git/doc
https://spectrum-os.org/git/mktuntap
https://spectrum-os.org/git/nixpkgs
https://spectrum-os.org/git/spectrum
https://spectrum-os.org/git/ucspi-vsock
https://spectrum-os.org/git/www
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).