Skip to content

cmp: don't reopen /dev/null on Linux - #267

Open
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:linux-dev-null
Open

oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:linux-dev-null

Conversation

@oech3

@oech3 oech3 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Don't reopen 2nd /dev/null at fn is_stdout_dev_null on Linux.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

GNU diffutils testsuite comparison:

Test results comparison:
  Current:   TOTAL: 33 / PASSED: 0 / FAILED: 33 / SKIPPED: 0
  Reference: TOTAL: 33 / PASSED: 8 / FAILED: 21 / SKIPPED: 4

Changes from main branch:
  TOTAL: +0
  PASSED: -8
  FAILED: +12

New test failures (12):
  - basic
  - bignum
  - brief-vs-stat-zero-kernel-lies
  - bug-64316
  - cmp
  - diff3
  - help-version
  - large-subopt
  - strcoll-0-names
  - strip-trailing-cr
  - timezone
  - y2038-vs-32bit

Comment thread Cargo.toml Outdated
diff = "0.1.13"
itoa = "1.0.11"
regex = "1.10.4"
rustix = { version = "1.1.4", features = ["fs"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need a new dependency to save one stat() call?
please justify it, or use std (as_fd().try_clone_to_owned() + MetadataExt::rdev())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try_clone needs additional fcntl call.

Comment thread Cargo.toml Outdated
Comment thread src/cmp.rs Outdated
Comment thread src/cmp.rs
Comment thread src/cmp.rs

#[cfg(any(target_os = "linux", target_os = "android"))]
fn is_stdout_dev_null() -> bool {
// as_fd().try_clone_to_owned() needs additional overhead of fcntl

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unix version below doesn't need fcntl either (from_raw_fd + forget)
could you please do the same with MetadataExt::rdev() + FileTypeExt::is_char_device() and drop rustix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is needs unsafe. Is it OK? clap-rs already pulls rustix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think std does not have a feature to check major/minor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants