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.

Sign up for GitHub

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

Fix nix for Dragonfly #684

Closed
wants to merge 4 commits into from
Closed

Fix nix for Dragonfly #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3b0d08
Fix errno on DragonFly
mneumann Jul 17, 2017
3da33a9
Fix socket constants for DragonFly
mneumann Jul 17, 2017
afd178a
Use c_int instead of i32
mneumann Jul 17, 2017
d1e19d5
Do not run aio test cases on DragonFly
mneumann Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ bitflags = "0.9"
cfg-if = "0.1.0"
void = "1.0.2"

[target.'cfg(target_os = "dragonfly")'.dependencies]
errno-dragonfly = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need an extra crate just for this. The basic symbols should all be defined in libc, and nix's won src/errno.rs defines errno_location. Can you merge whatever you need into nix and libc so we don't depend on the extra crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there is no other possibility!!! I talked with @alexcrichton to make the thread_local attribute attribute stable, and he actually suggested me to write a C extension like errno-dragonfly. libc does not expose the errno variable directly AFAIK.

The problem is that in DragonFly, the errno variable is not a global, but a thread locale variable, so a simple "extern __errno" as used by the other OSes simply does not work. In libstd, errno can be accessed by using "#[thread_local]", as unstable features are allowed for libstd.

If you want to support DragonFly there really isn't any other option to go via C atm.

Copy link
Member

Choose a reason for hiding this comment

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

That's ... sad. I took a stab at this myself. It doesn't look as though we can use #[thread_local] unless we enable it for all builds, not just Dragonfly's. Still, what would you say to moving the C extension into Nix instead of making it its own crate? Cargo just seems like an awfully heavyweight way to manage such a tiny amount of code.

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 have a problem with this crate staying external, though I'd much rather rely on a crate that had proper semver (i.e. was 1.0+).


[dev-dependencies]
lazy_static = "0.2"
rand = "0.3.8"
Expand Down
10 changes: 4 additions & 6 deletions src/errno.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
#[cfg(not(target_os = "dragonfly"))]
use libc::c_int;
use std::{fmt, io, error};
use {Error, Result};

pub use self::consts::*;
pub use self::consts::Errno::*;

#[cfg(target_os = "dragonfly")]
use errno_dragonfly::errno_location;

#[cfg(any(target_os = "macos",
target_os = "ios",
target_os = "freebsd"))]
Expand All @@ -23,12 +27,6 @@ fn errno_location() -> *mut c_int {
}
}

#[cfg(target_os = "dragonfly")]
unsafe fn errno_location() -> *mut c_int {
extern { fn __dfly_error() -> *mut c_int; }
__dfly_error()
}

#[cfg(any(target_os = "openbsd", target_os = "netbsd"))]
unsafe fn errno_location() -> *mut c_int {
extern { fn __errno() -> *mut c_int; }
Expand Down
3 changes: 3 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ extern crate void;
#[cfg(test)]
extern crate nix_test as nixtest;

#[cfg(target_os = "dragonfly")]
extern crate errno_dragonfly;

#[macro_use] mod macros;

// In rust 1.8+ this should be `pub extern crate libc` but prior
Expand Down
87 changes: 4 additions & 83 deletions src/sys/socket/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,12 @@ mod os {
}

// Not all of these constants exist on freebsd
#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "ios", target_os = "openbsd", target_os = "netbsd"))]
#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "ios", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly"))]
mod os {
#[cfg(any(target_os = "macos",
target_os = "ios",
target_os = "freebsd"))]
target_os = "freebsd",
target_os = "dragonfly"))]
use libc::{self, c_int, uint8_t};
#[cfg(any(target_os = "openbsd", target_os = "netbsd"))]
use libc::{self, c_int, uint8_t};
Expand Down Expand Up @@ -193,7 +194,7 @@ mod os {
pub const TCP_MAXSEG: c_int = 2;
#[cfg(any(target_os = "macos", target_os = "ios"))]
pub const TCP_KEEPALIVE: c_int = libc::TCP_KEEPALIVE;
#[cfg(target_os = "freebsd")]
#[cfg(any(target_os = "freebsd", target_os = "dragonfly"))]
pub const TCP_KEEPIDLE: c_int = libc::TCP_KEEPIDLE;

// Socket options for the IP layer of the socket
Expand Down Expand Up @@ -237,86 +238,6 @@ mod os {
pub const SCM_RIGHTS: c_int = 1;
}

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

pub const AF_UNIX: c_int = libc::AF_UNIX;
pub const AF_LOCAL: c_int = libc::AF_LOCAL;
pub const AF_INET: c_int = libc::AF_INET;
pub const AF_INET6: c_int = libc::AF_INET6;

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;
pub const SOCK_RDM: c_int = libc::SOCK_RDM;

pub const SOL_SOCKET: c_int = libc::SOL_SOCKET;
pub const IPPROTO_IP: c_int = libc::IPPROTO_IP;
pub const IPPROTO_IPV6: c_int = libc::IPPROTO_IPV6;
pub const IPPROTO_TCP: c_int = libc::IPPROTO_TCP;
pub const IPPROTO_UDP: c_int = libc::IPPROTO_UDP;

pub const SO_ACCEPTCONN: c_int = libc::SO_ACCEPTCONN;
pub const SO_BROADCAST: c_int = libc::SO_BROADCAST;
pub const SO_DEBUG: c_int = libc::SO_DEBUG;
pub const SO_ERROR: c_int = libc::SO_ERROR;
pub const SO_DONTROUTE: c_int = libc::SO_DONTROUTE;
pub const SO_KEEPALIVE: c_int = libc::SO_KEEPALIVE;
pub const SO_LINGER: c_int = libc::SO_LINGER;
pub const SO_NOSIGPIPE: c_int = libc::SO_NOSIGPIPE;
pub const SO_OOBINLINE: c_int = libc::SO_OOBINLINE;
pub const SO_RCVBUF: c_int = libc::SO_RCVBUF;
pub const SO_RCVLOWAT: c_int = libc::RCVLOWAT;
pub const SO_SNDLOWAT: c_int = libc::SO_SNDLOWAT;
pub const SO_RCVTIMEO: c_int = libc::SO_RCVTIMEO;
pub const SO_SNDTIMEO: c_int = libc::SO_SNDTIMEO;
pub const SO_REUSEADDR: c_int = libc::SO_REUSEADDR;
pub const SO_REUSEPORT: c_int = libc::SO_REUSEPORT;
pub const SO_SNDBUF: c_int = libc::SO_SNDBUF;
pub const SO_TIMESTAMP: c_int = libc::SO_TIMESTAMP;
pub const SO_TYPE: c_int = libc::SO_TYPE;

// Socket options for TCP sockets
pub const TCP_NODELAY: c_int = libc::TCP_NODELAY;
pub const TCP_MAXSEG: c_int = libc::TCP_MAXSEG;
pub const TCP_KEEPIDLE: c_int = libc::TCP_KEEPIDLE;

// Socket options for the IP layer of the socket
pub const IP_MULTICAST_IF: c_int = 9;

pub type IpMulticastTtl = uint8_t;

pub const IP_MULTICAST_TTL: c_int = libc::IP_MULTICAST_TTL;
pub const IP_MULTICAST_LOOP: c_int = libc::IP_MULTICAST_LOOP;
pub const IP_ADD_MEMBERSHIP: c_int = libc::IP_ADD_MEMBERSHIP;
pub const IP_DROP_MEMBERSHIP: c_int = libc::IP_DROP_MEMBERSHIP;
pub const IPV6_JOIN_GROUP: c_int = libc::IPV6_JOIN_GROUP;
pub const IPV6_LEAVE_GROUP: c_int = libc::IPV6_LEAVE_GROUP;

pub type InAddrT = u32;

// Declarations of special addresses
pub const INADDR_ANY: InAddrT = 0;
pub const INADDR_NONE: InAddrT = 0xffffffff;
pub const INADDR_BROADCAST: InAddrT = 0xffffffff;

// Flags for send/recv and their relatives
libc_bitflags!{
pub flags MsgFlags: libc::c_int {
MSG_OOB,
MSG_PEEK,
MSG_DONTWAIT,
}
}

// shutdown flags
pub const SHUT_RD: c_int = libc::SHUT_RD;
pub const SHUT_WR: c_int = libc::SHUT_WR;
pub const SHUT_RDWR: c_int = libc::SHUT_RDWR;
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
5 changes: 4 additions & 1 deletion src/sys/socket/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ pub type type_of_cmsg_len = socklen_t;
#[cfg(target_os = "macos")]
pub type type_of_cmsg_data = c_uint;

#[cfg(not(target_os = "macos"))]
#[cfg(target_os = "dragonfly")]
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be #[cfg(any(target_os = "dragonfly", target_os = "macos"))]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is correct! type_of_cmsg_data is already defined above as c_uint. Here it is defined for DragonFly as c_int. Below it is defined for all other OSes.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're right. I didn't notice the "u".

pub type type_of_cmsg_data = c_int;

#[cfg(not(any(target_os = "macos", target_os = "dragonfly")))]
pub type type_of_cmsg_data = size_t;

// Private because we don't expose any external functions that operate
Expand Down
21 changes: 14 additions & 7 deletions src/sys/wait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,27 +145,34 @@ mod status {
target_os = "netbsd"))]
mod status {
use sys::signal::Signal;
use libc::c_int;
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary to change all of these types?

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 don't understand why you use i32 here instead of c_int. The latter is the correct type for those APIs, as waitpid returns a c_int, not a i32. On most platforms these types are identical, but c_int should be the correct one.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, if you want to be pedantically correct. But it's got nothing to do with DragonFly. It you're going to change those types, please do it in a separate commit, not comingled with DragonFly specific changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @asomers here, please remove this changes from this PR.


const WCOREFLAG: i32 = 0x80;
const WSTOPPED: i32 = 0x7f;
const WCOREFLAG: c_int = 0x80;
const WSTOPPED: c_int = 0x7f;

fn wstatus(status: i32) -> i32 {
fn wstatus(status: c_int) -> c_int {
status & 0x7F
}

pub fn stopped(status: i32) -> bool {
pub fn stopped(status: c_int) -> bool {
wstatus(status) == WSTOPPED
}

pub fn stop_signal(status: i32) -> Signal {
pub fn stop_signal(status: c_int) -> Signal {
Signal::from_c_int(status >> 8).unwrap()
}

pub fn signaled(status: i32) -> bool {
#[cfg(target_os = "dragonfly")]
pub fn signaled(status: c_int) -> bool {
wstatus(status) != WSTOPPED && wstatus(status) != 0
}

#[cfg(not(target_os = "dragonfly"))]
pub fn signaled(status: c_int) -> bool {
wstatus(status) != WSTOPPED && wstatus(status) != 0 && status != 0x13
Copy link
Member

Choose a reason for hiding this comment

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

What does the magic number 0x13 mean? This sounds like a constant that should be in libc.

}

pub fn term_signal(status: i32) -> Signal {
pub fn term_signal(status: c_int) -> Signal {
Signal::from_c_int(wstatus(status)).unwrap()
}

Expand Down
8 changes: 7 additions & 1 deletion test/sys/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
mod test_signal;
#[cfg(any(target_os = "freebsd", target_os = "dragonfly", target_os = "ios",

// NOTE: DragonFly lacks a kernel-level implementation of Posix AIO
// as of this writing, while there is an user-level implementation,
// but whether aio works or not heavily depends on which pthread
// implementation is chosen by the user at link time. For this
// reason we do not want to run aio test cases on DragonFly.
#[cfg(any(target_os = "freebsd", target_os = "ios",
target_os = "netbsd", target_os = "macos", target_os = "linux"))]
mod test_aio;
mod test_socket;
Expand Down