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

src/uu/cp/src/cp.rs

@@ -1847,6 +1847,9 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe
     } else {
         copy_xattrs(source, dest)
     };
+    // Every attribute has been tried; report the first one that failed.
+    let copy_xattrs_result = copy_xattrs_result
+        .and_then(|failed| failed.into_iter().next().map_or(Ok(()), |(_, e)| Err(e)));
 
     // Restore read-only if we changed it.
     if was_readonly {

src/uu/mv/locales/en-US.ftl

@@ -38,6 +38,7 @@ mv-error-dangling-symlink = can't determine symlink type, since it is dangling
 mv-error-no-symlink-support = your operating system does not support symlinks
 mv-error-permission-denied = Permission denied
 mv-error-inter-device-move-failed = inter-device move failed: {$from} to {$to}; unable to remove target: {$err}
+mv-error-setting-attribute = setting attribute {$name}: {$err}
 mv-error-exchange-two-operands = --exchange requires exactly two operands
 mv-error-exchange-not-supported = --exchange is not supported on this platform
 

src/uu/mv/locales/fr-FR.ftl

@@ -38,6 +38,7 @@ mv-error-dangling-symlink = impossible de déterminer le type de lien symbolique
 mv-error-no-symlink-support = votre système d'exploitation ne prend pas en charge les liens symboliques
 mv-error-permission-denied = Permission refusée
 mv-error-inter-device-move-failed = échec du déplacement inter-périphérique : {$from} vers {$to} ; impossible de supprimer la cible : {$err}
+mv-error-setting-attribute = définition de l'attribut {$name} : {$err}
 mv-error-exchange-two-operands = --exchange nécessite exactement deux opérandes
 mv-error-exchange-not-supported = --exchange n'est pas pris en charge sur cette plateforme
 

src/uu/mv/src/mv.rs

@@ -1051,7 +1051,9 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
         target_os = "netbsd"
     ))]
     {
-        let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to);
+        if let Ok(failed) = fsxattr::copy_xattrs_ignore_unsupported(from, to) {
+            show_xattr_failures(failed);
+        }
     }
     let _ = preserve_ownership(from, to);
     fs::remove_file(from)
@@ -1384,7 +1386,9 @@ fn copy_file_with_hardlinks_helper(
             target_os = "netbsd"
         ))]
         {
-            let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to);
+            if let Ok(failed) = fsxattr::copy_xattrs_ignore_unsupported(from, to) {
+                show_xattr_failures(failed);
+            }
         }
         // Preserve ownership (uid/gid) from the source
         let _ = preserve_ownership(from, to);
@@ -1455,7 +1459,9 @@ fn rename_file_fallback(
             target_os = "netbsd"
         ))]
         {
-            let _ = fsxattr::copy_xattrs_fd_ignore_unsupported(&src_file, &dst_file);
+            if let Ok(failed) = fsxattr::copy_xattrs_fd_ignore_unsupported(&src_file, &dst_file) {
+                show_xattr_failures(failed);
+            }
         }
 
         // chown before chmod: chown(2) clears setuid/setgid for non-root,
@@ -1485,6 +1491,31 @@ fn rename_file_fallback(
     Ok(())
 }
 
+/// Report each xattr that a cross-device move could not copy. Like GNU, these
+/// are only warnings: the move itself still succeeds.
+#[cfg(any(
+    target_os = "freebsd",
+    target_os = "hurd",
+    target_os = "linux",
+    target_os = "android",
+    target_os = "netbsd"
+))]
+fn show_xattr_failures(failed: Vec<(OsString, io::Error)>) {
+    use uucore::error::strip_errno;
+    use uucore::show_error;
+
+    for (name, err) in failed {
+        show_error!(
+            "{}",
+            translate!(
+                "mv-error-setting-attribute",
+                "name" => name.quote(),
+                "err" => strip_errno(&err)
+            )
+        );
+    }
+}
+
 /// Preserve ownership (uid/gid) from source to destination.
 /// Uses lchown so it works on symlinks without following them.
 /// Errors are silently ignored for non-root users who cannot chown.

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

@@ -29,64 +29,100 @@ fn is_xattr_unsupported(_err: &std::io::Error) -> bool {
     false
 }
 
