From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.3 (2019-12-06) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-1.5 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=3.4.3 Received: by atuin.qyliss.net (Postfix, from userid 496) id DBF7E9A7A; Tue, 16 Jun 2020 09:32:36 +0000 (UTC) Received: from [127.0.0.1] (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id B1F099AF6; Tue, 16 Jun 2020 09:32:31 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 496) id 975899A66; Tue, 16 Jun 2020 09:32:29 +0000 (UTC) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by atuin.qyliss.net (Postfix) with ESMTPS id E0F029A65 for ; Tue, 16 Jun 2020 09:32:25 +0000 (UTC) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id DD1775C00BF; Tue, 16 Jun 2020 05:32:23 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Tue, 16 Jun 2020 05:32:23 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h= from:to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=fm3; bh=jmj+PItCrPlkqfd0Ux50QXAFmB c7RG0MBMn79BZffY4=; b=iqoLVYiKeElBVj9IvE8CHhVRVA+Utspc78dBsOEMnN u85dA4Mvdd+b4wlGLY0oK6kOLql1WkyBhuP0kw54l4pUQIDekIFj4yc5dtitXKQb ttC9X1IGlYZ47Tv7e3+7+/JTmsBI3f5KCP0N6I/dM+JAHAHPFurYifOaHnerBtvd aCMk12WWCj4UTCsnp6k9FOzR1Np70IODb7IhVSxLeVta+dr0DsJ9rZCPKvFPf7oF qpVrTlv6VZHsS9u6lhbm7mkkFoFKJFgZtiz6NWeUl5Qe6QCz42SDWUuLFgOQJZyD ILh0mqDy+U6rrAluiKCgxIk2ZoYOVKrshOLd7lSTdn3w== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=jmj+PI tCrPlkqfd0Ux50QXAFmBc7RG0MBMn79BZffY4=; b=leBPxJimMRFqs5HiQDg4ip isg6wFPjIIGZ3kMAoLDsxR4jOeTgS9hVT1LY68nrvdCCL7xNKw1AbV3oYgp6rC4z E9uSGE7B8iwb3ZdBvFGox7kRIfpbKX8XqiEEVYeb0hsRTkyKLtNqjRlCBwm0vJJ1 yGWGpfP6DlUVVcypaTByrp8HTNDK7YP+Q5vfGgL5TU+k4samdDYOAREET4O8Dedy ORTBmtUwfJzUO2+8XTrfszVrjOszrr2CPGA4EmqhVovQCK8sKP+tXuI7GGjdcI+5 iEW9V/VfpPkoGtaSFS3i/KDtobmCYkaqZrkyfevvTDFVv8I/AbSRTXDMZY7eT1lQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrudejtddgudelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufgjfhffkfggtgesghdtreertddttdenucfhrhhomheptehlhihsshgr ucftohhsshcuoehhihesrghlhihsshgrrdhisheqnecuggftrfgrthhtvghrnhepvdeuvd eivdehhfeikedtvddvhefgheffleevueeigffgtdffffeujedujeejkeejnecukfhppeeg iedrkedtrdduvdekrdduhedvnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpe hmrghilhhfrhhomhephhhisegrlhihshhsrgdrihhs X-ME-Proxy: Received: from x220.qyliss.net (p2e508098.dip0.t-ipconnect.de [46.80.128.152]) by mail.messagingengine.com (Postfix) with ESMTPA id 285C83280063; Tue, 16 Jun 2020 05:32:23 -0400 (EDT) Received: by x220.qyliss.net (Postfix, from userid 1000) id 60E55FD0; Tue, 16 Jun 2020 09:32:21 +0000 (UTC) From: Alyssa Ross To: Cole Helbling Subject: Re: [PATCH crosvm 1/2] msg_socket: introduce UnixSeqpacketExt In-Reply-To: References: Date: Tue, 16 Jun 2020 09:32:19 +0000 Message-ID: <87a713qrto.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Message-ID-Hash: JEPT6QJW7QUTP3JWNJQKMTHMIK6WMY4A X-Message-ID-Hash: JEPT6QJW7QUTP3JWNJQKMTHMIK6WMY4A X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header CC: devel@spectrum-os.org X-Mailman-Version: 3.3.1 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable >> 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: =20=20=20=20 pub struct ChosenSocket<'a> { request: &'a VmRequest, socket: &'a UnixSeqpacket, } =20=20=20=20 impl<'a> AsRef for ChosenSocket<'a> where T: AsRef, { fn as_ref(&self) -> &UnixSeqpacket { self.socket } } =20=20=20=20 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. =20=20=20=20 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), ... } =20=20=20=20 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! --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEH9wgcxqlHM/ARR3h+dvtSFmyccAFAl7okaMACgkQ+dvtSFmy ccC7nw/8CRbxTthWjixuZksgbadcQwqa5woEOAMoo3T7lL5fCR/tuhr8xvX5m+sa Px8KDHpWA9gBdiG2uuiUnGL71phPhZcxkpRiBNE7fSp947a+cIgwS/J36JKZu47n N9HoWpTS1DXQxe3bNhqnyc1zvxIDAHM7dXvisrcCxoMnZTiqrrxJSS0pCQJCi9AL Uly61pK7XnTmcvadpLB5XrOaPD6SZ047JvA9zPNR/2egXd+PXKylpmr4SUsZ5U2g CZ/8IrNnofsOjLwyyBUu+B3b7QFzHGcKd9yBM3M7bures16WZ2oCgLbi5KGUahHU xvq/dBElUNoS8GywX/prHGxVyLwR1P+kq0zCmZt98D7Ase+ri6XVrj4A/ZfYQKvy k2jBcTKk3rFMx0Me0F8QvzgfuE3GeOICURg+HNPPJMDkkemoMGNQNNaCrE/Jp9br Oy4eaSGgJ9EqI1vKvu11BEfM5kq+CYcInoJ93ysX7DYwR3bltvIUs6I5ldhVi5kp fr/dnMnsMFHxie6WY/S0yePLzBik2KEencvVKtfOzFZOhFSsJ0dtVtKQ0+wLlpkf JECCrjRO2mADsA1uXGpwBnmr7IBF/9YcS2J5vlOWf9P9PSZoENb/OuD9J0bYn//6 Z03Vo3nxEmlrrJW1gRtt+8wThLakLZ89VNirtxrGufpdvfNv+/U= =LWed -----END PGP SIGNATURE----- --=-=-=--