Skip to content

/ ipc-channel

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

Support Freebsd #102

Merged
merged 6 commits into from Oct 25, 2016
Merged

Support Freebsd #102

merged 6 commits into from Oct 25, 2016
+56 −46

Conversation

@dlrobertson
Copy link
Collaborator

dlrobertson commented Sep 18, 2016

Add support for FreeBSD and rename linux module to unix.

NB: Some tests are still currently failing on freebsd. I believe most of which have to do with system limits e.g. (platform::medium_data blocks on a send_followup_fragment due to system limits and never recovers).
@dlrobertson
dlrobertson reviewed Sep 18, 2016
View changes
Copy link
Collaborator Author

dlrobertson left a comment

Some brief comments on things changed.
src/platform/unix/mod.rs
@@ -26,18 +26,64 @@ use std::thread;

const MAX_FDS_IN_CMSG: u32 = 64;

const SCM_RIGHTS: c_int = 0x01;

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 18, 2016

Author Collaborator

Moved static and const delarations to the top
src/platform/unix/mod.rs Outdated
fn CMSG_ALIGN(length: size_t) -> size_t {
(length + mem::size_of::<size_t>() - 1) & !(mem::size_of::<size_t>() - 1)
#[cfg(target_os = "freebsd")]
unsafe fn CMSG_DATA(cmsg: *mut cmsghdr) -> *mut c_void {

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 18, 2016 •
edited

Author Collaborator

Ensure proper alignment. cmsg.offset(1) on my box is the equivalent of addq 0xc, %rcx, but it should be addq 0x10, %rcx.
src/platform/unix/mod.rs
(*cmsg_buffer).cmsg_level = libc::SOL_SOCKET;
(*cmsg_buffer).cmsg_type = SCM_RIGHTS;

ptr::copy_nonoverlapping(fds.as_ptr(),
cmsg_buffer.offset(1) as *mut c_int,
CMSG_DATA(cmsg_buffer) as *mut c_int,

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 18, 2016

Author Collaborator

Use CMSG_DATA instead of ptr::offset
@antrik
Copy link
Contributor

antrik commented Sep 18, 2016

@danlrobertson I'm not sure if/when I get to look at the changes in detail; so some quick comments for now:

  1. Formalities: I believe the changes you commented on here should actually be separate commits, with the comments as commit messages. The comments here won't be terribly useful when someone tries to make sense of the Git changelog at some point in the future...
  2. The CMSG_DATA/CMSG_ALIGN/CMSG_LEN macros are direct copies of macros from Linux-specific bits of glibc, only translated to Rust -- so they obviously need system-specific variants. The code invoking them on the other hand should be portable I think...
  3. This might be considered out of scope -- but I have been thinking for a while that these extremely system-specific details should probably not be present in the code of ipc-channel at all, but rather covered by external crates, such as nix and/or libc -- though quite probably this will require some upstream enhancements to said crates...
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Sep 18, 2016

@antrik thanks for the quick reply. Don't feel rushed.

  1. Shouldn't be a problem.

  2. The changes I made to the FreeBSD specific CMSG_DATA function was also a translation of the C macro. I don't see how the current code can be written to be more portable without something along the lines of 3.

  3. I completely agree. I was surprised this wasn't in libc
@l0kod
Copy link

l0kod commented Sep 19, 2016

About the CMSG_DATA, you may be interested in https://github.com/stemjail/fdpass-rs
@dlrobertson dlrobertson force-pushed the dlrobertson:support-freebsd branch from 65c0e76 to 6e68729 Sep 19, 2016
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Sep 19, 2016 •
edited

@antrik: Updated to include 2 commits. fbafaf1 move static and const declarations and 6e68729 support freebsd

@l0kod: per stemjail/fdpass-rs#1, it doesn't seem to be currently working on FreeBSD. Let me know if missed something. A transition to a library like fdpass-rs I think is a great idea (and the path forward), but imo that is currently outside the scope of this PR. That being said, I'm happy to look into it further if the consensus is to do so.
@antrik
Copy link
Contributor

antrik commented Sep 20, 2016

@l0kod I briefly considered fdpass-rs when I first read about it; but it seems to me nix with its much broader scope would be more appropriate for ipc-channel...
@antrik
antrik reviewed Sep 26, 2016
View changes
Copy link
Contributor

antrik left a comment

Personally, I'd prefer even finer-grained commits, with changes such as "rename platform/linux to platform/unix", "introduce (and use) CMSG_DATA()", "factor out new_sockaddr_un()", "use socklen_t", "create sockaddr in a platform-agnostic fashion" etc. all as separate patches... But then again, I'm a confirmed perfectionist, so my standards probably shouldn't be considered normative ;-)
src/platform/unix/linux/mod.rs Outdated
@@ -26,18 +26,64 @@ use std::thread;

const MAX_FDS_IN_CMSG: u32 = 64;

const SCM_RIGHTS: c_int = 0x01;

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016 •
edited

Contributor

To be honest, I'm not convinced moving these defines to the top really is an improvement... Personally I prefer definitions to be close to usage, rather than strictly following formal rules such as "all const should go to the top". But I'm aware that I might be in a minority here :-(

(Also, this is a bit off-topic I guess, but I'm still wondering why this one is not defined in libc?...)

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

It apears as if everything from cmsg is not defined in libc. It definitely seems like they should be defined somewhere like libc or nix.

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

libc I'd say. libc is for the raw FFI interfaces; while nix is for safe wrappers around them. Using nix to abstract all these dirty details would be nice to have, but it's a second (much bigger) step...
src/platform/unix/linux/mod.rs Outdated
// The value Linux returns for SO_SNDBUF
// is not the size we are actually allowed to use...
// Empirically, we have to deduct 32 bytes from that.
const RESERVED_SIZE: usize = 32;

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016 •
edited

Contributor

Have you checked that this is actually needed on FreeBSD as well? It's the kind of thing I would have expected to be extremely system-specific...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

I want to say yes, but I'll double check.
src/platform/unix/linux/mod.rs Outdated
#[cfg(target_os="linux")]
type IovLen = usize;
#[cfg(target_os="linux")]
type SockLen = usize;

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016 •
edited

Contributor

Can't we just use socklen_t everywhere?

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

That's what I was hoping. However, socklen_t:linux = u32 but msghdr.msg_controllen = size_t = usize. This works on FreeBSD though, and was what I originally did. Perhaps this is another change that should be made to libc

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 29, 2016

Contributor

Ah, so for some reason the POSIX committee decided to abuse socklen_t for something that is clearly not in any way related to socket address lengths; while glibc decided to stick with the more appropriate size_t... This is so retarded :-(

Calling the type SockLen is very confusing though, as it has no bearing on the primary use of socklen_t -- better call it MsgControllen, or something along these lines. Also, it's probably better to define it as size_t rather than usize, to match the libc definition.
src/platform/unix/linux/mod.rs Outdated
#[cfg(target_os="linux")]
type SockLen = usize;
#[cfg(target_os="linux")]
#[inline]

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

I'd leave that decision to the compiler...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

👍
src/platform/unix/linux/mod.rs Outdated
let mut sockaddr = sockaddr_un {
sun_family: libc::AF_UNIX as u16,
sun_path: [ 0; 108 ],
};

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

I suspect we could make this code generic, by using mem::zeroed() and then setting only the fields we need, just as we would in C? (Along with using sa_family_t to make the cast generic -- though I wonder why AF_UNIX isn't defined as sa_family_t in libc in the first place...)

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

I'll give it a shot.
src/platform/unix/linux/mod.rs Outdated
libc::strncpy(sockaddr.sun_path.as_mut_ptr(),
path,
sockaddr.sun_path.len() - 1);
let len = mem::size_of::<sockaddr_un>() - 104 + (libc::strlen(sockaddr.sun_path.as_ptr()) as usize);

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016 •
edited

Contributor

This doesn't look right -- copy&paste error perhaps?...

I think we could make that code (and thus the entire function) generic as well. It will be uglier than in C due to the lack of offsetof; but it should be doable with &sockaddr.sun_path as *const as usize - &sockaddr as *const as usize or something along these lines?...

BTW, the example in the Linux man page for using AF_UNIX sockets just passes sizeof sockaddr_un as the len parameter to connect(), rather than calculating the actual size in use... The POSIX man page for connect() is a bit unclear what is expected here: it says "the length of the sockaddr structure pointed to by the address argument". I'd tend to assume passing the entire size is probably correct... Which will make the code simpler.

(I suggest changing this in a separate commit before the main change.)

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

Yes, this is an error. linux would be - 108, while freebsd would be - 104
src/platform/unix/linux/mod.rs Outdated
@@ -953,21 +976,32 @@ fn is_socket(fd: c_int) -> bool {

// FFI stuff follows:

const SCM_RIGHTS: c_int = 0x01;
#[allow(non_snake_case)]
fn CMSG_LEN(length: SockLen) -> SockLen {

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

I don't think it's right for these functions to work on SockLen: I'd say they should stick with size_t; while the cast to socklen_t would only be done by the caller while filling the actual socket structures. This feels more natural to me, and avoids a lot of unnecessary confusing casts.

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

Agreed 👍
src/platform/unix/linux/mod.rs Outdated
CMSG_ALIGN(mem::size_of::<cmsghdr>()) + length
#[cfg(target_os = "linux")]
unsafe fn CMSG_DATA(cmsg: *mut cmsghdr) -> *mut c_void {
cmsg.offset(1) as *mut c_void

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

While this matches the glibc definition of CMSG_DATA, I think it wouldn't hurt to just use FreeBSD definition everywhere? AIUI, Linux only gets away with the simplistic definition, because sizeof cmsghdr happens to always have the right alignment there. This is inconsistent with the other CMSG_ definitions though, which are defined in a more generic way, always explicitly doing the alignment...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

Didn't think about that. That would make this a bit more simplistic. I'd definitely be okay with making this change
src/platform/unix/mod.rs Outdated
@@ -0,0 +1,1029 @@
// Copyright 2015 The Servo Project Developers. See the COPYRIGHT

This comment has been minimized.

Sign in to view
@antrik

antrik Sep 26, 2016

Contributor

Something went wrong with your commit: it introduces both src/platform/unix/mod.rs and src/platform/unix/linux/mod.rs (with identical contents) in place of the original src/platform/linux/mod.rs...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Sep 26, 2016

Author Collaborator

I'll fix this in the update... My bad.
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Sep 26, 2016

Thanks for the very thorough review. I'll make the changes and comment when I make the update. I'll also start looking into a better home for cmsg
@dlrobertson
Move static and const declarations
Unverified
The committer email address is not verified.
Learn about signing commits
392bc21 
Move the static and const declarations to the top of the file
@dlrobertson dlrobertson force-pushed the dlrobertson:support-freebsd branch from 6e68729 to 7e2c42e Sep 30, 2016
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Sep 30, 2016

Personally, I'd prefer even finer-grained commits

Updated, and I must admit you were right... It makes a lot more sense this way.

Calling the type SockLen is very confusing though.. better call it MsgControllen

Updated.

Also, it's probably better to define it as size_t rather than usize, to match the libc definition.

Updated.

I suspect we could make this code [new_sockaddr_un] generic, by using mem::zeroed() and then setting only the fields we need

Updated.
@dlrobertson dlrobertson force-pushed the dlrobertson:support-freebsd branch from 7e2c42e to 95dccff Sep 30, 2016
@antrik
antrik reviewed Oct 5, 2016
View changes
src/platform/mod.rs
#[cfg(all(not(feature = "force-inprocess"), target_os = "linux"))]
include!("linux/mod.rs");
#[cfg(all(not(feature = "force-inprocess"), any(target_os = "linux",
target_os = "freebsd")))]

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 5, 2016 •
edited

Contributor

Personally, I'd add the freebsd case only right before or along with actually adding FreeBSD-specific code, after the generic portability improvements... But this way is fine too I guess :-)

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 11, 2016

Author Collaborator

Not sure what you mean by this. Do you mean use something like a all(unix,not(macos)) instead of specifying linux and freebsd for using the unix module?

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 19, 2016

Contributor

No, I was only referring to how changes are split up into patches, rather than the final code. (Sorry for being so pedantic about this... It's really not important; just a suggestion :-) )

Having said that, I already pondered something along the lines of what you said here. I'm honestly not sure. Normally, I'd prefer this approach indeed; but in this specific case, we need to handle each platform explicitly anyway because of the MsgControllen ugliness :-( It might still be a win though -- especially if this can be handled at the libc level at some point?...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 25, 2016

Author Collaborator

Updated with the suggested patch splits.

Agreed. In the future we could/should be able to get away with just unix.
src/platform/unix/mod.rs Outdated
unsafe fn new_sockaddr_un(path: *const c_char) -> (sockaddr_un, usize) {
let mut sockaddr: sockaddr_un = mem::zeroed();
libc::strncpy(sockaddr.sun_path.as_mut_ptr(),
path, sockaddr.sun_path.len() - 1);
sockaddr.sun_family = libc::AF_UNIX as u16;
sockaddr.sun_family = libc::AF_UNIX as SunFamily;

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 5, 2016

Contributor

Can't you just use sa_family_t here as I suggested?...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 11, 2016

Author Collaborator

Yup. Good point.
src/platform/unix/mod.rs Outdated
@@ -961,7 +954,7 @@ fn CMSG_LEN(length: size_t) -> size_t {
}

#[allow(non_snake_case)]
fn CMSG_DATA(cmsg: *mut cmsghdr) -> *mut c_void {
unsafe fn CMSG_DATA(cmsg: *mut cmsghdr) -> *mut c_void {

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 5, 2016

Contributor

I think that belongs with the previous commit?...

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 11, 2016

Author Collaborator

My bad
src/platform/unix/mod.rs Outdated
let mut sockaddr: sockaddr_un = mem::zeroed();
libc::strncpy(sockaddr.sun_path.as_mut_ptr(),
path, sockaddr.sun_path.len() - 1);
sockaddr.sun_family = libc::AF_UNIX as u16;

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 5, 2016 •
edited

Contributor

I'd split the commits a bit differently: first only split out new_sockaddr_un(), without any change to the actual code moved in there, so it's a pure refactoring; and only after that one or more commits that actually make it platform-agnostic, by introducing mem::zeroed(), sa_family_t, etc.

(Not insisting though -- probably it's clear enough that way as well...)

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 11, 2016

Author Collaborator

Done! in the latest batch of commits
src/platform/unix/mod.rs Outdated
path, sockaddr.sun_path.len() - 1);
sockaddr.sun_family = libc::AF_UNIX as u16;
let len = mem::size_of::<c_short>() + (libc::strlen(sockaddr.sun_path.as_ptr())
as usize);

This comment has been minimized.

Sign in to view
@antrik

antrik Oct 5, 2016

Contributor

While this happens to work on Linux and FreeBSD, I don't think it's really generic... As I said, this would probably require some pointer arithmetic -- but I still think the right way to handle this is to just use sizeof sockaddr_un, like in the Linux man page example, rather than trying to determine the actually used size dynamically.

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Oct 11, 2016

Author Collaborator

Good point. Used sizeof(sockaddr_un) in the latest batch of commits.
@dlrobertson dlrobertson force-pushed the dlrobertson:support-freebsd branch 4 times, most recently from 601b464 to b9c149d Oct 8, 2016
@antrik
Copy link
Contributor

antrik commented Oct 19, 2016

Apart from the pending question regarding handling of platform conditionals, I'd say this is good to go :-)
@myfreeweb
Copy link

myfreeweb commented Oct 21, 2016

Thank you for doing this work! Got Webrender working on FreeBSD :)
dlrobertson added 5 commits Sep 29, 2016
@dlrobertson
Rename platform linux module to unix
Unverified
The committer email address is not verified.
Learn about signing commits
5b5db80 
Rename platform/linux to platform/unix in preparation to adding support
for FreeBSD.
@dlrobertson
Add FreeBSD specific conditionals
Unverified
The committer email address is not verified.
Learn about signing commits
9b6915e
@dlrobertson
Add and use CMSG_DATA
Unverified
The committer email address is not verified.
Learn about signing commits
4b25084 
Instead of directly using ptr::offset and the equivalent of CMSG_DATA in
glibc on Linux, create a CMSG_DATA function that should work on Linux
and FreeBSD. This should not cause any side effects, but should make it
easier if we should introduce support for another OS that may have a
different implementation of CMSG_DATA.
@dlrobertson
Add new_sockaddr_un
Unverified
The committer email address is not verified.
Learn about signing commits
c63c8b1 
Add the new_sockaddr_un function that builds a sockaddr_un struct in a
platform agnostic way.
@dlrobertson
Add OS specific types
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
4886205 
Add OS specific types that address alignment problems and their
respective libc definitions.
@dlrobertson dlrobertson force-pushed the dlrobertson:support-freebsd branch from b9c149d to 4886205 Oct 25, 2016
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Oct 25, 2016

Updated with the suggested patch splits.
@antrik
Copy link
Contributor

antrik commented Oct 25, 2016

Great, let's get this landed :-)
@pcwalton
Copy link
Collaborator

pcwalton commented Oct 25, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 25, 2016

📌 Commit 4886205 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Oct 25, 2016

⌛ Testing commit 4886205 with merge 3250a60...
bors-servo added a commit that referenced this pull request Oct 25, 2016
@bors-servo
Auto merge of #102 - danlrobertson:support-freebsd, r=pcwalton
Loading status checks…
3250a60 
Support Freebsd

Add support for FreeBSD and rename `linux` module to `unix`.

**NB:** Some tests are still currently failing on freebsd. I believe most of which have to do with system limits e.g. (`platform::medium_data` blocks on a `send_followup_fragment` due to system limits and never recovers).
@bors-servo
Copy link
Contributor

bors-servo commented Oct 25, 2016

☀️ Test successful - status-appveyor, status-travis
@bors-servo bors-servo merged commit 4886205 into servo:master Oct 25, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Oct 25, 2016
Closed
Merged
Closed
Merged
@dlrobertson dlrobertson mentioned this pull request Nov 8, 2016
Open
@dlrobertson dlrobertson deleted the dlrobertson:support-freebsd branch Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antrik antrik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@dlrobertson @antrik @l0kod @myfreeweb @pcwalton @bors-servo
You can’t perform that action at this time.