patches and low-level development discussion
 help / color / Atom feed
* [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv
@ 2020-07-06 15:59 Alyssa Ross
  2020-07-06 15:59 ` [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation Alyssa Ross
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alyssa Ross @ 2020-07-06 15:59 UTC (permalink / raw)
  To: devel; +Cc: Cole Helbling

It's very surprising that a method called "recv" would end up calling
read().  When UnixSeqpacket was introduced in 1d44223f9, the method
was called "read", so this behaviour made more sense.  It was renamed
to "recv" in b7196e2a1, but the implementation was not changed to call
libc::recv.

In most cases, according to recv(2), the difference between read() and
recv() doesn't matter:

> The only difference between recv() and read(2) is the presence of
> flags.  With a zero flags argument, recv() is generally equivalent
> to read(2) (but see NOTES).

But the NOTES section explains that

> If a zero-length datagram is pending, read(2) and recv() with a
> flags argument of zero provide different behavior.  In this
> circumstance, read(2) has no effect (the datagram remains pending),
> while recv() consumes the pending datagram.

This means that, while we might expect UnixSeqpacket::recv(&mut []) to
mirror the behaviour of recv() and discard a message despite the empty
buffer, it would actually leave it there.  This became relevant when I
implemented the startup message queue in 5bd2e7f60, because I used an
empty recv to drop the ready message from the socket, since it was
only used for polling and I didn't care about the content.  But, it
turns out that my UnixSeqpacket::recv call did nothing, and left the
message on the socket.  This wasn't immediately noticeable, but it
meant that when a message came through on the control socket later,
it would actually get the ready message from UnixSeqpacket::recv,
rather than the expected message on the control socket.

For example, trying to use crosvm disk resize would print a message
like the following:

> [ERROR:vm_control/src/lib.rs:649] unexpected disk socket result: Ready
> [ERROR:devices/src/virtio/block.rs:334] Attempted to resize read-only block device

We can see that it does try to process the received message, but first
it tries to do something with the Ready message, which should have
been discarded by now.

With this fix applied, we can see that it now receives and tries to
process the correct message (but then fails because the VM I tested
didn't support disk resizing!):

> [ERROR:devices/src/virtio/block.rs:334] Attempted to resize read-only block device

Fixes: b7196e2a1c1eb7123e7eace5418b7eb4a3e24dbe
Fixes: 5bd2e7f606b726b598fdc26fe26a9b4029fab6d3
Cc: Cole Helbling <cole.e.helbling@outlook.com>
---
This is the fix I described in This Week in Spectrum[1].

[1]: https://spectrum-os.org/lists/archives/spectrum-discuss/8736654ij2.fsf@alyssa.is/

 sys_util/src/net.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sys_util/src/net.rs b/sys_util/src/net.rs
index 71ab3eeb5..82b4f5bb0 100644
--- a/sys_util/src/net.rs
+++ b/sys_util/src/net.rs
@@ -197,7 +197,7 @@ impl UnixSeqpacket {
         }
     }
 
-    /// Read data from the socket fd to a given buffer
+    /// Receive data from the socket fd to a given buffer.
     ///
     /// # Arguments
     /// * `buf` - A mut reference to the data buffer.
@@ -206,11 +206,11 @@ impl UnixSeqpacket {
     /// * `usize` - The size of bytes read to the buffer.
     ///
     /// # Errors
-    /// Returns error when `libc::read` failed.
+    /// Returns error when `libc::recv` failed.
     pub fn recv(&self, buf: &mut [u8]) -> io::Result<usize> {
         // Safe since we make sure the input `count` == `buf.len()` and handle the returned error.
         unsafe {
-            let ret = libc::read(self.fd, buf.as_mut_ptr() as *mut _, buf.len());
+            let ret = libc::recv(self.fd, buf.as_mut_ptr() as *mut _, buf.len(), 0);
             if ret < 0 {
                 Err(io::Error::last_os_error())
             } else {
-- 
2.26.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation
  2020-07-06 15:59 [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Alyssa Ross
@ 2020-07-06 15:59 ` Alyssa Ross
  2020-07-06 16:24   ` Cole Helbling
  2020-07-06 23:14 ` [PATCH crosvm 2/2 v2] " Alyssa Ross
  2020-07-08 17:06 ` [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Cole Helbling
  2 siblings, 1 reply; 7+ messages in thread
From: Alyssa Ross @ 2020-07-06 15:59 UTC (permalink / raw)
  To: devel; +Cc: Cole Helbling

---
 sys_util/src/net.rs | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/sys_util/src/net.rs b/sys_util/src/net.rs
index 82b4f5bb0..0e1cf6e55 100644
--- a/sys_util/src/net.rs
+++ b/sys_util/src/net.rs
@@ -197,7 +197,9 @@ impl UnixSeqpacket {
         }
     }
 
-    /// Receive data from the socket fd to a given buffer.
+    /// Receive a message from the socket fd to a given buffer.  The message will then
+    /// be removed from the socket, whether `buf` was large enough to hold all of it or
+    /// not.
     ///
     /// # Arguments
     /// * `buf` - A mut reference to the data buffer.
@@ -205,6 +207,30 @@ impl UnixSeqpacket {
     /// # Returns
     /// * `usize` - The size of bytes read to the buffer.
     ///
+    /// # Examples
+    ///
+    /// ```
+    /// # fn main() -> Result<(), sys_util::Error> {
+    /// # use sys_util::net::UnixSeqpacket;
+    /// let (s1, s2) = UnixSeqpacket::pair()?;
+    /// s1.send(b"First message")?;
+    /// s1.send(b"Second message")?;
+    /// s1.send(b"Third message")?;
+    ///
+    /// let mut buf = [0; 13];
+    /// assert_eq!(s2.recv(&mut buf)?, 13);
+    /// assert_eq!(&buf, b"First message");
+    ///
+    /// // Even reading into a buffer zero bytes long will consume a message.
+    /// assert_eq!(s2.recv(&mut [])?, 0);
+    ///
+    /// let mut buf = [0; 5];
+    /// assert_eq!(s2.recv(&mut buf)?, 5);
+    /// assert_eq!(&buf, b"Third");
+    /// # Ok(())
+    /// # }
+    /// ```
+    ///
     /// # Errors
     /// Returns error when `libc::recv` failed.
     pub fn recv(&self, buf: &mut [u8]) -> io::Result<usize> {
-- 
2.26.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation
  2020-07-06 15:59 ` [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation Alyssa Ross
@ 2020-07-06 16:24   ` Cole Helbling
  2020-07-06 23:07     ` Alyssa Ross
  0 siblings, 1 reply; 7+ messages in thread
From: Cole Helbling @ 2020-07-06 16:24 UTC (permalink / raw)
  To: Alyssa Ross, devel

Alyssa Ross <hi@alyssa.is> writes:

> ---
>  sys_util/src/net.rs | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/sys_util/src/net.rs b/sys_util/src/net.rs
> index 82b4f5bb0..0e1cf6e55 100644
> --- a/sys_util/src/net.rs
> +++ b/sys_util/src/net.rs
> @@ -197,7 +197,9 @@ impl UnixSeqpacket {
>          }
>      }
>  
> -    /// Receive data from the socket fd to a given buffer.
> +    /// Receive a message from the socket fd to a given buffer.  The message will then
> +    /// be removed from the socket, whether `buf` was large enough to hold all of it or

"whether or not `buf` was large enough..." sounds better, IMO.

> +    /// not.
>      ///
>      /// # Arguments
>      /// * `buf` - A mut reference to the data buffer.
> @@ -205,6 +207,30 @@ impl UnixSeqpacket {
>      /// # Returns
>      /// * `usize` - The size of bytes read to the buffer.
>      ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # fn main() -> Result<(), sys_util::Error> {
> +    /// # use sys_util::net::UnixSeqpacket;
> +    /// let (s1, s2) = UnixSeqpacket::pair()?;
> +    /// s1.send(b"First message")?;
> +    /// s1.send(b"Second message")?;
> +    /// s1.send(b"Third message")?;
> +    ///
> +    /// let mut buf = [0; 13];
> +    /// assert_eq!(s2.recv(&mut buf)?, 13);
> +    /// assert_eq!(&buf, b"First message");
> +    ///
> +    /// // Even reading into a buffer zero bytes long will consume a message.
> +    /// assert_eq!(s2.recv(&mut [])?, 0);
> +    ///
> +    /// let mut buf = [0; 5];
> +    /// assert_eq!(s2.recv(&mut buf)?, 5);
> +    /// assert_eq!(&buf, b"Third");
> +    /// # Ok(())
> +    /// # }
> +    /// ```
> +    ///
>      /// # Errors
>      /// Returns error when `libc::recv` failed.

Returns *an* error.

>      pub fn recv(&self, buf: &mut [u8]) -> io::Result<usize> {
> -- 
> 2.26.2

I love seeing docs expanded :^)

Cole

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation
  2020-07-06 16:24   ` Cole Helbling
@ 2020-07-06 23:07     ` Alyssa Ross
  0 siblings, 0 replies; 7+ messages in thread
From: Alyssa Ross @ 2020-07-06 23:07 UTC (permalink / raw)
  To: Cole Helbling; +Cc: devel

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

Cole Helbling <cole.e.helbling@outlook.com> writes:

>> -    /// Receive data from the socket fd to a given buffer.
>> +    /// Receive a message from the socket fd to a given buffer.  The message will then
>> +    /// be removed from the socket, whether `buf` was large enough to hold all of it or
>
> "whether or not `buf` was large enough..." sounds better, IMO.

I agree.

>> +    ///
>>      /// # Errors
>>      /// Returns error when `libc::recv` failed.
>
> Returns *an* error.

Ah, yes, I can change this while I'm here.  Good catch. :)

v2 on its way!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH crosvm 2/2 v2] sys_util: expand on UnixSeqpacket documentation
  2020-07-06 15:59 [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Alyssa Ross
  2020-07-06 15:59 ` [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation Alyssa Ross
@ 2020-07-06 23:14 ` " Alyssa Ross
  2020-07-06 23:22   ` Cole Helbling
  2020-07-08 17:06 ` [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Cole Helbling
  2 siblings, 1 reply; 7+ messages in thread
From: Alyssa Ross @ 2020-07-06 23:14 UTC (permalink / raw)
  To: devel; +Cc: Cole Helbling

---
Incorporated Cole's feedback.

 sys_util/src/net.rs | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/sys_util/src/net.rs b/sys_util/src/net.rs
index 82b4f5bb0..8df1743b6 100644
--- a/sys_util/src/net.rs
+++ b/sys_util/src/net.rs
@@ -197,7 +197,9 @@ impl UnixSeqpacket {
         }
     }
 
-    /// Receive data from the socket fd to a given buffer.
+    /// Receive a message from the socket fd to a given buffer.  The message will then
+    /// be removed from the socket, whether or not `buf` was large enough to hold all of
+    /// it.
     ///
     /// # Arguments
     /// * `buf` - A mut reference to the data buffer.
@@ -205,8 +207,32 @@ impl UnixSeqpacket {
     /// # Returns
     /// * `usize` - The size of bytes read to the buffer.
     ///
+    /// # Examples
+    ///
+    /// ```
+    /// # fn main() -> Result<(), sys_util::Error> {
+    /// # use sys_util::net::UnixSeqpacket;
+    /// let (s1, s2) = UnixSeqpacket::pair()?;
+    /// s1.send(b"First message")?;
+    /// s1.send(b"Second message")?;
+    /// s1.send(b"Third message")?;
+    ///
+    /// let mut buf = [0; 13];
+    /// assert_eq!(s2.recv(&mut buf)?, 13);
+    /// assert_eq!(&buf, b"First message");
+    ///
+    /// // Even reading into a buffer zero bytes long will consume a message.
+    /// assert_eq!(s2.recv(&mut [])?, 0);
+    ///
+    /// let mut buf = [0; 5];
+    /// assert_eq!(s2.recv(&mut buf)?, 5);
+    /// assert_eq!(&buf, b"Third");
+    /// # Ok(())
+    /// # }
+    /// ```
+    ///
     /// # Errors
-    /// Returns error when `libc::recv` failed.
+    /// Returns an error when `libc::recv` failed.
     pub fn recv(&self, buf: &mut [u8]) -> io::Result<usize> {
         // Safe since we make sure the input `count` == `buf.len()` and handle the returned error.
         unsafe {
-- 
2.26.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH crosvm 2/2 v2] sys_util: expand on UnixSeqpacket documentation
  2020-07-06 23:14 ` [PATCH crosvm 2/2 v2] " Alyssa Ross
@ 2020-07-06 23:22   ` Cole Helbling
  0 siblings, 0 replies; 7+ messages in thread
From: Cole Helbling @ 2020-07-06 23:22 UTC (permalink / raw)
  To: Alyssa Ross, devel

Alyssa Ross <hi@alyssa.is> writes:

> ---
> Incorporated Cole's feedback.
>
>  sys_util/src/net.rs | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)

LGTM, thanks!

Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv
  2020-07-06 15:59 [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Alyssa Ross
  2020-07-06 15:59 ` [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation Alyssa Ross
  2020-07-06 23:14 ` [PATCH crosvm 2/2 v2] " Alyssa Ross
@ 2020-07-08 17:06 ` Cole Helbling
  2 siblings, 0 replies; 7+ messages in thread
From: Cole Helbling @ 2020-07-08 17:06 UTC (permalink / raw)
  To: Alyssa Ross, devel

Alyssa Ross <hi@alyssa.is> writes:

>  sys_util/src/net.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Better late than never!

Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06 15:59 [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Alyssa Ross
2020-07-06 15:59 ` [PATCH crosvm 2/2] sys_util: expand on UnixSeqpacket documentation Alyssa Ross
2020-07-06 16:24   ` Cole Helbling
2020-07-06 23:07     ` Alyssa Ross
2020-07-06 23:14 ` [PATCH crosvm 2/2 v2] " Alyssa Ross
2020-07-06 23:22   ` Cole Helbling
2020-07-08 17:06 ` [PATCH crosvm 1/2] sys_util: make UnixSeqpacket::recv use libc::recv Cole Helbling

patches and low-level development discussion

Archives are clonable:
	git clone --mirror https://spectrum-os.org/lists/archives/spectrum-devel/0 spectrum-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 spectrum-devel spectrum-devel/ https://spectrum-os.org/lists/archives/spectrum-devel \
		public-inbox+spectrum-devel@spectrum-os.org devel@spectrum-os.org
	public-inbox-index spectrum-devel

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntps://spectrum-os.org/inbox.comp.spectrum.devel
	nntp://spectrum-os.org/inbox.comp.spectrum.devel


AGPL code for this site: https://ftp.qyliss.net/public-inbox/public-inbox-1.2.0-qyliss-pyyyj8sghi6gjpwx7g5sqipbd7sxfnzr.tar.gz