ls: only show "+" for a real ACL, and show it before "." by 18Abhinav07 · Pull Request #15857 · uutils/coreutils

18Abhinav07 · 2026-10-26T07:11:34Z

Fixes #15809

Problem

ls -l shows + for an ACL and . for a security context. Two things went wrong:

uucore::fsxattr::has_acl returned true for any extended attribute on Linux, so a file with only a SELinux or SMACK label (security.*) was treated as having an ACL.
ls checked the label before the ACL, so a file with both showed . and the ACL was hidden.

Change

has_acl on Linux and Android counts only system.posix_acl_access, system.posix_acl_default, or a non-trivial system.nfs4_acl. A non-trivial NFSv4 ACL names a principal other than OWNER@, GROUP@ or EVERYONE@. An unreadable or absent value is not counted; unparseable content is. macOS still uses the ACL API; other platforms are unchanged.
ls picks + for an ACL, else . for a security context, else a space.

Behaviour change

A file whose only extended attributes are user.* or security.capability is no longer marked +.

Tests

uucore unit tests for has_acl and the NFSv4 parser; ls unit test for the ACL-over-context precedence.
test_ls: raw POSIX ACL shown as +, other xattrs not shown as +, and an ACL plus a context case in test_ls_selinux_context_indicator.
Run locally in a Linux container: fmt, clippy -D warnings (including feat_selinux), fsxattr 8/8, uu_ls unit 19/19, test_ls 201 passed, 1 ignored. In a Fedora container with real libselinux and a faked is_selinux_enabled, the test_acl* tests pass and the binary prints ., +, + for label, ACL, and both.

Not verified

No run on a kernel with SELinux or SMACK enabled; the label-plus-ACL test will first run in this PR's SELinux CI job.
NFSv4 was not tried on a real mount, and the "non-trivial" rule is my reading of the attribute format.
cspell was not run locally.

Overlap

#15210 also changes has_acl. Whichever lands second will need a rebase.

`ls -l` marked a file with "." when it had a security context and "+" only when has_acl() was true. has_acl() treated any extended attribute as an ACL, so a security.selinux or security.SMACK64 label alone made it return true; and the label check ran first, so a file with both a label and an ACL showed ".". On Linux and Android has_acl() now counts only system.posix_acl_access, system.posix_acl_default and a non-trivial system.nfs4_acl (one naming a principal other than OWNER@, GROUP@ or EVERYONE@). ls checks the ACL first, then the label. Fixes uutils#15809

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

sylvestre · 2026-10-26T21:14:31Z

+/// The choice behind [`alt_access_indicator`]: an ACL wins over a security
+/// context, because the label is itself an extended attribute and says nothing
+/// about the ACL.
+fn alt_access_indicator_for(has_acl: bool, has_context: bool) -> u8 {


do we need a second function just for the unit test? test_ls already covers the precedence

sylvestre · 2026-10-26T21:14:42Z

+/// through the raw attribute so that only `setfattr` is needed. Returns false when
+/// that is not possible here (no `setfattr`, or a file system without ACLs).
+#[cfg(any(target_os = "linux", target_os = "android"))]
+fn set_posix_acl(path: &str) -> bool {


why a raw hex blob? the other tests just use setfacl, please do the same

sylvestre · 2026-10-26T21:14:48Z

+    let at = &scene.fixtures;
+    at.touch("with_xattr");
+
+    match Command::new("setfattr")


this setfattr match block could be a small helper, it is the same pattern repeated in the file

Inline the precedence in alt_access_indicator and drop the extra function and its unit test, set ACLs with setfacl like the other tests, and share the command handling in one helper.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

18Abhinav07 · 2026-10-30T05:21:56Z

Thanks for the review, all three are addressed (449eccf, plus 00d3e23 for a clippy error in the SELinux test that the first push introduced):

The precedence is inline in alt_access_indicator again; the extra function and its unit test are gone, test_ls covers it.
The tests use setfacl -m u:bin:r-- instead of the raw ACL blob.
The repeated command handling is one small helper, run_attr_tool, used by the three new tests.

Checked locally on Linux with --features ls and ls feat_selinux: clippy -D warnings clean, rustfmt clean, the ls tests pass apart from test_ls_capabilities, which needs sudo and fails on main in the same environment.

codspeed · 2026-10-30T06:57:51Z

Merging this PR will degrade performance by 5.48%

2 warnings, 1 note

⚠️ 125 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 1 benchmark has no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 399 untouched benchmarks
⏩ 70 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	CPU Simulation	`mv_force_overwrite`	127.9 ms	154.4 ms	-17.13%
⚡	CPU Simulation	`sort_long_line[10000]`	1,077 µs	999 µs	+7.8%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing 18Abhinav07:ls-acl-indicator (00d3e23) with main (c2fd410)}

70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Copilot AI balanced review requested due to automatic review settings October 26, 2026 07:11

Copilot AI reviewed Oct 26, 2026

sylvestre reviewed Oct 26, 2026

View reviewed changes

ls: simplify the ACL indicator and its tests per review

449eccf

Inline the precedence in alt_access_indicator and drop the extra function and its unit test, set ACLs with setfacl like the other tests, and share the command handling in one helper.

Copilot AI balanced review requested due to automatic review settings October 27, 2026 10:56

Copilot AI reviewed Oct 27, 2026

ls: drop an empty else left in the SELinux indicator test

00d3e23

Copilot AI balanced review requested due to automatic review settings October 30, 2026 05:14

Copilot AI reviewed Oct 30, 2026

Navigation Menu

Repository navigation

ls: only show "+" for a real ACL, and show it before "." - #15857

ls: only show "+" for a real ACL, and show it before "."#15857
18Abhinav07 wants to merge 3 commits into
uutils:mainfrom
18Abhinav07:ls-acl-indicator

18Abhinav07 commented Oct 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sylvestre Oct 26, 2026

Uh oh!

sylvestre Oct 26, 2026

Uh oh!

sylvestre Oct 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

18Abhinav07 commented Oct 30, 2026

Uh oh!

codspeed Bot commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

18Abhinav07 commented Oct 26, 2026

Problem

Change

Behaviour change

Tests

Not verified

Overlap

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

18Abhinav07 commented Oct 30, 2026

Uh oh!

codspeed Bot commented Oct 30, 2026

Merging this PR will degrade performance by 5.48%

Performance Changes

Footnotes

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants