mv: keep copying xattrs after one fails on a cross-device move

src/uu/cp/src/cp.rs

@@ -87,6 +87,10 @@ pub enum CpError {
     #[error("{}", translate!("cp-error-not-all-files-copied"))]
     NotAllFilesCopied,
 
+    /// Xattr copying already reported each failure; only the exit code is needed.
+    #[error("")]
+    XattrErrorsReported,
+
     /// Simple [`walkdir::Error`] wrapper
     #[error("{0}")]
     WalkDirErr(#[from] walkdir::Error),
@@ -1423,9 +1427,12 @@ fn show_error_if_needed(error: &CpError) {
         CpError::NotAllFilesCopied => {
             // Need to return an error code
         }
-        CpError::Skipped(_) => {
+        CpError::Skipped(_) | CpError::XattrErrorsReported => {
             // touch a b && echo "n"|cp -i a b && echo $?
             // should return an error
+            // XattrErrorsReported: each failing attribute was already
+            // reported on stderr by `copy_xattrs*`; only the exit code
+            // matters now.
         }
         // Format IoErrContext using strip_errno to remove "(os error N)" suffix
         // for GNU-compatible output
@@ -1857,12 +1864,16 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe
         fs::set_permissions(dest, revert_perms)?;
     }
 
-    // If copying xattrs failed, propagate that error now with context.
+    // `copy_xattrs*` already reported each failure; add context only when xattrs are unsupported.
     copy_xattrs_result.map_err(|e| {
-        CpError::IoErrContext(
-            e,
-            translate!("cp-error-setting-attributes", "path" => dest.quote()),
-        )
+        if uucore::fsxattr::is_xattr_unsupported(&e) {
+            CpError::IoErrContext(
+                e,
+                translate!("cp-error-setting-attributes", "path" => dest.quote()),
+            )
+        } else {
+            CpError::XattrErrorsReported
+        }
     })?;
 
     Ok(())

src/uu/mv/src/mv.rs

@@ -1146,12 +1146,14 @@ fn rename_dir_fallback(
         display_manager,
     );
 
-    // Apply xattrs using a file descriptor to avoid TOCTOU races, ignoring
-    // ENOTSUP/EOPNOTSUPP (filesystem without xattr support, which is expected
-    // for cross-device moves).
+    // Apply xattrs using a file descriptor to avoid TOCTOU races.
     //
     // The fd is opened read-only: a directory cannot be opened for writing, and
     // fsetxattr checks write permission on the inode, not the open mode.
+//
+    // Per-attribute failures are reported by `apply_xattrs_fd_*` on stderr and
+    // must not fail the move. The source was already fully copied, so GNU mv
+    // completes the move and still exits 0.
     #[cfg(any(
         target_os = "freebsd",
         target_os = "hurd",
@@ -1162,7 +1164,7 @@ fn rename_dir_fallback(
     {
         use std::fs::File;
         let dest = File::open(to)?;
-        fsxattr::apply_xattrs_fd_ignore_unsupported(&dest, xattrs)?;
+        let _ = fsxattr::apply_xattrs_fd_ignore_unsupported(&dest, xattrs);
     }
 
     result?;

src/uucore/src/lib/features/fsxattr.rs

@@ -6,6 +6,9 @@
 // spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer
 
 //! Set of functions to manage xattr on files and dirs
+use crate::display::Quotable;
+use crate::error::strip_errno;
+use crate::show_error;
 use itertools::Itertools;
 use rustc_hash::FxHashMap;
 use std::ffi::{OsStr, OsString};
@@ -16,28 +19,63 @@ use std::path::Path;
 /// True if the error is `ENOTSUP` / `EOPNOTSUPP` (same errno on Linux,
 /// distinct on the BSDs).
 #[cfg(unix)]
-fn is_xattr_unsupported(err: &std::io::Error) -> bool {
+pub fn is_xattr_unsupported(err: &std::io::Error) -> bool {
     matches!(
         err.raw_os_error(),
         Some(e) if e == libc::ENOTSUP || e == libc::EOPNOTSUPP
     )
 }
 
 #[cfg(not(unix))]
-fn is_xattr_unsupported(_err: &std::io::Error) -> bool {
+pub fn is_xattr_unsupported(_err: &std::io::Error) -> bool {
     false
 }
 
+/// Report a per-attribute failure on stderr and record it so the copy loop
+/// can keep working on the remaining attributes and still fail afterwards.
+///
+/// `ENOTSUP` / `EOPNOTSUPP` mean the filesystem simply has no xattr support
+/// and are recorded but not reported: best-effort callers map them to `Ok`
+/// through the `*_ignore_unsupported` wrappers and must stay quiet.
+fn record_xattr_failure(
+    attr_name: &OsStr,
+    reading: bool,
+    err: std::io::Error,
+    pending_error: &mut Option<std::io::Error>,
+) {
+    if !is_xattr_unsupported(&err) {
+        let action = if reading {
+            "cannot read attribute"
+        } else {
+            "setting attribute"
+        };
+        show_error!("{action} {}: {}", attr_name.quote(), strip_errno(&err));
+    }
+    if pending_error.is_none() {
+        *pending_error = Some(err);
+    }
+}
+
 /// Copies extended attributes (xattrs) from one path to another.
-/// All errors propagate, including `ENOTSUP` / `EOPNOTSUPP`; for
+///
+/// A failed attribute is reported on stderr and does not stop the other
+/// attributes from being copied; the first such failure is propagated at
+/// the end. `ENOTSUP` / `EOPNOTSUPP` are recorded but not reported; for
 /// best-effort callers see [`copy_xattrs_ignore_unsupported`].
 pub fn copy_xattrs<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
+    let mut pending_error = None;
     for attr_name in xattr::list(&source)? {
-        if let Some(value) = xattr::get(&source, &attr_name)? {
-            xattr::set(&dest, &attr_name, &value)?;
+        match xattr::get(&source, &attr_name) {
+            Ok(Some(value)) => {
+                if let Err(err) = xattr::set(&dest, &attr_name, &value) {
+                    record_xattr_failure(&attr_name, false, err, &mut pending_error);
+                }
+            }
+            Ok(None) => {}
+            Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error),
         }
     }
-    Ok(())
+    pending_error.map_or(Ok(()), Err)
 }
 
 /// Like [`copy_xattrs`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`
@@ -52,15 +90,25 @@ pub fn copy_xattrs_ignore_unsupported<P: AsRef<Path>>(source: P, dest: P) -> std
 /// Copies xattrs between two open file descriptors. Pins both inodes so
 /// list/get/set calls cannot be redirected by a concurrent renamer, unlike
 /// the path-based [`copy_xattrs`].
+///
+/// Failures are handled like in [`copy_xattrs`]: each one is reported and
+/// the remaining attributes are still copied.
 #[cfg(unix)]
 pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> {
     use xattr::FileExt;
+    let mut pending_error = None;
     for attr_name in source.list_xattr()? {
-        if let Some(value) = source.get_xattr(&attr_name)? {
-            dest.set_xattr(&attr_name, &value)?;
+        match source.get_xattr(&attr_name) {
+            Ok(Some(value)) => {
+                if let Err(err) = dest.set_xattr(&attr_name, &value) {
+                    record_xattr_failure(&attr_name, false, err, &mut pending_error);
+                }
+            }
+            Ok(None) => {}
+            Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error),
         }
     }
-    Ok(())
+    pending_error.map_or(Ok(()), Err)
 }
 
 /// Like [`copy_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`.
@@ -76,16 +124,27 @@ pub fn copy_xattrs_fd_ignore_unsupported(
 }
 
 /// Like `copy_xattrs`, but skips the security.selinux attribute.
+///
+/// Failures are handled like in [`copy_xattrs`]: each one is reported and
+/// the remaining attributes are still copied.
 #[cfg(unix)]
 pub fn copy_xattrs_skip_selinux<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
+    let mut pending_error = None;
     for attr_name in xattr::list(&source)? {
-        if attr_name.as_bytes() != b"security.selinux"
-            && let Some(value) = xattr::get(&source, &attr_name)?
-        {
-            xattr::set(&dest, &attr_name, &value)?;
+        if attr_name.as_bytes() == b"security.selinux" {
+            continue;
+        }
+        match xattr::get(&source, &attr_name) {
+            Ok(Some(value)) => {
+                if let Err(err) = xattr::set(&dest, &attr_name, &value) {
+                    record_xattr_failure(&attr_name, false, err, &mut pending_error);
+                }
+            }
+            Ok(None) => {}
+            Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error),
         }
     }
-    Ok(())
+    pending_error.map_or(Ok(()), Err)
 }
 
 /// Copies only the POSIX ACL xattrs (`system.posix_acl_access` and
@@ -155,6 +214,9 @@ pub fn retrieve_xattrs_fd(source: &std::fs::File) -> std::io::Result<FxHashMap<O
 
 /// Applies extended attributes (xattrs) to a given file or directory.
 ///
+/// Failures are handled like in [`copy_xattrs`]: each one is reported and
+/// the remaining attributes are still applied.
+///
 /// # Arguments
 ///
 /// * `dest` - A reference to the path of the file or directory.
@@ -167,16 +229,19 @@ pub fn apply_xattrs<P: AsRef<Path>>(
     dest: P,
     xattrs: FxHashMap<OsString, Vec<u8>>,
 ) -> std::io::Result<()> {
+    let mut pending_error = None;
     for (attr, value) in xattrs {
-        xattr::set(&dest, &attr, &value)?;
+        if let Err(err) = xattr::set(&dest, &attr, &value) {
+            record_xattr_failure(&attr, false, err, &mut pending_error);
+        }
     }
-    Ok(())
+    pending_error.map_or(Ok(()), Err)
 }
 
 /// Applies extended attributes (xattrs) to a given file using a file descriptor.
 ///
-/// This version avoids TOCTOU races by operating on an open file descriptor
-/// rather than a path, ensuring all operations target the same inode.
+/// Failures are handled like in [`copy_xattrs`]: each one is reported and
+/// the remaining attributes are still applied.
 ///
 /// # Arguments
 ///
@@ -192,10 +257,13 @@ pub fn apply_xattrs_fd(
     xattrs: FxHashMap<OsString, Vec<u8>>,
 ) -> std::io::Result<()> {
     use xattr::FileExt;
+    let mut pending_error = None;
     for (attr, value) in xattrs {
-        dest.set_xattr(&attr, &value)?;
+        if let Err(err) = dest.set_xattr(&attr, &value) {
+            record_xattr_failure(&attr, false, err, &mut pending_error);
+        }
     }
-    Ok(())
+    pending_error.map_or(Ok(()), Err)
 }
 
 /// Like [`apply_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`.
@@ -369,6 +437,81 @@ mod tests {
         assert_eq!(copied, test_value);
     }
 
+    #[test]
+    #[cfg(target_os = "linux")]
+    fn test_copy_xattrs_continues_after_failure() {
+        use std::path::PathBuf;
+        use std::process::Command;
+
+        // tmpfs accepts large user-xattr values while most disk filesystems
+        // cap them near the block size. Put the source on /dev/shm and the
+        // destination on the build filesystem so the first attribute fails to
+        // copy while the source holds it fine; skip when this machine cannot
+        // produce that layout.
+        let pid = std::process::id();
+        let source_dir = PathBuf::from(format!("/dev/shm/xattr_copy_fail_{pid}"));
+        let dest_dir = std::env::temp_dir().join(format!("xattr_copy_fail_{pid}"));
+        if std::fs::create_dir(&source_dir).is_err() || std::fs::create_dir(&dest_dir).is_err() {
+            return; // skip: no usable /dev/shm or temp dir
+        }
+
+        let mut usable_size = None;
+        for size in [9_100, 40_000] {
+            let value = "y".repeat(size);
+            let source_probe = source_dir.join(format!("probe_{size}"));
+            let dest_probe = dest_dir.join(format!("probe_{size}"));
+            std::fs::write(&source_probe, "x").ok();
+            std::fs::write(&dest_probe, "x").ok();
+            let src_accepts = Command::new("setfattr")
+                .args(["-n", "user.huge", "-v", &value])
+                .arg(&source_probe)
+                .status()
+                .is_ok_and(|s| s.success());
+            let dest_rejects = !Command::new("setfattr")
+                .args(["-n", "user.huge", "-v", &value])
+                .arg(&dest_probe)
+                .status()
+                .is_ok_and(|s| s.success());
+            std::fs::remove_file(&source_probe).ok();
+            std::fs::remove_file(&dest_probe).ok();
+            if src_accepts && dest_rejects {
+                usable_size = Some(size);
+                break;
+            }
+        }
+        let Some(size) = usable_size else {
+            std::fs::remove_dir_all(&source_dir).ok();
+            std::fs::remove_dir_all(&dest_dir).ok();
+            return; // skip: this filesystem combination cannot fail the copy
+        };
+
+        // Set small attributes around the failing big attribute so that
+        // regardless of filesystem listing order (alphabetical, insertion,
+        // or reverse-insertion), at least one surviving attribute is
+        // processed after the failing one.
+        let source = source_dir.join("source");
+        let dest = dest_dir.join("dest");
+        std::fs::write(&source, "data").unwrap();
+        std::fs::write(&dest, "data").unwrap();
+        let big_value = "y".repeat(size);
+        xattr::set(&source, "user.a_small", b"12345678").unwrap();
+        xattr::set(&source, "user.m_big", big_value.as_bytes()).unwrap();
+        xattr::set(&source, "user.z_small", b"87654321").unwrap();
+
+        let result = copy_xattrs(&source, &dest);
+        assert!(result.is_err(), "the failed attribute must fail the copy");
+
+        let copied_a = xattr::get(&dest, "user.a_small").unwrap();
+        assert_eq!(copied_a.as_deref(), Some(b"12345678".as_slice()));
+        let copied_z = xattr::get(&dest, "user.z_small").unwrap();
+        assert_eq!(copied_z.as_deref(), Some(b"87654321".as_slice()));
+        let copied_big = xattr::get(&dest, "user.m_big").unwrap();
+        assert_eq!(copied_big, None);
+
+        std::fs::remove_dir_all(&source_dir).ok();
+        std::fs::remove_dir_all(&dest_dir).ok();
+    }
+
     #[test]
     fn test_apply_and_retrieve_xattrs() {
         let temp_dir = tempdir().unwrap();

tests/by-util/test_cp.rs

@@ -9444,7 +9444,7 @@ fn test_cp_xattr_failure_keeps_dest_contents() {
         .arg(&source)
         .arg(&out)
         .fails()
-        .stderr_contains("setting attributes");
+        .stderr_contains("setting attribute 'user.huge'");
     assert_eq!(std_fs::read_to_string(&out).unwrap(), "kept content");
 
     // A read-only source propagates its mode to the destination; the failure
@@ -9457,7 +9457,7 @@ fn test_cp_xattr_failure_keeps_dest_contents() {
         .arg(&source)
         .arg(&out_ro)
         .fails()
-        .stderr_contains("setting attributes");
+        .stderr_contains("setting attribute 'user.huge'");
     assert_eq!(std_fs::read_to_string(&out_ro).unwrap(), "kept content");
     assert_eq!(
         std_fs::metadata(&out_ro).unwrap().mode() & 0o777,