-/// Copies extended attributes (xattrs) from one path to another.
-/// All errors propagate, including `ENOTSUP` / `EOPNOTSUPP`; for
-/// best-effort callers see [`copy_xattrs_ignore_unsupported`].
-pub fn copy_xattrs<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
-    for attr_name in xattr::list(&source)? {
-        if let Some(value) = xattr::get(&source, &attr_name)? {
-            xattr::set(&dest, &attr_name, &value)?;
+/// The result of copying xattrs: each attribute that could not be copied,
+/// along with its error. Only failing to list the attributes of the source
+/// is an `Err`.
+pub type XattrCopyResult = std::io::Result<Vec<(OsString, std::io::Error)>>;
+
+/// Leaves `ENOTSUP` / `EOPNOTSUPP` out of the result of copying xattrs, for
+/// callers where xattr preservation is best-effort.
+fn without_unsupported(result: XattrCopyResult) -> XattrCopyResult {
+    match result {
+        Ok(mut failed) => {
+            failed.retain(|(_, e)| !is_xattr_unsupported(e));
+            Ok(failed)
         }
+        Err(e) if is_xattr_unsupported(&e) => Ok(Vec::new()),
+        Err(e) => Err(e),
     }
-    Ok(())
 }
 
-/// Like [`copy_xattrs`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`
-/// for callers where xattr preservation is best-effort.
-pub fn copy_xattrs_ignore_unsupported<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
-    match copy_xattrs(source, dest) {
-        Err(e) if is_xattr_unsupported(&e) => Ok(()),
-        res => res,
+/// Copies each attribute in `names` that `keep` accepts, reading its value
+/// with `get` and writing it with `set`. An attribute that cannot be copied
+/// does not stop the others; it is returned along with its error.
+fn copy_each_xattr(
+    names: impl IntoIterator<Item = OsString>,
+    keep: impl Fn(&OsStr) -> bool,
+    get: impl Fn(&OsStr) -> std::io::Result<Option<Vec<u8>>>,
+    set: impl Fn(&OsStr, &[u8]) -> std::io::Result<()>,
+) -> Vec<(OsString, std::io::Error)> {
+    let mut failed = Vec::new();
+    for attr_name in names.into_iter().filter(|name| keep(name)) {
+        let result = get(&attr_name).and_then(|value| match value {
+            Some(value) => set(&attr_name, &value),
+            None => Ok(()),
+        });
+        if let Err(e) = result {
+            failed.push((attr_name, e));
+        }
     }
+    failed
+}
+
+/// Copies extended attributes (xattrs) from one path to another.
+///
+/// An attribute that cannot be copied does not stop the others from being
+/// copied. Returns each attribute that failed along with its error, including
+/// `ENOTSUP` / `EOPNOTSUPP`; for best-effort callers see
+/// [`copy_xattrs_ignore_unsupported`]. Only failing to list the attributes of
+/// `source` is an `Err`.
+pub fn copy_xattrs<P: AsRef<Path>>(source: P, dest: P) -> XattrCopyResult {
+    Ok(copy_each_xattr(
+        xattr::list(&source)?,
+        |_| true,
+        |name| xattr::get(&source, name),
+        |name, value| xattr::set(&dest, name, value),
+    ))
+}
+
+/// Like [`copy_xattrs`], but leaves out `ENOTSUP` / `EOPNOTSUPP` for callers
+/// where xattr preservation is best-effort.
+pub fn copy_xattrs_ignore_unsupported<P: AsRef<Path>>(source: P, dest: P) -> XattrCopyResult {
+    without_unsupported(copy_xattrs(source, dest))
 }
 
 /// 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`].
+/// the path-based [`copy_xattrs`], and reports failures the same way.
 #[cfg(unix)]
-pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> {
+pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> XattrCopyResult {
     use xattr::FileExt;
-    for attr_name in source.list_xattr()? {
-        if let Some(value) = source.get_xattr(&attr_name)? {
-            dest.set_xattr(&attr_name, &value)?;
-        }
-    }
-    Ok(())
+    Ok(copy_each_xattr(
+        source.list_xattr()?,
+        |_| true,
+        |name| source.get_xattr(name),
+        |name, value| dest.set_xattr(name, value),
+    ))
 }
 
-/// Like [`copy_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`.
+/// Like [`copy_xattrs_fd`], but leaves out `ENOTSUP` / `EOPNOTSUPP`.
 #[cfg(unix)]
 pub fn copy_xattrs_fd_ignore_unsupported(
     source: &std::fs::File,
     dest: &std::fs::File,
-) -> std::io::Result<()> {
-    match copy_xattrs_fd(source, dest) {
-        Err(e) if is_xattr_unsupported(&e) => Ok(()),
-        res => res,
-    }
+) -> XattrCopyResult {
+    without_unsupported(copy_xattrs_fd(source, dest))
 }
 
 /// Like `copy_xattrs`, but skips the security.selinux attribute.
 #[cfg(unix)]
-pub fn copy_xattrs_skip_selinux<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
-    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)?;
-        }
-    }
-    Ok(())
+pub fn copy_xattrs_skip_selinux<P: AsRef<Path>>(source: P, dest: P) -> XattrCopyResult {
+    Ok(copy_each_xattr(
+        xattr::list(&source)?,
+        |name| name.as_bytes() != b"security.selinux",
+        |name| xattr::get(&source, name),
+        |name, value| xattr::set(&dest, name, value),
+    ))
 }
 
 /// Copies only the POSIX ACL xattrs (`system.posix_acl_access` and
@@ -335,7 +371,7 @@ mod tests {
         let test_value = b"test value";
         xattr::set(&source_path, test_attr, test_value).unwrap();
 
-        copy_xattrs(&source_path, &dest_path).unwrap();
+        assert!(copy_xattrs(&source_path, &dest_path).unwrap().is_empty());
 
         let copied_value = xattr::get(&dest_path, test_attr).unwrap().unwrap();
         assert_eq!(copied_value, test_value);
@@ -364,7 +400,7 @@ mod tests {
             .open(&dest_path)
             .unwrap();
 
-        copy_xattrs_fd(&src, &dst).unwrap();
+        assert!(copy_xattrs_fd(&src, &dst).unwrap().is_empty());
 
         let copied = xattr::get(&dest_path, test_attr).unwrap().unwrap();
         assert_eq!(copied, test_value);

tests/by-util/test_cp.rs

@@ -9605,6 +9605,76 @@ fn test_cp_xattr_failure_keeps_dest_contents() {
     std_fs::remove_dir_all(&dest_dir).ok();
 }
 
+/// An xattr the destination refuses must not stop the other xattrs from being
+/// copied. cp still reports the failure and exits 1. tmpfs takes large values
+/// while ext4 caps a value at one block, so the large attributes fail there
+/// and the small ones must survive.
+#[test]
+#[cfg(target_os = "linux")]
+#[cfg_attr(
+    wasi_runner,
+    ignore = "WASI sandbox: host paths (/dev/shm) not visible"
+)]
+fn test_cp_preserve_xattr_failure_keeps_the_rest() {
+    use rustc_hash::FxHashMap;
+    use std::ffi::OsStr;
+    use tempfile::TempDir;
+    use uucore::fsxattr::{apply_xattrs, retrieve_xattrs};
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let too_big = vec![b'x'; 8000];
+    at.touch("probe");
+    let probe = FxHashMap::from_iter([(OsString::from("user.probe"), too_big.clone())]);
+    if apply_xattrs(at.plus("probe"), probe).is_ok() {
+        println!("test skipped: the destination filesystem accepts large xattr values");
+        return;
+    }
+
+    // tmpfs lists attributes sorted by name, so interleaving the names puts a
+    // refused attribute before a kept one whichever way the list is sorted.
+    let attrs: FxHashMap<OsString, Vec<u8>> = [
+        ("user.a_kept", b"first".to_vec()),
+        ("user.b_too_big", too_big.clone()),
+        ("user.c_kept", b"middle".to_vec()),
+        ("user.d_too_big", too_big),
+        ("user.e_kept", b"last".to_vec()),
+    ]
+    .into_iter()
+    .map(|(name, value)| (OsString::from(name), value))
+    .collect();
+
+    let src_dir =
+        TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
+    let file = src_dir.path().join("file");
+    let dir = src_dir.path().join("dir");
+    std_fs::write(&file, "content").unwrap();
+    std_fs::create_dir(&dir).unwrap();
+    if apply_xattrs(&file, attrs.clone()).is_err() {
+        println!("test skipped: /dev/shm does not accept user xattrs");
+        return;
+    }
+    apply_xattrs(&dir, attrs.clone()).unwrap();
+
+    // A regular file is copied through file descriptors, a directory by path.
+    for (src, dest) in [(&file, "file"), (&dir, "dir")] {
+        scene
+            .ucmd()
+            .args(&["-r", "--preserve=xattr"])
+            .arg(src)
+            .arg(dest)
+            .fails_with_code(1)
+            .stderr_contains(format!("cp: setting attributes for '{dest}"));
+
+        let copied = retrieve_xattrs(at.plus(dest)).unwrap();
+        for name in ["user.a_kept", "user.c_kept", "user.e_kept"] {
+            let name = OsStr::new(name);
+            assert_eq!(copied.get(name), attrs.get(name), "{name:?} on {dest}");
+        }
+    }
+}
+
 #[test]
 #[cfg(not(windows))]
 #[cfg_attr(

tests/by-util/test_mv.rs

@@ -3257,6 +3257,74 @@ fn test_mv_cross_device_dir_xattr_preserved() {
     assert_eq!(out.stdout, b"dirvalue");
 }
 
+/// An xattr the destination refuses must not stop the other xattrs from being
+/// copied in a cross-device move. Each refused one is reported, but the move
+/// still succeeds. tmpfs takes large values while ext4 caps a value at one
+/// block, so the large attributes fail there and the small ones must survive.
+#[test]
+#[cfg(target_os = "linux")]
+fn test_mv_cross_device_xattr_failure_keeps_the_rest() {
+    use rustc_hash::FxHashMap;
+    use std::ffi::{OsStr, OsString};
+    use tempfile::TempDir;
+    use uucore::fsxattr::{apply_xattrs, retrieve_xattrs};
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let too_big = vec![b'x'; 8000];
+    at.touch("probe");
+    let probe = FxHashMap::from_iter([(OsString::from("user.probe"), too_big.clone())]);
+    if apply_xattrs(at.plus("probe"), probe).is_ok() {
+        println!("test skipped: the destination filesystem accepts large xattr values");
+        return;
+    }
+
+    // tmpfs lists attributes sorted by name, so interleaving the names puts a
+    // refused attribute before a kept one whichever way the list is sorted.
+    let attrs: FxHashMap<OsString, Vec<u8>> = [
+        ("user.a_kept", b"first".to_vec()),
+        ("user.b_too_big", too_big.clone()),
+        ("user.c_kept", b"middle".to_vec()),
+        ("user.d_too_big", too_big),
+        ("user.e_kept", b"last".to_vec()),
+    ]
+    .into_iter()
+    .map(|(name, value)| (OsString::from(name), value))
+    .collect();
+
+    let src_dir =
+        TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
+    let file = src_dir.path().join("file");
+    let dir = src_dir.path().join("dir");
+    std::fs::write(&file, "content").unwrap();
+    std::fs::create_dir(&dir).unwrap();
+    std::fs::write(dir.join("file"), "content").unwrap();
+    if apply_xattrs(&file, attrs.clone()).is_err() {
+        println!("test skipped: /dev/shm does not accept user xattrs");
+        return;
+    }
+    apply_xattrs(dir.join("file"), attrs.clone()).unwrap();
+
+    // A single file, then a file copied as part of a directory.
+    for (src, dest, moved) in [(&file, "file", "file"), (&dir, "dir", "dir/file")] {
+        scene
+            .ucmd()
+            .arg(src)
+            .arg(dest)
+            .succeeds()
+            .stderr_contains("mv: setting attribute 'user.b_too_big': ")
+            .stderr_contains("mv: setting attribute 'user.d_too_big': ");
+
+        assert!(!src.exists());
+        let copied = retrieve_xattrs(at.plus(moved)).unwrap();
+        for name in ["user.a_kept", "user.c_kept", "user.e_kept"] {
+            let name = OsStr::new(name);
+            assert_eq!(copied.get(name), attrs.get(name), "{name:?} on {moved}");
+        }
+    }
+}
+
 /// Cross-device mv of a symlink onto an existing file must replace the
 /// destination atomically, matching GNU.
 #[test]