Skip to content
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

Show mode_t as octal in std::fs Debug impls #122812

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Mar 21, 2024

Example:

fn main() {
    println!("{:?}", std::fs::metadata("Cargo.toml").unwrap().permissions());
}
  • Before: Permissions(FilePermissions { mode: 33204 })

  • After: Permissions(FilePermissions { mode: 0o100664 })

  • After: Permissions(FilePermissions { mode: 0o100664 (-rw-rw-r--) })

I thought about using the format from ls -l (-rw-rw-r--, drwxrwxr-x) but I am not sure how transferable the meaning of the higher bits between different unix systems, and anyway starting the value with a leading negative-sign seems objectionable.

@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 21, 2024
pub struct DirBuilder {
mode: mode_t,
}

struct Mode(mode_t);
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for not using Mode as a field and continuing to derive the impls?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question — I started out this change that way in fact. Here is what it looks like: #123127. I switched to the way in this PR because I found the other one too invasive to the rest of the code in this module which involves arithmetic on the mode, but either way is fine with me. Let me know if you have a preference based on reading both. There is also a third possibility which omits those std::ops impls on Mode.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

The format of ls -l is specified by POSIX. It would be reasonable to use, but yes, some systems do extend it slightly. They tend to extend it in the same way or not at all, however. More precisely, all of the ones I am aware of, even IBM i and Solaris and the like, have e.g. S_ISVTX as 01000, even though there seems to be considerable disagreement on what exactly the "sticky bit" means.

The output of this should at least be padded consistently, so that it renders in a tabular way.

What is the comparative output of something with the sticky bit set?

@@ -1574,6 +1616,12 @@ impl fmt::Debug for File {
}
}

impl fmt::Debug for Mode {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "0o{:03o}", self.0)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this is the ideal formatting, but it should be 4-padded at least. It is usually written padded to the 4th digit, so that the suid, guid, and sticky bits don't change the offset.

Suggested change
write!(f, "0o{:03o}", self.0)
write!(f, "0o{:04o}", self.0)

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up going with {:06o} because libc::S_IFMT is 0o170000 i.e. there are some higher bits that determine the file type. https://www.gnu.org/software/libc/manual/html_node/Testing-File-Type.html

Here is a related PR you might have feedback on: rust-lang/libc#3634.

@workingjubilee
Copy link
Member

GNU would admonish, re: Permission Bits:

Warning: Writing explicit numbers for file permissions is bad practice. Not only is it not portable, it also requires everyone who reads your program to remember what the bits mean. To make your program clean use the symbolic names.

@dtolnay
Copy link
Member Author

dtolnay commented Mar 27, 2024

The format of ls -l is specified by POSIX. It would be reasonable to use, but yes, some systems do extend it slightly.

👍 Good to know. I updated the PR, keeping octal but putting the ls -l style representation next to it in parens:

Permissions(FilePermissions { mode: 0o100664 (-rw-rw-r--) })

What is the comparative output of something with the sticky bit set?

https://pubs.opengroup.org/onlinepubs/009696899/utilities/ls.html doesn't call it "sticky" but it seems like "restricted deletion flag" is the same thing, which is t.

$ chmod o+t src
$ ls -ld src
drwxrwxr-t

That directory has mode 0o041775, where the 4 is S_IFDIR and the 1 is S_ISVTX.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Contemplated the vibes of the resulting formatting for a bit. Looks cool to me. The libc PR pretty much demonstrates the uniformity-in-actuality of a supposedly variably-implemented standard.

Slightly worried about the test bouncing, but there's an easier way to find out than hemming and hawwing over it for longer.

Comment on lines 29 to 61
for (expected, mode) in [
// owner readable
("FilePermissions { mode: 0o100400 (-r--------) }", libc::S_IRUSR),
// owner writable
("FilePermissions { mode: 0o100200 (--w-------) }", libc::S_IWUSR),
// owner executable
("FilePermissions { mode: 0o100100 (---x------) }", libc::S_IXUSR),
// setuid
("FilePermissions { mode: 0o104000 (---S------) }", libc::S_ISUID),
// owner executable and setuid
("FilePermissions { mode: 0o104100 (---s------) }", libc::S_IXUSR | libc::S_ISUID),
// group readable
("FilePermissions { mode: 0o100040 (----r-----) }", libc::S_IRGRP),
// group writable
("FilePermissions { mode: 0o100020 (-----w----) }", libc::S_IWGRP),
// group executable
("FilePermissions { mode: 0o100010 (------x---) }", libc::S_IXGRP),
// setgid
("FilePermissions { mode: 0o102000 (------S---) }", libc::S_ISGID),
// group executable and setgid
("FilePermissions { mode: 0o102010 (------s---) }", libc::S_IXGRP | libc::S_ISGID),
// other readable
("FilePermissions { mode: 0o100004 (-------r--) }", libc::S_IROTH),
// other writeable
("FilePermissions { mode: 0o100002 (--------w-) }", libc::S_IWOTH),
// other executable
("FilePermissions { mode: 0o100001 (---------x) }", libc::S_IXOTH),
// sticky
("FilePermissions { mode: 0o101000 (----------) }", libc::S_ISVTX),
// other executable and sticky
("FilePermissions { mode: 0o101001 (---------x) }", libc::S_IXOTH | libc::S_ISVTX),
] {
assert_eq!(format!("{:?}", FilePermissions::from_inner(libc::S_IFREG | mode)), expected);
}
Copy link
Member

Choose a reason for hiding this comment

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

This test may bounce in CI due to insufficient libc definitions. As we can see from libc, no one disagrees on the values, but some may not have the values defined in the libc crate. I think we are best off just finding out, however, if we need to trim this assert down for later.

Comment on lines 1643 to 1683
f.write_str(" (")?;
f.write_char(entry_type)?;

// Owner permissions
f.write_char(if mode & libc::S_IRUSR != 0 { 'r' } else { '-' })?;
f.write_char(if mode & libc::S_IWUSR != 0 { 'w' } else { '-' })?;
f.write_char(match (mode & libc::S_IXUSR != 0, mode & libc::S_ISUID != 0) {
(true, true) => 's', // executable and setuid
(false, true) => 'S', // setuid
(true, false) => 'x', // executable
(false, false) => '-',
})?;

// Group permissions
f.write_char(if mode & libc::S_IRGRP != 0 { 'r' } else { '-' })?;
f.write_char(if mode & libc::S_IWGRP != 0 { 'w' } else { '-' })?;
f.write_char(match (mode & libc::S_IXGRP != 0, mode & libc::S_ISGID != 0) {
(true, true) => 's', // executable and setgid
(false, true) => 'S', // setgid
(true, false) => 'x', // executable
(false, false) => '-',
})?;

// Other permissions
f.write_char(if mode & libc::S_IROTH != 0 { 'r' } else { '-' })?;
f.write_char(if mode & libc::S_IWOTH != 0 { 'w' } else { '-' })?;
f.write_char(match (entry_type, mode & libc::S_IXOTH != 0, mode & libc::S_ISVTX != 0) {
('d', true, true) => 't', // searchable and restricted deletion
('d', false, true) => 'T', // restricted deletion
(_, true, _) => 'x', // executable
(_, false, _) => '-',
})?;

f.write_char(')')
Copy link
Member

Choose a reason for hiding this comment

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

I am slightly concerned about the resulting code weight, but we can optimize that later if LLVM cannot figure this out for us.

@workingjubilee
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Apr 6, 2024

📌 Commit f50009f has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2024
@bors
Copy link
Contributor

bors commented Apr 6, 2024

⌛ Testing commit f50009f with merge e8f551e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
Show mode_t as octal in std::fs Debug impls

Example:

```rust
fn main() {
    println!("{:?}", std::fs::metadata("Cargo.toml").unwrap().permissions());
}
```

- Before: `Permissions(FilePermissions { mode: 33204 })`

- ~~After: `Permissions(FilePermissions { mode: 0o100664 })`~~

- After: `Permissions(FilePermissions { mode: 0o100664 (-rw-rw-r--) })`

~~I thought about using the format from `ls -l` (`-rw-rw-r--`, `drwxrwxr-x`) but I am not sure how transferable the meaning of the higher bits between different unix systems, and anyway starting the value with a leading negative-sign seems objectionable.~~
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 6, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2024
@dtolnay
Copy link
Member Author

dtolnay commented Apr 6, 2024

In Fuchsia, not all of the constants in https://www.gnu.org/software/libc/manual/html_node/Permission-Bits.html have the same type. 😮

@dtolnay
Copy link
Member Author

