sys_util: Refactor IntoIovec

The original stated purpose of this trait was to reduce memory
allocations but having the `into_iovec` method return a Vec kind of
defeats that purpose.

Refactor the trait so that it can either convert a T into an iovec or
convert a &[T] into a &[iovec].  Implement the trait for VolatileSlice,
IoSlice, and IoSliceMut and update all the callers.

BUG=none
TEST=unit tests

Cq-Depend: chromium:2210272
Change-Id: I9d0d617a23030d241d50411f4a5a16e7cba4bcee
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2208527
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: Chirantan Ekbote <chirantan@chromium.org>

6 files changed, 78 insertions, 86 deletions

diff --git a/crosvm_plugin/src/lib.rs b/crosvm_plugin/src/lib.rs
index 05c19bf..e9d135c 100644
--- a/crosvm_plugin/src/lib.rs
+++ b/crosvm_plugin/src/lib.rs
@@ -17,7 +17,7 @@
 
 use std::env;
 use std::fs::File;
-use std::io::{Read, Write};
+use std::io::{IoSlice, Read, Write};
 use std::mem::{size_of, swap};
 use std::os::raw::{c_int, c_void};
 use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
@@ -290,7 +290,7 @@ impl crosvm {
             .write_to_vec(&mut self.request_buffer)
             .map_err(proto_error_to_int)?;
         self.socket
-            .send_with_fds(self.request_buffer.as_slice(), fds)
+            .send_with_fds(&[IoSlice::new(self.request_buffer.as_slice())], fds)
             .map_err(|e| -e.errno())?;
 
         let mut datagram_fds = [0; MAX_DATAGRAM_FD];
diff --git a/devices/src/virtio/descriptor_utils.rs b/devices/src/virtio/descriptor_utils.rs
index fcf5793..c878a45 100644
--- a/devices/src/virtio/descriptor_utils.rs
+++ b/devices/src/virtio/descriptor_utils.rs
@@ -14,9 +14,7 @@ use std::ptr::copy_nonoverlapping;
 use std::result;
 
 use data_model::{DataInit, Le16, Le32, Le64, VolatileMemoryError, VolatileSlice};
-use sys_util::{
-    FileReadWriteAtVolatile, FileReadWriteVolatile, GuestAddress, GuestMemory, IntoIovec,
-};
+use sys_util::{FileReadWriteAtVolatile, FileReadWriteVolatile, GuestAddress, GuestMemory};
 
 use super::DescriptorChain;
 
@@ -166,33 +164,6 @@ impl<'a> DescriptorChainConsumer<'a> {
 
         other
     }
-
-    fn get_iovec(&mut self, len: usize) -> io::Result<DescriptorIovec<'a>> {
-        let mut iovec = Vec::with_capacity(self.get_remaining().len());
-
-        let mut rem = len;
-        for buf in self.get_remaining() {
-            let iov = if rem < buf.size() {
-                // Safe because we know that `rem` is in-bounds.
-                buf.sub_slice(0, rem).unwrap().as_iovec()
-            } else {
-                buf.as_iovec()
-            };
-
-            rem -= iov.iov_len;
-            iovec.push(iov);
-
-            if rem == 0 {
-                break;
-            }
-        }
-        self.consume(len);
-
-        Ok(DescriptorIovec {
-            iovec,
-            mem: PhantomData,
-        })
-    }
 }
 
 /// Provides high-level interface over the sequence of memory regions
@@ -381,6 +352,19 @@ impl<'a> Reader<'a> {
         self.buffer.bytes_consumed()
     }
 
+    /// Returns a `&[VolatileSlice]` that represents all the remaining data in this `Reader`.
+    /// Calling this method does not actually consume any data from the `Reader` and callers should
+    /// call `consume` to advance the `Reader`.
+    pub fn get_remaining(&self) -> &[VolatileSlice] {
+        self.buffer.get_remaining()
+    }
+
+    /// Consumes `amt` bytes from the underlying descriptor chain. If `amt` is larger than the
+    /// remaining data left in this `Reader`, then all remaining data will be consumed.
+    pub fn consume(&mut self, amt: usize) {
+        self.buffer.consume(amt)
+    }
+
     /// Splits this `Reader` into two at the given offset in the `DescriptorChain` buffer. After the
     /// split, `self` will be able to read up to `offset` bytes while the returned `Reader` can read
     /// up to `available_bytes() - offset` bytes. If `offset > self.available_bytes()`, then the
