8000 Make mv command fallback to copy only if the src and dst are on different device by hamflx · Pull Request #6040 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make mv command fallback to copy only if the src and dst are on different device #6040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .vscode/cspell.dictionaries/workspace.wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ vmsplice

# * vars/libc
COMFOLLOW
EXDEV
FILENO
FTSENT
HOSTSIZE
Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/uu/mv/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ uucore = { workspace = true, features = [
] }
thiserror = { workspace = true }

[target.'cfg(windows)'.dependencies]
windows-sys = { workspace = true, features = [
"Win32_Foundation",
"Win32_Security",
"Win32_Storage_FileSystem",
] }

[target.'cfg(unix)'.dependencies]
libc = { workspace = true }

[[bin]]
name = "mv"
path = "src/main.rs"
69 changes: 68 additions & 1 deletion src/uu/mv/src/mv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,22 @@ fn rename_with_fallback(
to: &Path,
multi_progress: Option<&MultiProgress>,
) -> io::Result<()> {
if fs::rename(from, to).is_err() {
if let Err(err) = fs::rename(from, to) {
#[cfg(windows)]
const EXDEV: i32 = windows_sys::Win32::Foundation::ERROR_NOT_SAME_DEVICE as _;
#[cfg(unix)]
const EXDEV: i32 = libc::EXDEV as _;

// We will only copy if:
// 1. Files are on different devices (EXDEV error)
// 2. On Windows, if the target file exists and source file is opened by another process
// (MoveFileExW fails with "Access Denied" even if the source file has FILE_SHARE_DELETE permission)
let should_fallback = matches!(err.raw_os_error(), Some(EXDEV))
|| (from.is_file() && can_delete_file(from).unwrap_or(false));
if !should_fallback {
return Err(err);
}

// Get metadata without following symlinks
let metadata = from.symlink_metadata()?;
let file_type = metadata.file_type();
Expand Down Expand Up @@ -792,3 +807,55 @@ fn is_empty_dir(path: &Path) -> bool {
Err(_e) => false,
}
}

/// Checks if a file can be deleted by attempting to open it with delete permissions.
#[cfg(windows)]
fn can_delete_file(path: &Path) -> Result<bool, io::Error> {
use std::{
os::windows::ffi::OsStrExt as _,
ptr::{null, null_mut},
};

use windows_sys::Win32::{
Foundation::{CloseHandle, INVALID_HANDLE_VALUE},
Storage::FileSystem::{
CreateFileW, DELETE, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ,
FILE_SHARE_WRITE, OPEN_EXISTING,
},
};

let wide_path = path
.as_os_str()
.encode_wide()
.chain([0])
.collect::<Vec<u16>>();

let handle = unsafe {
CreateFileW(
wide_path.as_ptr(),
DELETE,
FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
null(),
OPEN_EXISTING,
FILE_ATTRIBUTE_NORMAL,
null_mut(),
)
};

if handle == INVALID_HANDLE_VALUE {
return Err(io::Error::last_os_error());
}

unsafe { CloseHandle(handle) };

Ok(true)
}

#[cfg(not(windows))]
fn can_delete_file(_: &Path) -> Result<bool, io::Error> {
// On non-Windows platforms, always return false to indicate that we don't need
// to try the copy+delete fallback. This is because on Unix-like systems,
// rename() failing with errors other than EXDEV means the operation cannot
// succeed even with a copy+delete approach (e.g. permission errors).
Ok(false)
}
61 changes: 61 additions & 0 deletions tests/by-util/test_mv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,22 @@ fn test_mv_rename_file() {
assert!(at.file_exists(file2));
}

#[test]
fn test_mv_with_source_file_opened_and_target_file_exists() {
let (at, mut ucmd) = at_and_ucmd!();

let src = "source_file_opened";
let dst = "target_file_exists";

let f = at.make_file(src);

at.touch(dst);

ucmd.arg(src).arg(dst).succeeds().no_stderr().no_stdout();

drop(f);
}

#[test]
fn test_mv_move_file_into_dir() {
let (at, mut ucmd) = at_and_ucmd!();
Expand Down Expand Up @@ -1670,6 +1686,51 @@ fn test_acl() {
assert!(compare_xattrs(&file, &file_target));
}

#[test]
#[cfg(windows)]
fn test_move_should_not_fallback_to_copy() {
use std::os::windows::fs::OpenOptionsExt;

let (at, mut ucmd) = at_and_ucmd!();

let locked_file = "a_file_is_locked";
let locked_file_path = at.plus(locked_file);
let file = std::fs::OpenOptions::new()
.create(true)
.write(true)
.share_mode(
uucore::windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ
| uucore::windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE,
)
.open(locked_file_path);

let target_file = "target_file";
ucmd.arg(locked_file).arg(target_file).fails();

assert!(at.file_exists(locked_file));
assert!(!at.file_exists(target_file));

drop(file);
}

#[test]
#[cfg(unix)]
fn test_move_should_not_fallback_to_copy() {
let (at, mut ucmd) = at_and_ucmd!();

let readonly_dir = "readonly_dir";
let locked_file = "readonly_dir/a_file_is_locked";
at.mkdir(readonly_dir);
at.touch(locked_file);
at.set_mode(readonly_dir, 0o555);

let target_file = "target_file";
ucmd.arg(locked_file).arg(target_file).fails();

assert!(at.file_exists(locked_file));
assert!(!at.file_exists(target_file));
}

// Todo:

// $ at.touch a b
Expand Down
Loading
0