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

dtolnay · 2024-03-21T05:17:42Z

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 · 2024-03-21T05:17:50Z

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

Noratrieb · 2024-03-21T06:25:10Z

library/std/src/sys/pal/unix/fs.rs

 pub struct DirBuilder {
    mode: mode_t,
 }

+struct Mode(mode_t);


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

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.

workingjubilee

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?

workingjubilee · 2024-03-21T06:31:41Z

library/std/src/sys/pal/unix/fs.rs

@@ -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)


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)

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 · 2024-03-21T06:42:18Z

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 · 2024-03-27T08:59:50Z

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.

workingjubilee

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.

workingjubilee · 2024-04-06T18:51:55Z

library/std/src/sys/pal/unix/fs/tests.rs

+    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);
+    }


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.

workingjubilee · 2024-04-06T18:53:03Z

library/std/src/sys/pal/unix/fs.rs

+        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(')')


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

workingjubilee · 2024-04-06T18:55:18Z

@bors r+ rollup=iffy

bors · 2024-04-06T18:55:21Z

📌 Commit f50009f has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-04-06T19:16:47Z

⌛ Testing commit f50009f with merge e8f551e...

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.~~

bors · 2024-04-06T19:41:15Z

💔 Test failed - checks-actions

dtolnay · 2024-04-06T20:14:16Z

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 · 2024-04-06T20:14:55Z

@bors try

bors · 2024-04-06T20:16:07Z

⌛ Trying commit aa4165b with merge 8b96ca2...

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 · 2024-04-06T21:08:04Z

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

workingjubilee · 2024-04-06T21:10:39Z

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 · 2024-04-07T00:25:30Z

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 · 2024-04-09T16:29:39Z

@djkoloski Hello again!

djkoloski · 2024-04-09T16:30:59Z

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

djkoloski · 2024-04-09T17:09:49Z

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

workingjubilee · 2024-04-09T20:13:17Z

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

@bors r+

bors · 2024-04-09T20:13:20Z

📌 Commit aa4165b has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-04-10T00:07:06Z

⌛ Testing commit aa4165b with merge 87a02c3...

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.~~

bors · 2024-04-10T00:39:27Z

💔 Test failed - checks-actions

dtolnay · 2024-04-10T02:11:38Z

@bors r=workingjubilee

bors · 2024-04-10T02:11:40Z

📌 Commit caf3766 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-04-10T04:48:00Z

⌛ Testing commit caf3766 with merge b14d8b2...

bors · 2024-04-10T06:50:00Z

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

rust-timer · 2024-04-10T08:06:45Z

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%)

…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

rustbot assigned workingjubilee Mar 21, 2024

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

Noratrieb reviewed Mar 21, 2024

View reviewed changes

workingjubilee reviewed Mar 21, 2024

View reviewed changes

dtolnay force-pushed the mode branch from 64c122a to f955b6f Compare March 27, 2024 08:25

dtolnay mentioned this pull request Mar 27, 2024

Use Mode instead of mode_t in std::fs fields #123127

Closed

dtolnay force-pushed the mode branch from f955b6f to f50009f Compare March 27, 2024 09:16

workingjubilee approved these changes Apr 6, 2024

View reviewed changes

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

This comment has been minimized.

Sign in to view

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 force-pushed the mode branch from f50009f to aa4165b Compare April 6, 2024 20:14

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

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

Show mode_t as octal in std::fs Debug impls

caf3766

dtolnay force-pushed the mode branch from e0b5a82 to caf3766 Compare April 10, 2024 01:13

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 added the merged-by-bors This PR was explicitly merged by bors. label Apr 10, 2024

bors merged commit b14d8b2 into rust-lang:master Apr 10, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 10, 2024

dtolnay deleted the mode branch April 10, 2024 10:28

Noratrieb mentioned this pull request Apr 21, 2024

Debug for FilePermissions prints the mode in decimal. #124226

Closed

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

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

Conversation

dtolnay commented Mar 21, 2024 • edited Loading

rustbot commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Mar 21, 2024

dtolnay commented Mar 27, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Apr 6, 2024

bors commented Apr 6, 2024

bors commented Apr 6, 2024

This comment has been minimized.

bors commented Apr 6, 2024

dtolnay commented Apr 6, 2024

dtolnay commented Apr 6, 2024

bors commented Apr 6, 2024

workingjubilee commented Apr 6, 2024

workingjubilee commented Apr 6, 2024

djkoloski commented Apr 7, 2024

workingjubilee commented Apr 9, 2024

djkoloski commented Apr 9, 2024

djkoloski commented Apr 9, 2024

workingjubilee commented Apr 9, 2024

bors commented Apr 9, 2024

bors commented Apr 10, 2024

This comment has been minimized.

bors commented Apr 10, 2024

This comment has been minimized.

dtolnay commented Apr 10, 2024

bors commented Apr 10, 2024

bors commented Apr 10, 2024

bors commented Apr 10, 2024

rust-timer commented Apr 10, 2024

Overall result: ❌ regressions - no action needed

dtolnay commented Mar 21, 2024 •

edited

Loading

workingjubilee left a comment •

edited

Loading