Misc cleanup #814

Susurrus · 2017-12-06T16:12:02Z

Add more traits to the various datatypes in nix. Also try to utilize more libc types where appropriate.

This is still WIP because:

Looking for feedback on ba2d85b. Does it make sense for bitflags structs to have Ord & PartialOrd derived for them?
Need to implement a newtype wrapper around libc::linger with both a new() constructor and getters. Additionally this needs manual implementations of Eq, PartialEq, Debug (I've been skipping manually implementing Hash for now.
In 2b3fb11 I need to implement newtype wrappers around ip_mreq and ipv6_mreq that have new() constructors and field getters. Additionally they need manual implementations of Eq, PartialEq, Debug (I've been skipping manually implementing Hash for now. This commit also needs a CHANGELOG entry.
ed79cfb needs a CHANGELOG entry because its variants names have changed with the switch to using libc_bitflags!.

Susurrus · 2017-12-06T16:58:15Z

@asomers It looks like ucred isn't in libc for FreeBSD, but I did find that it should exist here. I am a little confused though on whether this is a similar interface on Linux and FreeBSD or if this should be restricted to Linux, as I'm not too familiar with the whole ucred API.

Susurrus · 2017-12-06T16:59:59Z

For example, I'm not certain if we should be using ucred on FreeBSD or should be using xucred instead.

Susurrus · 2017-12-06T17:53:55Z

For the BSDs, it's looks like NetBSD uses the sockcred struct, which is missing from libc and FreeBSD and DragonFlyBSD uses the cmsgcred struct, also missing from libc. I don't believe OpenBSD has this interface as SCM_CREDS is missing from their socket.h.

Next steps are to add those structs to libc. I'm not certain how I want to abstract this, if I want to hide all these structs behind the new UserCredentials datatype with appropriate accessor methods or have separate types for each different platform. I'm leaning towards abstrating it all away behind UserCredentials, but we'll see how the implementation goes.

asomers · 2017-12-07T16:04:07Z

@Susurrus The answer lies here. Despite the doc comments, it looks like Nix is using UnixCredentials with Linux's SO_PEERCRED socket option. The analogous socket option on FreeBSD is LOCAL_PEERCRED, which returns a struct xucred. struct xucred, however, has different fields than Linux's struct ucred, so we would need to conditionally compile the various accessors. Linux also has an SCM_CREDENTIALS message type for use with sendmsg. FreeBSD uses a struct cmsgcred for that. However, Nix doesn't seem to support that yet.

asomers · 2017-12-07T16:07:02Z

src/sys/socket/mod.rs

@@ -173,6 +170,47 @@ libc_bitflags!{
    }
 }

+/// Unix credentials of the sending process.
+///
+/// This struct is used with the `SCM_CREDENTIALS` ancillary message for UNIX sockets.


This message, while true, is red herring because Nix doesn't support SCM_CREDENTIALS. It should read "This struct is used with the SO_PEERCRED/LOCAL_PEERCRED socket options for UNIX sockets.

asomers · 2017-12-07T16:07:23Z

src/sys/socket/mod.rs

+/// This struct is used with the `SCM_CREDENTIALS` ancillary message for UNIX sockets.
+#[repr(C)]
+#[derive(Clone, Copy)]
+pub struct UnixCredentials(libc::ucred);


should be libc::xucred on FreeBSD.

This is a very different looking struct, I'm really surprised that this is correct. I would have thought it'd be the struct I mentioned above, cmsgcred, which does have pid, uid, and gid fields.

Except there are two problems. If UnixCredentials were actually used with SCM_CREDENTIALS, then you would be correct. struct cmsgcred would be the struct to wrap. However, the SCM_CREDENTIALS comment is red herring. UnixCredentials is actually used as a socket option, with SO_PEERCRED. That's why I say that it should wrap xucred.

From this SO question it seems like SCM_CREDENTIALS should be what we offer as an API and we abandon the SO_PEERCRED implementation, since one looks to be a newer/better version of the other.

asomers · 2017-12-07T16:08:15Z

src/sys/socket/sockopt.rs

@@ -151,7 +152,7 @@ sockopt_impl!(Both, OobInline, libc::SOL_SOCKET, libc::SO_OOBINLINE, bool);
 sockopt_impl!(GetOnly, SocketError, libc::SOL_SOCKET, libc::SO_ERROR, i32);
 sockopt_impl!(Both, KeepAlive, libc::SOL_SOCKET, libc::SO_KEEPALIVE, bool);
 #[cfg(all(target_os = "linux", not(target_arch="arm")))]
-sockopt_impl!(GetOnly, PeerCredentials, libc::SOL_SOCKET, libc::SO_PEERCRED, super::ucred);
+sockopt_impl!(GetOnly, PeerCredentials, libc::SOL_SOCKET, libc::SO_PEERCRED, super::UnixCredentials);


On FreeBSD, this should be LOCAL_PEERCRED instead of SO_PEERCRED.

Susurrus · 2017-12-08T05:22:35Z

Note that all Linux MIPS build jobs will fail until rust-lang/libc#867 is merged. Once that's merged, this can go in.

I'm going to back off expanding APIs to any other platforms as it's just too much work. I'd really like to get all this libc cleanup stuff wrapper up, and it's just taking way too long! I keep finding new constants or types we have squirreled away that should be in libc instead. Hopefully this gets us pretty damn close to the end of it.

Susurrus · 2017-12-08T15:55:49Z

Should go green once this runs through. @asomers Let me know if you have any feedback here, hopefully these changes are uncontroversial at this point.

Susurrus · 2017-12-09T22:08:42Z

@asomers Can I get a final check with you on this before I merge it?

asomers

It all looks good, but I think you should group the CHANGELOG entries with the changes that made them.

asomers · 2017-12-11T03:56:36Z

CHANGELOG.md

@@ -83,6 +85,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - The `ucred` struct has been removed in favor of a `UserCredentials` struct that
  contains only getters for its fields.
  ([#814](https://github.com/nix-rust/nix/pull/814))
+- Both `ip_mreq` and `ipv6_mreq` have been replaced with `IpMembershipRequest` and


This bit should be squashed with a9022b5

asomers · 2017-12-11T04:00:53Z

CHANGELOG.md

@@ -98,6 +103,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#747](https://github.com/nix-rust/nix/pull/747))
 - The `Errno` variants are no longer reexported from the `errno` module. `Errno` itself is no longer reexported from the
  crate root and instead must be accessed using the `errno` module. ([#696](https://github.com/nix-rust/nix/pull/696))
+- Removed `MS_VERBOSE`, `MS_NOSEC`, and `MS_BORN` from `MsFlags`. These


This part should be squashed with 5837c6d

asomers · 2017-12-11T04:01:46Z

CHANGELOG.md

@@ -48,6 +48,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - Added the `from_raw()` method to `WaitStatus` for converting raw status values
  to `WaitStatus` independent of syscalls.
  ([#741](https://github.com/nix-rust/nix/pull/741))
+- Added more standard trait implementations for various types.


This bit should be squashed with e59c72b and 310d205

asomers · 2017-12-11T04:02:37Z

CHANGELOG.md

+- The `ucred` struct has been removed in favor of a `UserCredentials` struct that
+  contains only getters for its fields.
+  ([#814](https://github.com/nix-rust/nix/pull/814))
+- Both `ip_mreq` and `ipv6_mreq` have been replaced with `IpMembershipRequest` and


This bit should be squashed wit a9022b5

asomers · 2017-12-11T04:03:03Z

CHANGELOG.md

  ([#749](https://github.com/nix-rust/nix/pull/749))
 - `AioCb::Drop` will now panic if the `AioCb` is still in-progress ([#715](https://github.com/nix-rust/nix/pull/715))
 - Restricted `nix::sys::socket::UnixAddr::new_abstract` to Linux and Android only.
  ([#785](https://github.com/nix-rust/nix/pull/785))
+- The `ucred` struct has been removed in favor of a `UserCredentials` struct that


This bit should be squashed with c25f551

This hasn't been available for a while now. And was not updated for even longer before that. Let's not mislead our users any longer.

Susurrus · 2017-12-11T16:38:34Z

You're right, I was just being lazy about it.

Bors r+ asomers

814: Misc cleanup r=Susurrus a=Susurrus Add more traits to the various datatypes in `nix`. Also try to utilize more `libc` types where appropriate. This is still WIP because: * Looking for feedback on ba2d85b. Does it make sense for bitflags structs to have `Ord` & `PartialOrd` derived for them? * Need to implement a newtype wrapper around `libc::linger` with both a `new()` constructor and getters. Additionally this needs manual implementations of `Eq`, `PartialEq`, `Debug` (I've been skipping manually implementing `Hash` for now. * In 2b3fb11 I need to implement newtype wrappers around `ip_mreq` and `ipv6_mreq` that have `new()` constructors and field getters. Additionally they need manual implementations of `Eq`, `PartialEq`, `Debug` (I've been skipping manually implementing `Hash` for now. This commit also needs a CHANGELOG entry. * ed79cfb needs a CHANGELOG entry because its variants names have changed with the switch to using `libc_bitflags!`.

bors · 2017-12-11T17:52:09Z

Build succeeded

Susurrus force-pushed the misc_cleanup branch from 2b3fb11 to eb998f3 Compare December 6, 2017 16:57

asomers reviewed Dec 7, 2017

View reviewed changes

Susurrus force-pushed the misc_cleanup branch 2 times, most recently from 3462db7 to 3a741ed Compare December 8, 2017 05:11

Susurrus force-pushed the misc_cleanup branch from 3a741ed to fe51348 Compare December 8, 2017 05:26

Susurrus changed the title ~~WIP: Misc cleanup~~ Misc cleanup Dec 8, 2017

Susurrus force-pushed the misc_cleanup branch 4 times, most recently from e15e5dc to 7c387b6 Compare December 9, 2017 19:01

asomers requested changes Dec 11, 2017

View reviewed changes

Bryant Mairs added 8 commits December 10, 2017 20:46

Expand derives for all libc_bitflags! use

75e798f

Add more derives for SignalFd and various enums

de9d72d

Remove internal kernel flags

e95cf88

Migrate more bitflags to use libc_bitflags!

3924361

Use libc::linger

ff79dcb

Replace custom ucred struct with newtype wrapper around libc::ucred

0d78c04

Replace ip(v6)_mreq structs with libc equivalent

2c4315a

Remove references to developer docs

111950d

This hasn't been available for a while now. And was not updated for even longer before that. Let's not mislead our users any longer.

Susurrus force-pushed the misc_cleanup branch from 7c387b6 to 111950d Compare December 11, 2017 04:49

bors bot merged commit 111950d into nix-rust:master Dec 11, 2017

Susurrus deleted the misc_cleanup branch December 11, 2017 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc cleanup #814

Misc cleanup #814

Susurrus commented Dec 6, 2017 •

edited

Loading

Susurrus commented Dec 6, 2017

Susurrus commented Dec 6, 2017

Susurrus commented Dec 6, 2017

asomers commented Dec 7, 2017

asomers Dec 7, 2017

asomers Dec 7, 2017

Susurrus Dec 8, 2017

asomers Dec 8, 2017

Susurrus Dec 8, 2017

asomers Dec 7, 2017

Susurrus commented Dec 8, 2017

Susurrus commented Dec 8, 2017

Susurrus commented Dec 9, 2017

asomers left a comment

asomers Dec 11, 2017

asomers Dec 11, 2017

asomers Dec 11, 2017

asomers Dec 11, 2017

asomers Dec 11, 2017

Susurrus commented Dec 11, 2017

bors bot commented Dec 11, 2017

Misc cleanup #814

Misc cleanup #814

Conversation

Susurrus commented Dec 6, 2017 • edited Loading

Susurrus commented Dec 6, 2017

Susurrus commented Dec 6, 2017

Susurrus commented Dec 6, 2017

asomers commented Dec 7, 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

Susurrus commented Dec 8, 2017

Susurrus commented Dec 8, 2017

Susurrus commented Dec 9, 2017

asomers left a 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

Susurrus commented Dec 11, 2017

bors bot commented Dec 11, 2017

Build succeeded

Susurrus commented Dec 6, 2017 •

edited

Loading