-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
cp: fix device file creation with -r flag #8693
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
base: main
Are you sure you want to change the base?
cp: fix device file creation with -r flag #8693
Conversation
source_is_symlink: bool, | ||
source_is_fifo: bool, | ||
source_is_socket: bool, | ||
source_is_char_device: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is starting to be too much, can we refactor this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you elaborate a little?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function has too many parameters, we should have less
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function has too many parameters, we should have less
Would something like this be acceptable:
#[derive(Debug, Clone)]
struct SourceFileType {
is_symlink: bool,
is_fifo: bool,
is_socket: bool,
is_char_device: bool,
is_block_device: bool,
#[cfg(unix)]
is_stream: bool,
}
impl SourceFileType {
fn from_metadata(metadata: &Metadata) -> Self {
#[cfg(unix)]
{
Self {
is_symlink: metadata.is_symlink(),
is_fifo: metadata.file_type().is_fifo(),
is_socket: metadata.file_type().is_socket(),
is_char_device: metadata.file_type().is_char_device(),
is_block_device: metadata.file_type().is_block_device(),
is_stream: is_stream(metadata),
}
}
#[cfg(not(unix))]
{
Self {
is_symlink: metadata.is_symlink(),
is_fifo: false,
is_socket: false,
is_char_device: false,
is_block_device: false,
}
}
}
fn is_special_file(&self) -> bool {
self.is_fifo || self.is_socket || self.is_char_device || self.is_block_device
}
}
...
fn handle_copy_mode(
source: &Path,
dest: &Path,
options: &Options,
context: &str,
source_metadata: &Metadata,
symlinked_files: &mut HashSet<FileInformation>,
source_in_command_line: bool,
source_file_type: &SourceFileType,
) -> CopyResult<PerformedAction>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use an enum
here? All those file types (symlink, fifo, socket, char, block) are distinct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, what about stream? (apologise if these questions are stupid, I'm less familiar with the linux environment than I am with Rust, I thought of an enum at first but then stopped short imagining well if the first person to do it didn't make it an enum, maybe it's possible they can happen simultaneously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the is_stream
functions and it says:
file_type.is_fifo() || file_type.is_char_device() || file_type.is_block_device()
So the is_stream
struct element could be dropped and converted into a check for matching the enum.
Fix cp -r to create device files instead of copying content when copying character and block devices. Add device detection for character and block devices similar to existing FIFO handling. Implement make_char_device() and make_block_device() functions in uucore using mknod syscall to create device files with correct major/minor numbers. Update copy logic to create device files when --recursive is used and --copy-contents is not specified. Fixes uutils#8679
9375c68
to
66d4d0d
Compare
CodSpeed Performance ReportMerging #8693 will not alter performanceComparing Summary
Footnotes
|
GNU testsuite comparison:
|
Add comprehensive tests to ensure cp -r correctly handles character and block devices: - test_cp_char_device: Verifies character devices are copied as device files - test_cp_block_device: Verifies block devices are copied as device files - test_cp_device_copy_contents: Ensures --copy-contents flag overrides default behavior - test_cp_device_permission_error: Tests proper error handling without permissions - test_cp_device_preserve_permissions: Verifies permission preservation These tests prevent regression of the fix for infinite copying when using cp -r with device files like /dev/urandom.
bba1fe2
to
54ccdd8
Compare
GNU testsuite comparison:
|
GNU testsuite comparison:
|
Fix cp -r to create device files instead of copying content when copying character and block devices.
Add device detection for character and block devices similar to existing FIFO handling. Implement make_char_device() and make_block_device() functions in uucore using mknod syscall to create device files with correct major/minor numbers.
Update copy logic to create device files when --recursive is used and --copy-contents is not specified.
Fixes #8679