split: fix I/O error handling for device full conditions

src/uu/df/src/df.rs

@@ -22,7 +22,7 @@ use clap::{Arg, ArgAction, ArgMatches, Command, parser::ValueSource};
 
 use std::ffi::OsString;
 use std::io::{BufWriter, Write, stdout};
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use thiserror::Error;
 
 use crate::blocks::{BlockSize, read_block_size};
@@ -416,10 +416,11 @@ impl UError for DfError {
 }
 
 /// Gather the filesystems that `df` would report on, without formatting anything.
-pub fn filesystems<P>(paths: Option<&[P]>, opt: &Options) -> UResult<Vec<Filesystem>>
-where
-    P: AsRef<Path>,
-{
+///
+/// `paths` is `None` to report on every mounted filesystem, as `df` does when
+/// called without operands, or `Some` to report on the filesystems containing
+/// the given paths.
+pub fn filesystems(paths: Option<&[&Path]>, opt: &Options) -> UResult<Vec<Filesystem>> {
     // Run a sync call before any operation if so instructed.
     if opt.sync {
         #[cfg(not(any(windows, target_os = "redox")))]
@@ -458,10 +459,9 @@ where
 }
 
 /// Display filesystem usage information as a table on stdout.
-pub fn df<P>(paths: Option<&[P]>, opt: &Options) -> UResult<()>
-where
-    P: AsRef<Path>,
-{
+///
+/// `paths` has the same meaning as in [`filesystems`].
+pub fn df(paths: Option<&[&Path]>, opt: &Options) -> UResult<()> {
     let filesystems = filesystems(paths, opt)?;
 
     // Every path given on the command line was rejected; `filesystems` has
@@ -495,9 +495,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     }
 
     let opt = Options::from_matches(&matches)?;
-    let paths: Option<Vec<PathBuf>> = matches
+    let paths: Option<Vec<&Path>> = matches
         .get_many::<OsString>(OPT_PATHS)
-        .map(|paths| paths.map(PathBuf::from).collect());
+        .map(|paths| paths.map(Path::new).collect());
 
     df(paths.as_deref(), &opt)
 }
@@ -656,10 +656,20 @@ mod tests {
     // spell-checker:ignore apfs
     mod write_table {
 
+        use crate::blocks::BlockSize;
         use crate::{Filesystem, Options, write_table};
         use std::ffi::OsString;
         use uucore::fsext::{FsUsage, MountInfo};
 
+        /// `Options::default()` picks up the block size from the environment
+        /// (`POSIXLY_CORRECT` halves it), so pin it for reproducible output.
+        fn options() -> Options {
+            Options {
+                block_size: BlockSize::Bytes(1024),
+                ..Options::default()
+            }
+        }
+
         fn filesystem() -> Filesystem {
             Filesystem {
                 file: Some(OsString::from("/tmp/file")),
@@ -690,7 +700,7 @@ mod tests {
         #[test]
         fn test_write_table_to_arbitrary_writer() {
             let mut buffer: Vec<u8> = Vec::new();
-            write_table(&mut buffer, vec![filesystem()], &Options::default()).unwrap();
+            write_table(&mut buffer, vec![filesystem()], &options()).unwrap();
 
             // Header text is localized and the bundle is not loaded in unit
             // tests, so only the data row is asserted on here.
@@ -710,7 +720,7 @@ mod tests {
         #[test]
         fn test_write_table_empty() {
             let mut buffer: Vec<u8> = Vec::new();
-            write_table(&mut buffer, vec![], &Options::default()).unwrap();
+            write_table(&mut buffer, vec![], &options()).unwrap();
 
             let table = String::from_utf8(buffer).unwrap();
             assert_eq!(table.lines().count(), 1);

src/uu/split/src/split.rs

@@ -26,7 +26,7 @@ use std::io::{BufRead, BufReader, ErrorKind, Read, Seek, SeekFrom, Write, stdin}
 use std::path::Path;
 use thiserror::Error;
 use uucore::display::Quotable;
-use uucore::error::{FromIo, UResult, USimpleError, UUsageError};
+use uucore::error::{FromIo, UResult, USimpleError, UUsageError, set_exit_code, strip_errno};
 use uucore::parser::parse_size::parse_size_u64;
 use uucore::translate;
 
@@ -521,6 +521,12 @@ struct ByteChunkWriter<'a> {
 
     /// Iterator that yields filenames for each chunk.
     filename_iterator: FilenameIterator<'a>,
+
+    /// Current filename being written to.
+    current_filename: OsString,
+
+    /// Whether an error has already been reported for the current file.
+    error_reported: bool,
 }
 
 impl<'a> ByteChunkWriter<'a> {
@@ -540,8 +546,45 @@ impl<'a> ByteChunkWriter<'a> {
             num_chunks_written: 0,
             inner,
             filename_iterator,
+            current_filename: filename,
+            error_reported: false,
         })
     }
+
+    /// Report a write error on the current chunk, at most once per chunk.
+    ///
+    /// Returns an empty error: the message is already out, so the caller only
+    /// has to stop, without printing anything else.
+    fn report_error(&mut self, e: &io::Error) -> io::Error {
+        if !self.error_reported {
+            uucore::show_error!("{}: {}", self.current_filename.display(), strip_errno(e));
+            set_exit_code(1);
+            self.error_reported = true;
+        }
+        io::Error::other("")
+    }
+
+    /// Write to the current chunk, flushing right away.
+    ///
+    /// Without the flush, a write failure on a full device would only surface
+    /// when the buffer happens to be flushed, by which point we have moved on
+    /// to another chunk and would blame the wrong file.
+    fn write_chunk(&mut self, bytes: &[u8]) -> io::Result<usize> {
+        let written = custom_write(bytes, &mut self.inner, self.settings)
+            .and_then(|n| self.inner.flush().map(|()| n));
+        match written {
+            Err(e) if ignorable_io_error(&e, self.settings) => Ok(bytes.len()),
+            Err(e) => Err(self.report_error(&e)),
+            ok => ok,
+        }
+    }
+}
+
+impl Drop for ByteChunkWriter<'_> {
+    fn drop(&mut self) {
+        // Safety net: a buffered error must not be lost when the writer goes away.
+        let _ = self.inner.flush().map_err(|e| self.report_error(&e));
+    }
 }
 
 impl Write for ByteChunkWriter<'_> {
@@ -555,6 +598,10 @@ impl Write for ByteChunkWriter<'_> {
         let mut carryover_bytes_written: usize = 0;
         while !buf.is_empty() {
             if self.num_bytes_remaining_in_current_chunk == 0 {
+                // Flush before switching file, so a delayed write error is
+                // still attributed to the chunk it belongs to.
+                self.inner.flush().map_err(|e| self.report_error(&e))?;
+
                 // Increment the chunk number, reset the number of bytes remaining, and instantiate the new underlying writer.
                 self.num_chunks_written += 1;
                 self.num_bytes_remaining_in_current_chunk = self.chunk_size;
@@ -567,6 +614,8 @@ impl Write for ByteChunkWriter<'_> {
                     writeln!(io::stdout(), "creating file {}", filename.quote())?;
                 }
                 self.inner = self.settings.instantiate_current_writer(&filename, true)?;
+                self.current_filename = filename;
+                self.error_reported = false;
             }
 
             // If the capacity of this chunk is greater than the number of
@@ -575,7 +624,7 @@ impl Write for ByteChunkWriter<'_> {
             // the chunk number and repeat.
             let buf_len = buf.len();
             if (buf_len as u64) < self.num_bytes_remaining_in_current_chunk {
-                let num_bytes_written = custom_write(buf, &mut self.inner, self.settings)?;
+                let num_bytes_written = self.write_chunk(buf)?;
                 self.num_bytes_remaining_in_current_chunk -= num_bytes_written as u64;
                 return Ok(carryover_bytes_written + num_bytes_written);
             }
@@ -586,7 +635,7 @@ impl Write for ByteChunkWriter<'_> {
             // self.num_bytes_remaining_in_current_chunk is lower than
             // n, which is already usize.
             let i = self.num_bytes_remaining_in_current_chunk as usize;
-            let num_bytes_written = custom_write(&buf[..i], &mut self.inner, self.settings)?;
+            let num_bytes_written = self.write_chunk(&buf[..i])?;
             self.num_bytes_remaining_in_current_chunk -= num_bytes_written as u64;
 
             // It's possible that the underlying writer did not
@@ -604,7 +653,7 @@ impl Write for ByteChunkWriter<'_> {
         Ok(carryover_bytes_written)
     }
     fn flush(&mut self) -> io::Result<()> {
-        self.inner.flush()
+        self.inner.flush().map_err(|e| self.report_error(&e))
     }
 }
 
@@ -1359,8 +1408,15 @@ fn split(settings: &Settings) -> UResult<()> {
         Strategy::Bytes(chunk_size) => {
             let mut writer = ByteChunkWriter::new(chunk_size, settings)?;
             // todo: distinct read error and write error
-            io::copy(&mut reader, &mut writer)?;
-            Ok(())
+            match io::copy(&mut reader, &mut writer) {
+                Ok(_) => Ok(()),
+                // The writer already reported the write error and set the exit
+                // code, and signals that with an empty error: stay quiet here.
+                Err(e) if e.kind() == ErrorKind::Other && e.to_string().is_empty() => {
+                    Err(USimpleError::new(1, ""))
+                }
+                Err(e) => Err(e.into()),
+            }
         }
         Strategy::LineBytes(chunk_size) => {
             line_bytes(settings, &mut reader, chunk_size as usize, io_blksize)

tests/by-util/test_split.rs

@@ -2115,3 +2115,26 @@ fn test_io_error() {
         //todo: add file path with proper distinction of input/output
         .stderr_contains("Input/output error\n");
 }
+
+/// Writing a chunk to a full device must be reported and must stop the split.
+#[test]
+#[cfg(target_os = "linux")]
+fn test_write_error_on_full_device() {
+    if !Path::new("/dev/full").exists() {
+        return;
+    }
+
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // The first chunk lands on /dev/full, so its write can never succeed.
+    at.symlink_file("/dev/full", "xaa");
+    at.write("input", "uv");
+
+    ucmd.args(&["-b", "1", "input"])
+        .fails_with_code(1)
+        .no_stdout()
+        .stderr_contains("split: xaa: No space left on device");
+
+    // split must not have moved on to the next chunk.
+    assert!(!at.file_exists("xab"));
+}