-
Notifications
You must be signed in to change notification settings - Fork 666
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
Conversation
There was a problem hiding this 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.
src/sys/socket/consts.rs
Outdated
@@ -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, |
There was a problem hiding this comment.
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.
src/sys/socket/consts.rs
Outdated
target_os = "ios", | ||
target_os = "openbsd", | ||
target_os = "netbsd"))] | ||
pub const SO_USELOOPBACK: c_int = libc::SO_USELOOPBACK; |
There was a problem hiding this comment.
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.
src/sys/socket/consts.rs
Outdated
target_os = "ios", | ||
target_os = "openbsd", | ||
target_os = "netbsd"))] | ||
pub const SO_NOSIGPIPE: c_int = 0x1022; |
There was a problem hiding this comment.
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
src/sys/socket/consts.rs
Outdated
target_os = "ios", | ||
target_os = "openbsd", | ||
target_os = "netbsd"))] | ||
pub const SO_NOADDRERR: c_int = 0x1023; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defined for macos/ios
src/sys/socket/consts.rs
Outdated
target_os = "ios", | ||
target_os = "openbsd", | ||
target_os = "netbsd"))] | ||
pub const SO_NWRITE: c_int = 0x1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defined for macos/ios
src/sys/socket/consts.rs
Outdated
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; |
There was a problem hiding this comment.
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.
src/sys/socket/consts.rs
Outdated
|
||
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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).
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 |
I could find the time for this :-) Just a bit confused where I could find |
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. |
@ndusart you'll have to go straight to the source. libc is built against the most recent releases of each of the BSDs. Here are some links you might find helpful:
|
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. |
src/sys/socket/consts.rs
Outdated
MSG_EOR, | ||
#[cfg(any(target_os = "linux", target_os = "android"))] |
There was a problem hiding this comment.
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]
^^
Worked a bit on this now. I can find
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 ? |
@ndusart One of the reasons we're upstreaming constants is because most of |
I have some difficulties with three constants for macos and ios:
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 Should we keep them hardcoded ? The values are correct. (Just |
Those constants don't actually exist on Mac as of 10.11.6 (I have one locally). I did |
src/sys/socket/consts.rs
Outdated
@@ -1,75 +1,253 @@ | |||
pub use self::os::*; | |||
|
|||
#[cfg(any(target_os = "linux", target_os = "android"))] | |||
mod os { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/sys/socket/consts.rs
Outdated
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
The constants that are not being defined in an enum are the socket options constants: My last commit remove the |
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
src/sys/socket/sockopt.rs
Outdated
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")] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/sys/socket/sockopt.rs
Outdated
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/sys/socket/mod.rs
Outdated
// Flags for send/recv and their relatives | ||
libc_bitflags!{ | ||
pub flags MsgFlags: libc::c_int { | ||
MSG_OOB, |
There was a problem hiding this comment.
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.
src/sys/socket/addr.rs
Outdated
#[cfg(any(target_os = "macos", target_os = "ios"))] | ||
System = consts::AF_SYSTEM, | ||
System = libc::AF_SYSTEM, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Travis is compiling |
a8cca80
to
5b0b047
Compare
@ndusart This commit is being merged: 5527ce8, which changes the version in the 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 |
Thanks @Wolvereness. 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 |
I added doc to Regarding the test |
@ndusart Yes, the tests are run. When you run |
Well, I think is quite done now. :) Do you have any remark on the current state of this PR ? |
Tests should pass now. |
There was a problem hiding this 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!
src/sys/socket/mod.rs
Outdated
@@ -70,6 +101,59 @@ bitflags!( | |||
} | |||
); | |||
|
|||
// Flags for send/recv and their relatives |
There was a problem hiding this comment.
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.
Done, and converted these comments into doc comments |
There was a problem hiding this 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.
src/sys/socket/addr.rs
Outdated
use ::sys::socket::addr::{AddressFamily}; | ||
use libc; |
There was a problem hiding this comment.
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, ...
src/sys/socket/mod.rs
Outdated
@@ -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; |
There was a problem hiding this comment.
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.
src/sys/socket/sockopt.rs
Outdated
@@ -1,9 +1,8 @@ | |||
use super::{ffi, consts, GetSockOpt, SetSockOpt}; | |||
use super::{ffi, GetSockOpt, SetSockOpt}; | |||
use libc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
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. |
@Susurrus rebased on master |
Awesome, thanks @ndusart! bors r+ |
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)
v0.2.29 adds missing IPPROTO_UDP bindings See: nix-rust/nix#647
Closes #637
(same PR as #646 but from another branch, to see if buildbot has a problem with PR made from master branch)