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

merge socket constants #647

Merged
merged 6 commits into from
Aug 1, 2017
Merged

merge socket constants #647

merged 6 commits into from
Aug 1, 2017

Conversation

ndusart
Copy link
Contributor

@ndusart ndusart commented Jul 5, 2017

Refactor the constant declarations such that all constants are only declared once with a #[cfg] that only enables the constant on the correct platforms.

Closes #637

(same PR as #646 but from another branch, to see if buildbot has a problem with PR made from master branch)

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

There are a ton of constants here that are available on more platforms than are specified. I went through most of them, but I'd appreciate if you double-checked all of them and exposed them on all platforms as well.

@@ -93,11 +277,16 @@ mod os {
pub flags MsgFlags: libc::c_int {
MSG_OOB,
MSG_PEEK,
MSG_DONTWAIT,
#[cfg(not(target_os = "dragonfly"))]
MSG_CTRUNC,
Copy link
Contributor

Choose a reason for hiding this comment

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

MSG_CTRUNC, MSG_EOR, and MSG_TRUNC are all available in libc for DragonflyBSD.

target_os = "ios",
target_os = "openbsd",
target_os = "netbsd"))]
pub const SO_USELOOPBACK: c_int = libc::SO_USELOOPBACK;
Copy link
Contributor

Choose a reason for hiding this comment

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

This exists on DragonflyBSD as well.

target_os = "ios",
target_os = "openbsd",
target_os = "netbsd"))]
pub const SO_NOSIGPIPE: c_int = 0x1022;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is defined for macos and ios

target_os = "ios",
target_os = "openbsd",
target_os = "netbsd"))]
pub const SO_NOADDRERR: c_int = 0x1023;
Copy link
Contributor

Choose a reason for hiding this comment

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

Defined for macos/ios

target_os = "ios",
target_os = "openbsd",
target_os = "netbsd"))]
pub const SO_NWRITE: c_int = 0x1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

defined for macos/ios

pub const SO_PEEK_OFF: c_int = libc::SO_PEEK_OFF;
#[cfg(any(target_os = "linux", target_os = "android"))]
pub const SO_PEERCRED: c_int = libc::SO_PEERCRED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also supported by openbsd, bitrig, and haiku.


pub const SOCK_STREAM: c_int = libc::SOCK_STREAM;
pub const SOCK_DGRAM: c_int = libc::SOCK_DGRAM;
pub const SOCK_SEQPACKET: c_int = libc::SOCK_SEQPACKET;
pub const SOCK_RAW: c_int = libc::SOCK_RAW;
#[cfg(not(any(target_os = "linux", target_os = "android")))]
pub const SOCK_RDM: c_int = libc::SOCK_RDM;
Copy link
Contributor

Choose a reason for hiding this comment

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

Supported on netbsd, freebsd, dragonfly, mac, ios.

Copy link
Contributor Author

@ndusart ndusart Jul 6, 2017

Choose a reason for hiding this comment

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

I wrote the #[cfg] in an exclusive manner as it's shorter (there is a not()).
I'll use an inclusive form for matching the other #[cfg]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, yes, missed that, sorry. But it is a good idea to use a whitelist and not a blacklist, as we'll be adding new platforms moving forward and they'll be more robust to these additions (though unfortunately likely more verbose).

@Susurrus Susurrus mentioned this pull request Jul 6, 2017
@Susurrus
Copy link
Contributor

Susurrus commented Jul 6, 2017

There are also 27 constants here that need to be upstreamed for all supported platforms. I'd love to have that work be done and merged in with this PR so we can have all of these constants defined correctly in libc and reused here. Would you be willing to do that work as part of this or is that too much to ask?

@ndusart
Copy link
Contributor Author

ndusart commented Jul 6, 2017

I could find the time for this :-)

Just a bit confused where I could find sys/ headers for BSD platforms. For other I have running machines where I can find them.

@Susurrus
Copy link
Contributor

Susurrus commented Jul 6, 2017

Excellent, we'd very much appreciate it. We have a huge backlog of FFI constants and functions that aren't correct across all platforms, and it'd be awesome to get better cross-platform support by upstreaming proper values.

I documented this in an old PR of mine: rust-lang/libc#625. I keep meaning to add that to the libc docs so that new users can more easily add missing constants, but I haven't had the time yet.

@asomers
Copy link
Member

asomers commented Jul 6, 2017

@ndusart
Copy link
Contributor Author

ndusart commented Jul 6, 2017

Thanks for the links.

@Susurrus I made the proposed change, thanks for the listing.