dtolnay commented Apr 6, 2024

@bors try

@bors
Copy link
Contributor

bors commented Apr 6, 2024

⌛ Trying commit aa4165b with merge 8b96ca2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
Show mode_t as octal in std::fs Debug impls

Example:

```rust
fn main() {
    println!("{:?}", std::fs::metadata("Cargo.toml").unwrap().permissions());
}
```

- Before: `Permissions(FilePermissions { mode: 33204 })`

- ~~After: `Permissions(FilePermissions { mode: 0o100664 })`~~

- After: `Permissions(FilePermissions { mode: 0o100664 (-rw-rw-r--) })`

~~I thought about using the format from `ls -l` (`-rw-rw-r--`, `drwxrwxr-x`) but I am not sure how transferable the meaning of the higher bits between different unix systems, and anyway starting the value with a leading negative-sign seems objectionable.~~
@workingjubilee
Copy link
Member

Hm? A "try" build will just be for Linux, it will not run the Fuchsia CI.

@workingjubilee
Copy link
Member

In Fuchsia, not all of the constants in https://www.gnu.org/software/libc/manual/html_node/Permission-Bits.html have the same type. 😮

This... seems extremely unlikely to be intentional.

@djkoloski
Copy link
Contributor

Still working on Fuchsia?

Yes!

Is it intentional that these constants have varying types?

My guess is "no", but I'll have to check when I'm back on Tuesday. I understand that's a while to wait, so cc @tmandry in case they know.

@workingjubilee
Copy link
Member

@djkoloski Hello again!

@djkoloski
Copy link
Contributor

Thanks for the ping, I'm taking a look now.

@djkoloski
Copy link
Contributor

rust-lang/libc#3654 should correct these incorrect constant types on Fuchsia.

@workingjubilee
Copy link
Member

Cool. I'm not going to block this PR on that landing/updating. Getting it in-process is the main part.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 9, 2024

📌 Commit aa4165b has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2024
@bors
Copy link
Contributor

bors commented Apr 10, 2024

⌛ Testing commit aa4165b with merge 87a02c3...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
Show mode_t as octal in std::fs Debug impls

Example:

```rust
fn main() {
    println!("{:?}", std::fs::metadata("Cargo.toml").unwrap().permissions());
}
```

- Before: `Permissions(FilePermissions { mode: 33204 })`

- ~~After: `Permissions(FilePermissions { mode: 0o100664 })`~~

- After: `Permissions(FilePermissions { mode: 0o100664 (-rw-rw-r--) })`

~~I thought about using the format from `ls -l` (`-rw-rw-r--`, `drwxrwxr-x`) but I am not sure how transferable the meaning of the higher bits between different unix systems, and anyway starting the value with a leading negative-sign seems objectionable.~~
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 10, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 10, 2024
@rust-log-analyzer

This comment has been minimized.

@dtolnay
Copy link
Member Author

dtolnay commented Apr 10, 2024

@bors r=workingjubilee

@bors
Copy link
Contributor

bors commented Apr 10, 2024

📌 Commit caf3766 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2024
@bors
Copy link
Contributor

bors commented Apr 10, 2024

⌛ Testing commit caf3766 with merge b14d8b2...

@bors
Copy link
Contributor

bors commented Apr 10, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing b14d8b2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 10, 2024
@bors bors merged commit b14d8b2 into rust-lang:master Apr 10, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 10, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b14d8b2): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [2.8%, 2.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.8% [2.8%, 2.8%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.201s -> 674.485s (-0.11%)
Artifact size: 318.35 MiB -> 318.39 MiB (0.01%)

@dtolnay dtolnay deleted the mode branch April 10, 2024 10:28
facebook-github-bot pushed a commit to facebookincubator/antlir that referenced this pull request Jul 15, 2024
…g format

Summary:
- [**Show mode_t as octal in std::fs Debug impls** (rust-lang/rust#122812)](rust-lang/rust#122812)
- [**Improve std::fs::Metadata Debug representation** (rust-lang/rust#124103)](rust-lang/rust#124103)

Test Plan: `buck2 test fbcode//antlir/antlir2/sendstream_parser:sendstream_parser-unittest`

Reviewed By: zertosh

Differential Revision: D59782828

fbshipit-source-id: 9cfabc3d9e82301d980e181f63a6c5ea2155e302
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants