Newtypes for uid_t, gid_t and pid_t. #629

Kixunil · 2017-06-24T08:06:44Z

No description provided.

asomers

I very much like this change. This sort of type safety is exactly the value that nix was meant to provide. But it does break backwards compatibility, so we need to be sure that our users know how to migrate their code. Could you please add an entry to CHANGELOG.md ?

asomers · 2017-06-24T15:37:04Z

src/unistd.rs

+        geteuid()
+    }
+
+    pub fn is_root(self) -> bool {


The argument should be &self so this method won't consume self

Doesn't matter much, since it's Copy, don't you think?

In this case, you're right that is_root(self) and is_root(&self) are probably equivalent. But stylistically, the latter is preferred. In the entire Nix crate, we only use the former style in one place, and that's because a std trait requires it. The nonconsuming variety is also more future-proof. For example, we might someday turn these methods into a Trait, and implement it for other objects that are harder to clone, like SIDs. Are there any downsides to taking self by reference?

I'm not aware of any downsides (apart from theoretical slow down, which should be optimized-out). I implemented it this way mostly because I didn't care, since it's Copy. I understand the style and, I'm going to change it.

Susurrus · 2017-06-24T16:02:57Z

src/unistd.rs

+    }
+}
+
+impl fmt::Display for Uid {


Can't you derive Display for this type?

AFAIK #[derive(Display)] doesn't work (what code should Rust generate?)

You're right, I was thinking of Debug. I don't think we need to have Display implemented for these types, but I'm not against it either.

Susurrus · 2017-06-24T16:06:29Z

Right now the ptrace* functions are broken because of #614. I'd like to see that fixed before this.

Kixunil · 2017-06-25T06:56:28Z

@asomers Is that changelog entry OK for you?

Susurrus · 2017-06-25T15:46:49Z

test/test_unistd.rs

@@ -90,7 +91,7 @@ mod linux_android {

    #[test]
    fn test_gettid() {
-        let tid = gettid();
+        let tid: ::libc::pid_t = gettid().into();


I'm pretty certain this is always going to be > 0 (related to getpid test above).

Me too. I just wanted to keep test as they were.

Susurrus · 2017-06-25T15:46:51Z

test/test_unistd.rs

@@ -78,8 +79,8 @@ fn test_mkstemp() {

 #[test]
 fn test_getpid() {
-    let pid = getpid();
-    let ppid = getppid();
+    let pid: ::libc::pid_t = getpid().into();


Why doesn't getpid() return a Result instead of a Pid regardless of success? That would be much more ergonomic then making the user have to cast into a libc type just to check if the return value is valid.

getpid can't fail.

You're right that it can't fail, but it can return 0. I meant to suggest using an Option<Pid> type here rather than Result. The thing is that the user shouldn't need to compare the Pid to 0, that's should be something abstracted away by this newtype. So if the user gets a Pid it should be valid, otherwise they should get a None.

I think the API is fine for normal use. The only reason for the cast is for the assertion. I don't think ordinary consumers of getpid will ever need the cast.

Something that we might improve later: use NonZero<T>, once it's stabilized (I watch relevant issue and I think it's getting close.) So the compiler would optimize Option<Pid> to Pid.

The question is whether zero PID is somehow forbidden by POSIX.

Susurrus · 2017-06-25T15:47:56Z

test/test_unistd.rs

@@ -21,7 +21,8 @@ fn test_fork_and_waitpid() {
        Ok(Child) => {} // ignore child here
        Ok(Parent { child }) => {
            // assert that child was created and pid > 0
-            assert!(child > 0);
+            let child_raw: ::libc::pid_t = child.into();


Again, it'd be great if we could make this check more ergonomic.

Susurrus · 2017-06-25T15:50:47Z

src/unistd.rs

+pub struct Uid(uid_t);
+
+impl Uid {
+    pub fn from_raw(uid: uid_t) -> Self {


All of these methods needs doccomments.

Susurrus · 2017-06-25T15:50:58Z

src/unistd.rs

+pub struct Gid(gid_t);
+
+impl Gid {
+    pub fn from_raw(gid: gid_t) -> Self {


Again, doccomments for these methods please.

Susurrus · 2017-06-25T15:51:08Z

src/unistd.rs

+pub struct Pid(pid_t);
+
+impl Pid {
+    pub fn from_raw(pid: pid_t) -> Self {


doc comments here as well.

Susurrus · 2017-06-25T15:51:37Z

CHANGELOG.md

@@ -32,6 +32,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - Changed type signature of `sys::select::FdSet::contains` to make `self`
  immutable ([#564](https://github.com/nix-rust/nix/pull/564))
 - Changed type of `sched::sched_setaffinity`'s `pid` argument to `pid_t`
+- Newtypes for UID, GID, and PID. `uid_t` changed to `Uid`, `gid_t` changed to


Please link to the pull request like the other changelog entries do. Additionally I'd rephrase this to be: "Introduced wrapper types for gid_t, pid_t, and uid_t as Gid, Pid, and Uid respectively. Various functions have been changed to use these new types as arguments. (PULL_REQUEST_LINK)"

OK, I did as you suggested.

asomers · 2017-06-29T13:59:47Z

src/unistd.rs

+
+    /// Returns true if the `Uid` represents privileged user - root. (If it equals zero.)
+    pub fn is_root(&self) -> bool {
+        self.0 == 0


if you're going to define a ROOT constant, then you should use it here.

Good point. I'll look into it.

asomers · 2017-07-01T00:15:23Z

It all looks good to me. The last step is to squash your commits. BTW, thanks for not squashing them until now; it's easier to review the unsquashed changes.

Kixunil · 2017-07-01T05:21:14Z

Done. I don't like squashing because it wipes history, that's why I didn't squash until asked. :)

Susurrus · 2017-07-01T05:26:05Z

Yeah, GitHub's interface is terrible about tracking revisions to things. I much prefer GitLab's interface for PRs where it will keep every revision, squashed or not, and let you see how things changed. Makes it nice to not have these final "alright now squash everything" discussions.

asomers · 2017-07-02T05:23:48Z

@Kixunil even though you squashed the commits, you didn't fixup the commit message. The commit message still describes changes between revisions that no longer exist. Could you please clean up the wording?

Kixunil · 2017-07-02T06:12:02Z

Sorry about that, I don't squash commits often (I probably did it only once two years ago). I hope it's fine now.

asomers · 2017-07-02T14:07:56Z

bors r+

629: Newtypes for uid_t, gid_t and pid_t. r=asomers

bors · 2017-07-02T18:13:55Z

Timed out

asomers · 2017-07-04T04:10:24Z

I'm not sure, but I think that buildbot might timeout when bors does a branch rename instead of a merge commit. If that's true, then this shouldn't timeout a second time, because there have been commits in the meantime.

bors r+

629: Newtypes for uid_t, gid_t and pid_t. r=asomers

asomers · 2017-07-04T04:44:01Z

Ok, I figured it out. Buildbot isn't building the merge commit because the author has a non-ascii character in his name, and buildbot 0.9.5 has an issue with that. I'll wait until Bors times out again, then I'll merge this PR manually.
buildbot/buildbot#3078

bors · 2017-07-04T08:24:39Z

Timed out

Kixunil · 2017-07-04T11:19:14Z

because the author has a non-ascii character in his name

Ouch, is this issue specific for the buildbot or are non-ASCII names non-standard/forbidden in git author names? (I've never had a problem with it until now.)

asomers · 2017-07-04T16:36:10Z

@Kixunil it's a bug in BuildBot, and it's already fixed upstream. I just haven't upgraded our buildbot server to that version yet. For now, I'm going to bypass bors and merge directly.

asomers requested changes Jun 24, 2017

View reviewed changes

Susurrus reviewed Jun 24, 2017

View reviewed changes

Susurrus suggested changes Jun 25, 2017

View reviewed changes

Susurrus mentioned this pull request Jun 28, 2017

Release 0.9.0 #632

Closed

10 tasks

asomers reviewed Jun 29, 2017

View reviewed changes

asomers approved these changes Jul 1, 2017

View reviewed changes

Kixunil force-pushed the newtypes branch from dd8ce7d to f7a1ce1 Compare July 1, 2017 05:19

Newtypes for uid_t, gid_t and pid_t.

b62ab09

Kixunil force-pushed the newtypes branch from f7a1ce1 to b62ab09 Compare July 2, 2017 06:10

bors bot added a commit that referenced this pull request Jul 2, 2017

Merge #629

5da6475

629: Newtypes for uid_t, gid_t and pid_t. r=asomers

bors bot added a commit that referenced this pull request Jul 4, 2017

Merge #629

acd3608

629: Newtypes for uid_t, gid_t and pid_t. r=asomers

asomers merged commit 9e51647 into nix-rust:master Jul 4, 2017

Kixunil mentioned this pull request Jul 4, 2017

Tracking issue for NonZero/Unique/Shared stabilization rust-lang/rust#27730

Closed

Kixunil deleted the newtypes branch July 5, 2017 10:54

Kixunil mentioned this pull request Jul 11, 2017

Peer credentials on Unix socket rust-lang/rust#42839

Open

Newtypes for uid_t, gid_t and pid_t. #629

Newtypes for uid_t, gid_t and pid_t. #629

Conversation

Kixunil commented Jun 24, 2017

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Jun 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jun 24, 2017

Kixunil commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Jul 1, 2017

Kixunil commented Jul 1, 2017

Susurrus commented Jul 1, 2017

asomers commented Jul 2, 2017

Kixunil commented Jul 2, 2017

asomers commented Jul 2, 2017

bors bot commented Jul 2, 2017

Timed out

asomers commented Jul 4, 2017

asomers commented Jul 4, 2017

bors bot commented Jul 4, 2017

Timed out

Kixunil commented Jul 4, 2017

asomers commented Jul 4, 2017

Kixunil Jun 24, 2017 •

edited

Loading