I added some marker comments beside the symbols I'd like to check. It'll be cleaned, it's just for helping me tracking the progress.

MSG_EOR,
#[cfg(any(target_os = "linux", target_os = "android"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn, was reading as a blacklist there, and seeing the symbols defined for them I removed it. Should definitely use the same form for all #[cfg] ^^

@ndusart
Copy link
Contributor Author

ndusart commented Jul 7, 2017

Worked a bit on this now.

I can find SO_DONTTRUNC reference only in MacOS source, it's not present on other BSD platforms. Currently we present this constant for all BSD except netbsd and dragonfly. That match the fact that this constant is defined in libc only for Apple architectures.

SO_LABEL seems to be only defined in freebsd, macos and ios. As of now, we present it for all BSD except dragonfly.

SO_NREAD and SO_NKE seem to only be defined in macos and ios. Now, we present it for all BSD except dragonfly.

SO_NOSIGPIPE does not seem to be defined for openbsd and netbsd. It appears in some source files, but I think this files are written with cross-platform compatibility in mind and there is some #ifdef somewhere to not include that part of the code on this architectures.

SO_NOADDRERR does not seem to be defined for openbsd, netbsd and freebsd.

That's how far I get for now.

I'd like to know how to handle these cases, should we remove them or keep them for not breaking the API ?
Maybe I'm wrong and I didn't look correctly, please correct me if it is the case.

@Susurrus
Copy link
Contributor

Susurrus commented Jul 7, 2017

@ndusart One of the reasons we're upstreaming constants is because most of nix's declarations were not correct across all supported platforms, especially as it's been supporting more platforms. So it doesn't surprise me that his many constants are improperly declared in nix. I have no concern of breaking the API in nix when it's correct to do so because at the end of the day if someone used those constants on platforms they didn't exist on and get undefined behavior, it'd eventually come back as a bug report to nix.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 9, 2017

I have some difficulties with three constants for macos and ios:

  • SO_RESTRICTIONS
  • SO_RESTRICT_DENYIN (became SO_RESTRICT_DENY_IN since Mac OS 10.9)
  • SO_RESTRICT_DENYOUT (became SO_RESTRICT_DENY_OUT since Mac OS 10.9)

There are defined here: https://github.com/opensource-apple/xnu/blob/dc0628e187c3148723505cf1f1d35bb948d3195b/bsd/sys/socket.h#L188-L190

But cannot make libc CI find these constants. The header was well included and PRIVATE was defined but no luck.

Should we keep them hardcoded ? The values are correct. (Just SO_RESTRICT_DENYSET which was removed since 10.9)

@Susurrus
Copy link
Contributor

Susurrus commented Jul 9, 2017

Those constants don't actually exist on Mac as of 10.11.6 (I have one locally). I did grep SO_RESTRICT -r /usr/include and turned up nothing. Additionally I looked through /usr/include/sys/socket.h and those values are gone. They shouldn't be added to libc for macos and ios and be excluded in nix as well.

@@ -1,75 +1,253 @@
pub use self::os::*;

