Refactor FileDescription::read/write helper

Author: tiif

src/shims/unix/fd.rs

@@ -151,7 +151,11 @@ impl FileDescription for io::Stdin {
             helpers::isolation_abort_error("`read` from stdin")?;
         }
         let result = Read::read(&mut { self }, &mut bytes);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(rw_bytes) =>
+                ecx.return_read_write_success(Some(ptr), Some(&bytes), true, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -182,7 +186,10 @@ impl FileDescription for io::Stdout {
         // the host -- there is no good in adding extra buffering
         // here.
         io::stdout().flush().unwrap();
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(rw_bytes) => ecx.return_read_write_success(None, None, false, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -208,7 +215,10 @@ impl FileDescription for io::Stderr {
         // We allow writing to stderr even with isolation enabled.
         // No need to flush, stderr is not buffered.
         let result = Write::write(&mut { self }, bytes);
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(rw_bytes) => ecx.return_read_write_success(None, None, false, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -235,8 +245,7 @@ impl FileDescription for NullOutput {
         ecx: &mut MiriInterpCx<'tcx>,
     ) -> InterpResult<'tcx> {
         // We just don't write anything, but report to the user that we did.
-        let result = Ok(len);
-        ecx.return_written_byte_count_or_error(result, dest)
+        ecx.return_read_write_success(None, None, false, len, dest)
     }
 }
 
@@ -655,47 +664,44 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(())
     }
 
-    /// Helper to implement `FileDescription::read`:
-    /// `result` should be the return value of some underlying `read` call that used `bytes` as its output buffer.
+    /// Helper to implement `FileDescription::read/write`:
     /// The length of `bytes` must not exceed either the host's or the target's `isize`.
-    /// If `Result` indicates success, `bytes` is written to `buf` and the size is written to `dest`.
-    /// Otherwise, `-1` is written to `dest` and the last libc error is set appropriately.
-    fn return_read_bytes_and_count(
+    /// `is_read` is used to indicate if this helper is used for `read`.
+    /// If it is `read`, `bytes` is written to `buf`.
+    /// For both `read` and `write`,`actual_rw_size`, which is the actual read/write size,
+    /// is written to `dest`.
+    fn return_read_write_success(
         &mut self,
-        buf: Pointer,
-        bytes: &[u8],
-        result: io::Result<usize>,
+        buf: Option<Pointer>,
+        bytes: Option<&[u8]>,
+        is_read: bool,
+        actual_rw_size: usize,
         dest: &MPlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        match result {
-            Ok(read_bytes) => {
-                // If reading to `bytes` did not fail, we write those bytes to the buffer.
-                // Crucially, if fewer than `bytes.len()` bytes were read, only write
-                // that much into the output buffer!
-                this.write_bytes_ptr(buf, bytes[..read_bytes].iter().copied())?;
-                // The actual read size is always less than what got originally requested so this cannot fail.
-                this.write_int(u64::try_from(read_bytes).unwrap(), dest)?;
-                Ok(())
-            }
-            Err(e) => {
-                this.set_last_error_from_io_error(e)?;
-                this.write_int(-1, dest)?;
-                Ok(())
-            }
+        if is_read {
+            let bytes = bytes.unwrap();
+            // If reading to `bytes` did not fail, we write those bytes to the buffer.
+            // Crucially, if fewer than `bytes.len()` bytes were read, only write
+            // that much into the output buffer!
+            this.write_bytes_ptr(buf.unwrap(), bytes[..actual_rw_size].iter().copied())?;
         }
+        // The actual read/write size is always less than what got originally requested so this cannot fail.
+        this.write_int(u64::try_from(actual_rw_size).unwrap(), dest)?;
+        Ok(())
     }
 
-    /// This function writes the number of written bytes (given in `result`) to `dest`, or sets the
-    /// last libc error and writes -1 to dest.
-    fn return_written_byte_count_or_error(
+    /// Helper to implement `FileDescription::read/write`.
+    /// This is only used when there is a read/write error.
+    ///`-1` is written to `dest` and the last libc error is set appropriately.
+    fn return_read_write_error(
         &mut self,
-        result: io::Result<usize>,
+        err: io::Error,
         dest: &MPlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let result = this.try_unwrap_io_result(result.map(|c| i64::try_from(c).unwrap()))?;
-        this.write_int(result, dest)?;
+        this.set_last_error(this.io_error_to_errnum(err)?)?;
+        this.write_int(-1, dest)?;
         Ok(())
     }
 }

src/shims/unix/fs.rs

@@ -42,7 +42,11 @@ impl FileDescription for FileHandle {
         assert!(communicate_allowed, "isolation should have prevented even opening a file");
         let mut bytes = vec![0; len];
         let result = (&mut &self.file).read(&mut bytes);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(rw_bytes) =>
+                ecx.return_read_write_success(Some(ptr), Some(&bytes), true, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn write<'tcx>(
@@ -57,7 +61,10 @@ impl FileDescription for FileHandle {
         assert!(communicate_allowed, "isolation should have prevented even opening a file");
         let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;
         let result = (&mut &self.file).write(bytes);
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(rw_bytes) => ecx.return_read_write_success(None, None, false, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn pread<'tcx>(
@@ -85,7 +92,11 @@ impl FileDescription for FileHandle {
             res
         };
         let result = f();
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(rw_bytes) =>
+                ecx.return_read_write_success(Some(ptr), Some(&bytes), true, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn pwrite<'tcx>(
@@ -113,7 +124,10 @@ impl FileDescription for FileHandle {
             res
         };
         let result = f();
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(rw_bytes) => ecx.return_read_write_success(None, None, false, rw_bytes, dest),
+            Err(e) => ecx.return_read_write_error(e, dest),
+        }
     }
 
     fn seek<'tcx>(

src/shims/unix/linux/eventfd.rs

@@ -127,8 +127,7 @@ impl FileDescription for Event {
         let ty = ecx.machine.layouts.u64;
         // Check the size of slice, and return error only if the size of the slice < 8.
         if len < ty.layout.size.bytes_usize() {
-            let result = Err(Error::from(ErrorKind::InvalidInput));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.return_read_write_error(Error::from(ErrorKind::InvalidInput), dest);
         }
 
         // Read the user supplied value from the pointer.
@@ -137,8 +136,7 @@ impl FileDescription for Event {
 
         // u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1.
         if num == u64::MAX {
-            let result = Err(Error::from(ErrorKind::InvalidInput));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.return_read_write_error(Error::from(ErrorKind::InvalidInput), dest);
         }
         // If the addition does not let the counter to exceed the maximum value, update the counter.
         // Else, block.
@@ -152,8 +150,7 @@ impl FileDescription for Event {
             }
             None | Some(u64::MAX) =>
                 if self.is_nonblock {
-                    let result = Err(Error::from(ErrorKind::WouldBlock));
-                    return ecx.return_written_byte_count_or_error(result, dest);
+                    return ecx.return_read_write_error(Error::from(ErrorKind::WouldBlock), dest);
                 } else {
                     throw_unsup_format!("eventfd: blocking is unsupported");
                 },

src/shims/unix/unnamed_socket.rs

@@ -137,8 +137,7 @@ impl FileDescription for AnonSocket {
 
         // Always succeed on read size 0.
         if len == 0 {
-            let result = Ok(0);
-            return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+            return ecx.return_read_write_success(Some(ptr), Some(&bytes), true, 0, dest);
         }
 
         let Some(readbuf) = &self.readbuf else {
@@ -151,17 +150,15 @@ impl FileDescription for AnonSocket {
             if self.peer_fd().upgrade().is_none() {
                 // Socketpair with no peer and empty buffer.
                 // 0 bytes successfully read indicates end-of-file.
-                let result = Ok(0);
-                return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+                return ecx.return_read_write_success(Some(ptr), Some(&bytes), true, 0, dest);
             } else {
                 if self.is_nonblock {
                     // Non-blocking socketpair with writer and empty buffer.
                     // https://linux.die.net/man/2/read
                     // EAGAIN or EWOULDBLOCK can be returned for socket,
                     // POSIX.1-2001 allows either error to be returned for this case.
                     // Since there is no ErrorKind for EAGAIN, WouldBlock is used.
-                    let result = Err(Error::from(ErrorKind::WouldBlock));
-                    return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+                    return ecx.return_read_write_error(Error::from(ErrorKind::WouldBlock), dest);
                 } else {
                     // Blocking socketpair with writer and empty buffer.
                     // FIXME: blocking is currently not supported
@@ -193,8 +190,7 @@ impl FileDescription for AnonSocket {
             ecx.check_and_update_readiness(&peer_fd)?;
         }
 
-        let result = Ok(actual_read_size);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        ecx.return_read_write_success(Some(ptr), Some(&bytes), true, actual_read_size, dest)
     }
 
     fn write<'tcx>(
@@ -209,16 +205,14 @@ impl FileDescription for AnonSocket {
         // Always succeed on write size 0.
         // ("If count is zero and fd refers to a file other than a regular file, the results are not specified.")
         if len == 0 {
-            let result = Ok(0);
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.return_read_write_success(None, None, false, 0, dest);
         }
 
         // We are writing to our peer's readbuf.
         let Some(peer_fd) = self.peer_fd().upgrade() else {
             // If the upgrade from Weak to Rc fails, it indicates that all read ends have been
             // closed.
-            let result = Err(Error::from(ErrorKind::BrokenPipe));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.return_read_write_error(Error::from(ErrorKind::BrokenPipe), dest);
         };
 
         let Some(writebuf) = &peer_fd.downcast::<AnonSocket>().unwrap().readbuf else {
@@ -232,8 +226,7 @@ impl FileDescription for AnonSocket {
         if available_space == 0 {
             if self.is_nonblock {
                 // Non-blocking socketpair with a full buffer.
-                let result = Err(Error::from(ErrorKind::WouldBlock));
-                return ecx.return_written_byte_count_or_error(result, dest);
+                return ecx.return_read_write_error(Error::from(ErrorKind::WouldBlock), dest);
             } else {
                 // Blocking socketpair with a full buffer.
                 throw_unsup_format!("socketpair write: blocking isn't supported yet");
@@ -255,8 +248,7 @@ impl FileDescription for AnonSocket {
         // The kernel does this even if the fd was already readable before, so we follow suit.
         ecx.check_and_update_readiness(&peer_fd)?;
 
-        let result = Ok(actual_write_size);
-        ecx.return_written_byte_count_or_error(result, dest)
+        ecx.return_read_write_success(None, None, false, actual_write_size, dest)
     }
 }