@@ -390,12 +374,6 @@ impl<'a> Reader<'a> {
             buffer: self.buffer.split_at(offset),
         }
     }
-
-    /// Returns a DescriptorIovec for the next `len` bytes of the descriptor chain
-    /// buffer, which can be used as an IntoIovec.
-    pub fn get_iovec(&mut self, len: usize) -> io::Result<DescriptorIovec<'a>> {
-        self.buffer.get_iovec(len)
-    }
 }
 
 impl<'a> io::Read for Reader<'a> {
@@ -581,12 +559,6 @@ impl<'a> Writer<'a> {
             buffer: self.buffer.split_at(offset),
         }
     }
-
-    /// Returns a DescriptorIovec for the next `len` bytes of the descriptor chain
-    /// buffer, which can be used as an IntoIovec.
-    pub fn get_iovec(&mut self, len: usize) -> io::Result<DescriptorIovec<'a>> {
-        self.buffer.get_iovec(len)
-    }
 }
 
 impl<'a> io::Write for Writer<'a> {
@@ -617,18 +589,6 @@ impl<'a> io::Write for Writer<'a> {
     }
 }
 
-pub struct DescriptorIovec<'a> {
-    iovec: Vec<libc::iovec>,
-    mem: PhantomData<&'a GuestMemory>,
-}
-
-// Safe because the lifetime of DescriptorIovec is tied to the underlying GuestMemory.
-unsafe impl<'a> IntoIovec for DescriptorIovec<'a> {
-    fn into_iovec(&self) -> Vec<libc::iovec> {
-        self.iovec.clone()
-    }
-}
-
 const VIRTQ_DESC_F_NEXT: u16 = 0x1;
 const VIRTQ_DESC_F_WRITE: u16 = 0x2;
 
diff --git a/devices/src/virtio/wl.rs b/devices/src/virtio/wl.rs
index 8e94783..3a5ecee 100644
--- a/devices/src/virtio/wl.rs
+++ b/devices/src/virtio/wl.rs
@@ -730,12 +730,10 @@ impl WlVfd {
     fn send(&mut self, fds: &[RawFd], data: &mut Reader) -> WlResult<WlResp> {
         if let Some(socket) = &self.socket {
             socket
-                .send_with_fds(
-                    data.get_iovec(usize::max_value())
-                        .map_err(WlError::ParseDesc)?,
-                    fds,
-                )
+                .send_with_fds(data.get_remaining(), fds)
                 .map_err(WlError::SendVfd)?;
+            // All remaining data in `data` is now considered consumed.
+            data.consume(::std::usize::MAX);
             Ok(WlResp::Ok)
         } else if let Some((_, local_pipe)) = &mut self.local_pipe {
             // Impossible to send fds over a simple pipe.
diff --git a/msg_socket/src/lib.rs b/msg_socket/src/lib.rs
index 7f5d1a6..540d49d 100644
--- a/msg_socket/src/lib.rs
+++ b/msg_socket/src/lib.rs
@@ -4,7 +4,7 @@
 
 mod msg_on_socket;
 
-use std::io::Result;
+use std::io::{IoSlice, Result};
 use std::marker::PhantomData;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::pin::Pin;
@@ -138,7 +138,8 @@ pub trait MsgSender: AsRef<UnixSeqpacket> {
             handle_eintr!(sock.send(&msg_buffer))
                 .map_err(|e| MsgError::Send(SysError::new(e.raw_os_error().unwrap_or(0))))?;
         } else {
-            sock.send_with_fds(&msg_buffer[..], &fd_buffer[0..fd_size])
+            let ioslice = IoSlice::new(&msg_buffer[..]);
+            sock.send_with_fds(&[ioslice], &fd_buffer[0..fd_size])
                 .map_err(MsgError::Send)?;
         }
         Ok(())
diff --git a/src/plugin/process.rs b/src/plugin/process.rs
index 783239a..688aa85 100644
--- a/src/plugin/process.rs
+++ b/src/plugin/process.rs
@@ -5,7 +5,7 @@
 use std::collections::hash_map::{Entry, HashMap, VacantEntry};
 use std::env::set_var;
 use std::fs::File;
-use std::io::Write;
+use std::io::{IoSlice, Write};
 use std::mem::transmute;
 use std::os::unix::io::{IntoRawFd, RawFd};
 use std::os::unix::net::UnixDatagram;
@@ -730,7 +730,7 @@ impl Process {
             .map_err(Error::EncodeResponse)?;
         assert_ne!(self.response_buffer.len(), 0);
         self.request_sockets[index]
-            .send_with_fds(&self.response_buffer[..], &response_fds)
+            .send_with_fds(&[IoSlice::new(&self.response_buffer[..])], &response_fds)
             .map_err(Error::PluginSocketSend)?;
 
         Ok(())
diff --git a/sys_util/src/sock_ctrl_msg.rs b/sys_util/src/sock_ctrl_msg.rs
index 30303da..77a724d 100644
--- a/sys_util/src/sock_ctrl_msg.rs
+++ b/sys_util/src/sock_ctrl_msg.rs
@@ -6,10 +6,12 @@
 //! (e.g. Unix domain sockets).
 
 use std::fs::File;
+use std::io::{IoSlice, IoSliceMut};
 use std::mem::size_of;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use std::os::unix::net::{UnixDatagram, UnixStream};
 use std::ptr::{copy_nonoverlapping, null_mut, write_unaligned};
+use std::slice;
 
 use libc::{
     c_long, c_void, cmsghdr, iovec, msghdr, recvmsg, sendmsg, MSG_NOSIGNAL, SCM_RIGHTS, SOL_SOCKET,
@@ -103,17 +105,17 @@ impl CmsgBuffer {
     }
 }
 
-fn raw_sendmsg<D: IntoIovec>(fd: RawFd, out_data: D, out_fds: &[RawFd]) -> Result<usize> {
+fn raw_sendmsg<D: IntoIovec>(fd: RawFd, out_data: &[D], out_fds: &[RawFd]) -> Result<usize> {
     let cmsg_capacity = CMSG_SPACE!(size_of::<RawFd>() * out_fds.len());
     let mut cmsg_buffer = CmsgBuffer::with_capacity(cmsg_capacity);
 
-    let mut iovec = out_data.into_iovec();
+    let iovec = IntoIovec::as_iovecs(out_data);
 
     let mut msg = msghdr {
         msg_name: null_mut(),
         msg_namelen: 0,
-        msg_iov: iovec.as_mut_ptr(),
-        msg_iovlen: 1,
+        msg_iov: iovec.as_ptr() as *mut iovec,
+        msg_iovlen: iovec.len(),
         msg_control: null_mut(),
         msg_controllen: 0,
         msg_flags: 0,
@@ -230,7 +232,7 @@ pub trait ScmSocket {
     ///
     /// * `buf` - A buffer of data to send on the `socket`.
     /// * `fd` - A file descriptors to be sent.
-    fn send_with_fd<D: IntoIovec>(&self, buf: D, fd: RawFd) -> Result<usize> {
+    fn send_with_fd<D: IntoIovec>(&self, buf: &[D], fd: RawFd) -> Result<usize> {
         self.send_with_fds(buf, &[fd])
     }
 
@@ -242,7 +244,7 @@ pub trait ScmSocket {
     ///
     /// * `buf` - A buffer of data to send on the `socket`.
     /// * `fds` - A list of file descriptors to be sent.
-    fn send_with_fds<D: IntoIovec>(&self, buf: D, fd: &[RawFd]) -> Result<usize> {
+    fn send_with_fds<D: IntoIovec>(&self, buf: &[D], fd: &[RawFd]) -> Result<usize> {
         raw_sendmsg(self.socket_fd(), buf, fd)
     }
 
@@ -307,27 +309,55 @@ impl ScmSocket for UnixSeqpacket {
 ///
 /// This trait is unsafe because interfaces that use this trait depend on the base pointer and size
 /// being accurate.
-pub unsafe trait IntoIovec {
-    /// Gets a vector of structures describing each contiguous region of a memory buffer.
-    fn into_iovec(&self) -> Vec<libc::iovec>;
+pub unsafe trait IntoIovec: Sized {
+    /// Returns a `iovec` that describes a contiguous region of memory.
+    fn into_iovec(&self) -> iovec;
+
+    /// Returns a slice of `iovec`s that each describe a contiguous region of memory.
+    fn as_iovecs(bufs: &[Self]) -> &[iovec];
+}
+
+// Safe because there are no other mutable references to the memory described by `IoSlice` and it is
+// guaranteed to be ABI-compatible with `iovec`.
+unsafe impl<'a> IntoIovec for IoSlice<'a> {
+    fn into_iovec(&self) -> iovec {
+        iovec {
+            iov_base: self.as_ptr() as *mut c_void,
+            iov_len: self.len(),
+        }
+    }
+
+    fn as_iovecs(bufs: &[Self]) -> &[iovec] {
+        // Safe because `IoSlice` is guaranteed to be ABI-compatible with `iovec`.
+        unsafe { slice::from_raw_parts(bufs.as_ptr() as *const iovec, bufs.len()) }
+    }
 }
 
-// Safe because this slice can not have another mutable reference and it's pointer and size are
-// guaranteed to be valid.
-unsafe impl<'a> IntoIovec for &'a [u8] {
-    fn into_iovec(&self) -> Vec<libc::iovec> {
-        vec![libc::iovec {
-            iov_base: self.as_ref().as_ptr() as *const c_void as *mut c_void,
+// Safe because there are no other references to the memory described by `IoSliceMut` and it is
+// guaranteed to be ABI-compatible with `iovec`.
+unsafe impl<'a> IntoIovec for IoSliceMut<'a> {
+    fn into_iovec(&self) -> iovec {
+        iovec {
+            iov_base: self.as_ptr() as *mut c_void,
             iov_len: self.len(),
-        }]
+        }
+    }
+
+    fn as_iovecs(bufs: &[Self]) -> &[iovec] {
+        // Safe because `IoSliceMut` is guaranteed to be ABI-compatible with `iovec`.
+        unsafe { slice::from_raw_parts(bufs.as_ptr() as *const iovec, bufs.len()) }
     }
 }
 
 // Safe because volatile slices are only ever accessed with other volatile interfaces and the
 // pointer and size are guaranteed to be accurate.
 unsafe impl<'a> IntoIovec for VolatileSlice<'a> {
-    fn into_iovec(&self) -> Vec<libc::iovec> {
-        vec![self.as_iovec()]
+    fn into_iovec(&self) -> iovec {
+        self.as_iovec()
+    }
+
+    fn as_iovecs(bufs: &[Self]) -> &[iovec] {
+        VolatileSlice::as_iovecs(bufs)
     }
 }
 
@@ -385,8 +415,9 @@ mod tests {
     fn send_recv_no_fd() {
         let (s1, s2) = UnixDatagram::pair().expect("failed to create socket pair");
 
+        let ioslice = IoSlice::new([1u8, 1, 2, 21, 34, 55].as_ref());
         let write_count = s1
-            .send_with_fds([1u8, 1, 2, 21, 34, 55].as_ref(), &[])
+            .send_with_fds(&[ioslice], &[])
             .expect("failed to send data");
 
         assert_eq!(write_count, 6);
@@ -407,8 +438,9 @@ mod tests {
         let (s1, s2) = UnixDatagram::pair().expect("failed to create socket pair");
 
         let evt = EventFd::new().expect("failed to create eventfd");
+        let ioslice = IoSlice::new([].as_ref());
         let write_count = s1
-            .send_with_fd([].as_ref(), evt.as_raw_fd())
+            .send_with_fd(&[ioslice], evt.as_raw_fd())
             .expect("failed to send fd");
 
         assert_eq!(write_count, 0);
@@ -434,8 +466,9 @@ mod tests {
         let (s1, s2) = UnixDatagram::pair().expect("failed to create socket pair");
 
         let evt = EventFd::new().expect("failed to create eventfd");
+        let ioslice = IoSlice::new([237].as_ref());
         let write_count = s1
-            .send_with_fds([237].as_ref(), &[evt.as_raw_fd()])
+            .send_with_fds(&[ioslice], &[evt.as_raw_fd()])
             .expect("failed to send fd");
 
         assert_eq!(write_count, 1);