#[cfg(any(target_os = "linux", target_os = "android"))]
mod os {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this os module anymore since all constants are now included individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll remove this

@@ -1,75 +1,253 @@
pub use self::os::*;

#[cfg(any(target_os = "linux", target_os = "android"))]
mod os {
use libc::{self, c_int, uint8_t};

pub const AF_UNIX: c_int = libc::AF_UNIX;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to group any of these values together into enums?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is not that already done in the nix::sys::socket::addr module ?
Specifically AF_* are grouped in the AddressFamily enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering then if this consts module should not be removed completely in favor of libc ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely should. All constants should be declared in libc and we should just wrap them up into nicer types. It looks like that's already done for the AF_* constants, but it should be done for these other ones as well.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 10, 2017

AF_* constants are in AddressFamily enum.
SOCK_* constants are defined in SockType enum.
SOL_* and IPPROTO_* are defined in SockLevel enum. (IPPROTO_* seem to be preferred over SOL_*, SockLevel correctly use these when applicable)

The constants that are not being defined in an enum are the socket options constants: SO_*, IP_* or TCP_* (maybe other depending on the protocol level of the option), but structs are built around them in sockopts.rs so we have sort of an "higher-level type" for these constants.

My last commit remove the consts module and tests are passing on linux (x86_64) when I use my branch of libc where I added the missing definitions (rust-lang/libc/pull/656).

bors added a commit to rust-lang/libc that referenced this pull request Jul 10, 2017
add missing socket constants

Add some missing socket constants that are hardcoded by `nix` (nix-rust/nix#647)

I took the opportunity to merge some constants in a upper module when applicable.
@@ -702,7 +702,7 @@ pub mod sys_control {
let addr = sockaddr_ctl {
sc_len: mem::size_of::<sockaddr_ctl>() as c_uchar,
sc_family: AddressFamily::System as c_uchar,
ss_sysaddr: consts::AF_SYS_CONTROL as uint16_t,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is defined as a c_int, is sockaddr_ctl correct or is libc wrong on the datatype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sockaddr_ctl is correctly defined. It's defined in macos here, ss_sysaddr is indeed u_int16_t.

I do not know if AF_SYS_CONTROL is used anywhere else.
We could still make that change in libc as it has not been released yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file a PR with upstream then? Seems like AF_SYS_CONTROL should be a u16, do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely agree. I'll be able to do that tomorrow.

Copy link
Contributor

@Susurrus Susurrus Jul 10, 2017

Choose a reason for hiding this comment

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

Looking at the headers it looks like the value is used as multiple datatypes. This might not be super straightforward.

Copy link
Contributor Author

@ndusart ndusart Jul 10, 2017

Choose a reason for hiding this comment

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

I don't think AF_SYS_CONTROL should be considered as the same type as other AF_* constants. You'll not use it in socket function for example, there you should use AF_SYSTEM. AF_SYS_CONTROL seems like a subtype to AF_SYSTEM that only appear in sockaddr_ctl when connecting to the socket.
But you are right, AF_* constants are used sometimes as int (in socket function) or as sa_family_t (in sockaddr struct).
I guess the problem is not applicable to AF_SYS_CONTROL but I cannot find good documentation about its use though :/

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this is resolved as a WONTFIX on our end then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. AF_SYS_CONTROL does not seem to be used in other way than as u16 but I never used these AF_SYSTEM addresses, so in doubt, probably it's better to let it as c_int ?

sockopt_impl!(Both, SndBuf, consts::SOL_SOCKET, consts::SO_SNDBUF, usize);
sockopt_impl!(Both, TcpKeepIdle, libc::IPPROTO_TCP, libc::TCP_KEEPIDLE, u32);
sockopt_impl!(Both, RcvBuf, libc::SOL_SOCKET, libc::SO_RCVBUF, usize);
sockopt_impl!(Both, SndBuf, libc::SOL_SOCKET, libc::SO_SNDBUF, usize);
#[cfg(target_os = "linux")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this and SndBufForce can be implemented on Android as well.

Copy link
Contributor Author

@ndusart ndusart Jul 17, 2017

Choose a reason for hiding this comment

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

You're right and SndBufForce is defined in Linux ARM too.

sockopt_impl!(SetOnly, SndBufForce, consts::SOL_SOCKET, consts::SO_SNDBUFFORCE, usize);
sockopt_impl!(GetOnly, SockType, consts::SOL_SOCKET, consts::SO_TYPE, super::SockType);
sockopt_impl!(SetOnly, SndBufForce, libc::SOL_SOCKET, libc::SO_SNDBUFFORCE, usize);
sockopt_impl!(GetOnly, SockType, libc::SOL_SOCKET, libc::SO_TYPE, super::SockType);
#[cfg(any(target_os = "freebsd",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is available on all targets, why don't you just remove the cfg attribute and we'll see if everything passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Flags for send/recv and their relatives
libc_bitflags!{
pub flags MsgFlags: libc::c_int {
MSG_OOB,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add doc comments for each of these? They're documented here.

#[cfg(any(target_os = "macos", target_os = "ios"))]
System = consts::AF_SYSTEM,
System = libc::AF_SYSTEM,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are actually a ton more AF_ constants defined, at least on Linux. If you search through the libc source, you'll find a bunch, would you be willing to add them for the appropriate platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll go through the constants defined in libc and try to merge all them in here.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 17, 2017

Travis is compiling libc 0.2.26 instead if the upstream master branch. Then it's complaining about missing constants that are not released yet.
Do you know why it keeps this version ?

@ndusart ndusart force-pushed the socket-const branch 2 times, most recently from a8cca80 to 5b0b047 Compare July 17, 2017 08:56
@Wolvereness
Copy link
Contributor

Wolvereness commented Jul 17, 2017

@ndusart This commit is being merged: 5527ce8, which changes the version in the Cargo.toml to rust-lang/libc@288942e, and it doesn't include the changes you need. You need to rebase on current master, because 3912a20 (your upstream) is before the previously mentioned commit, and then change the dependency back.

Basically, same thing I had to do: 7b21417

Edit: of note, I added the last line, and I did change the libc link to the v0.2.26 commit instead of v0.2.25.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 17, 2017

Thanks @Wolvereness.
I thought CI was testing against the head of the source branch of the PR. (It seems it's the case for buildbot as it fetch the master branch of libc)

Looking in Travis CI logs, I see that it fetch the source with

git fetch origin +refs/pull/647/merge

which is the PR merged to master.

I'll rebase then on master so I can modify libc dependency with a new commit.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 26, 2017

I added doc to AddressFamily and SockType.

Regarding the test nix-text/src/const.c, are these tests ever run ? Because tests are passing here while constants are not defined anymore.
Anyway, they can be all removed as they are no socket constant definition in this PR.

@Susurrus
Copy link
Contributor

@ndusart Yes, the tests are run. When you run cargo test it compiles and runs nix-test as part of that. It's only if there are tests in /test that depend on stuff implemented in nix-test will you see anything happen. Currently that's not for very much right now.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 27, 2017

Well, I think is quite done now. :)
Waiting rust-lang/libc#701 to fix the test failure for s390x, and it should be ok.

Do you have any remark on the current state of this PR ?

@ndusart
Copy link
Contributor Author

ndusart commented Jul 27, 2017

Tests should pass now.

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

Just clean up this one doc comment and I think we're good here!

@@ -70,6 +101,59 @@ bitflags!(
}
);

// Flags for send/recv and their relatives
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc comment for this should be the first line inside the macro.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 27, 2017

Done, and converted these comments into doc comments

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

Also need some CHANGELOG entries. Under "Added" you should summarize which constants were added. And under "Changed" we have the API changes to socket and socketpair. We don't need to say that we switched to using libc-defined types, since our end users don't care, stick to summarizing things they would care about.

use ::sys::socket::addr::{AddressFamily};
use libc;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be fixed by modifying the following line to be use libc::{self, ...

@@ -9,9 +9,9 @@ use libc::{c_void, c_int, socklen_t, size_t, pid_t, uid_t, gid_t};
use std::{mem, ptr, slice};
use std::os::unix::io::RawFd;
use sys::uio::IoVec;
use libc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this use, combine it with the previous one.

@@ -1,9 +1,8 @@
use super::{ffi, consts, GetSockOpt, SetSockOpt};
use super::{ffi, GetSockOpt, SetSockOpt};
use libc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 27, 2017

Are these CHANGELOGS entries good ?

I merged the use statements.

CHANGELOG.md Outdated
@@ -34,6 +34,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- On Linux and Android, added support for receiving `PTRACE_O_TRACESYSGOOD`
events from `wait` and `waitpid` using `WaitStatus::PtraceSyscall`
([#566](https://github.com/nix-rust/nix/pull/566)).
- Added protocol families in `AddressFamily` enum.
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be moved above into the "Unreleased" section.

CHANGELOG.md Outdated
@@ -64,6 +66,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
"*_buf" variants also now calculate total array size and take slice references for improved type
safety. The documentation has also been dramatically improved.
([#670](https://github.com/nix-rust/nix/pull/670))
- Changed function signature of `socket()` and `socketpair()`. The `protocol` argument
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@Susurrus
Copy link
Contributor

Susurrus commented Aug 1, 2017

This needs a rebase, and you need to move the CHANGELOG entries when you do that. I'd also prefer that the last commit be merged into the other commits where appropriate instead of being a separate one, but this doesn't need to block merging this.

@ndusart
Copy link
Contributor Author

ndusart commented Aug 1, 2017

@Susurrus rebased on master

@Susurrus
Copy link
Contributor

Susurrus commented Aug 1, 2017

Awesome, thanks @ndusart!

bors r+

bors bot added a commit that referenced this pull request Aug 1, 2017
647: merge socket constants r=Susurrus

> Refactor the constant declarations such that all constants are only declared once with a #[cfg] that only enables the constant on the correct platforms.

Closes #637 

(same PR as #646 but from another branch, to see if buildbot has a problem with PR made from master branch)
@Susurrus Susurrus mentioned this pull request Aug 1, 2017
@bors
Copy link
Contributor

bors bot commented Aug 1, 2017

@bors bors bot merged commit c0c8e5e into nix-rust:master Aug 1, 2017
@ndusart ndusart deleted the socket-const branch August 1, 2017 19:54
pronebird pushed a commit to mullvad/pfctl-rs that referenced this pull request Aug 15, 2017
v0.2.29 adds missing IPPROTO_UDP bindings

See: nix-rust/nix#647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants