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

Jump to bottom

FreeBSD: cfmakesane, EVFILT_* #825

Merged
merged 3 commits into from
Feb 21, 2018
Merged

FreeBSD: cfmakesane, EVFILT_* #825

merged 3 commits into from
Feb 21, 2018

Conversation

valpackett
Copy link
Contributor

Depends on: rust-lang/libc#887

Susurrus
Susurrus suggested changes Jan 4, 2018
View reviewed changes
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 needs a CHANGELOG entry.

src/sys/event.rs Outdated
@@ -39,6 +39,8 @@ libc_enum! {
#[cfg_attr(not(target_os = "netbsd"), repr(i16))]
pub enum EventFilter {
EVFILT_AIO,
#[cfg(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.

Each one of these need doccomments describing them.

@asomers
Copy link
Member

asomers commented Jan 4, 2018

Thanks for your submission! Have you checked the other BSDs to see how many define these symbols too? I see that cfmakesane is in Dragonfly, but I haven't checked any of the others.

@valpackett valpackett force-pushed the freebsd-missing-stuff branch from 6c4e055 to d550555 Compare January 4, 2018 00:21
@valpackett
Copy link
Contributor Author

valpackett commented Jan 4, 2018 •
edited
Loading

@asomers the libc crate only exposes it on FreeBSD… I'll add it there for DragonFly.

@Susurrus added.

@valpackett valpackett force-pushed the freebsd-missing-stuff branch 2 times, most recently from 7ea2152 to d550555 Compare January 4, 2018 00:33
@valpackett
Copy link
Contributor Author

cfmakesane was added to DragonFly last December, not in any release yet. It was in FreeBSD for seven years.

@asomers
Copy link
Member

asomers commented Jan 4, 2018

Ok. What about EVFILT_EMPTY ?

@valpackett
Copy link
Contributor Author

I have documented _EMPTY. There's nothing in the manpage for _SENDFILE.

(hmm, none of the other values have been documented here before…)

@Susurrus
Copy link
Contributor

Susurrus commented Jan 4, 2018

@myfreeweb Yes, there has been a severe lack of documentation within nix in the past, so we're trying to fix that as we go. It'd be much appreciated if you could add doccomments for the other constants that are there since you're already digging through these docs.

@valpackett
Copy link
Contributor Author

Yeah I'll write some docs there soon.

+ one more addition: fix for write_int ioctls. On FBSD & DFly they're defined with an IOWINT macro that actually uses the VOID type instead of IN, so wrong numbers were generated.

@Susurrus
Copy link
Contributor

Susurrus commented Jan 4, 2018

I'm currently rewriting the macro logic because of the conversation in #824 and #781, so let's skip the ioctl stuff for this PR. I should have my rewrite done by this weekend and I'll CC you on the PR to review that it corrects things for the BSDs as well.

Susurrus
Susurrus suggested changes Jan 28, 2018
View reviewed changes
src/sys/event.rs Outdated
@@ -51,7 +54,15 @@ libc_enum! {
#[cfg(any(target_os = "ios", target_os = "macos"))]
EVFILT_MACHPORT,
EVFILT_PROC,
/// Returns events associated with the process referenced by a given
/// process descriptor, created by pdfork(2). The events to monitor are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Backticks needed around pdfork() (leave out the "2").

src/sys/event.rs Outdated
EVFILT_READ,
#[cfg(target_os = "freebsd")]
EVFILT_SENDFILE,
Copy link
Contributor

Choose a reason for hiding this comment

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

@asomers Could you help us dig up some docs for this constant?

Copy link
Member

Choose a reason for hiding this comment

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

How about "indicates the completion of an asynchronous sendfile(2) call". I'm afraid be more specific than that.

src/sys/ioctl/mod.rs Outdated
@@ -325,7 +325,7 @@ macro_rules! ioctl {
pub unsafe fn $name(fd: $crate::libc::c_int,
data: $crate::libc::c_int)
-> $crate::Result<$crate::libc::c_int> {
convert_ioctl_res!($crate::libc::ioctl(fd, iow!($ioty, $nr, ::std::mem::size_of::<$crate::libc::c_int>()) as $crate::sys::ioctl::ioctl_num_type, data))
convert_ioctl_res!($crate::libc::ioctl(fd, iowint!($ioty, $nr, ::std::mem::size_of::<$crate::libc::c_int>()) as $crate::sys::ioctl::ioctl_num_type, data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the ioctl changes from this PR, please comment in #833 with a brief description and some test cases that fail on FreeBSD (see test/sys/test_ioctl.rs for existing test cases and please confirm which ones fail). @asomers could you help out here as well?

src/sys/termios.rs Outdated
///
/// Note that this is a non-standard function, available on FreeBSD.
#[cfg(target_os = "freebsd")]
pub fn cfmakesane(termios: &mut Termios) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a link to the man pages.

@valpackett valpackett mentioned this pull request Jan 28, 2018
Merged
@valpackett valpackett force-pushed the freebsd-missing-stuff branch 2 times, most recently from 4a042fe to 589cc50 Compare January 28, 2018 20:07
@valpackett
Copy link
Contributor Author

Removed the ioctl fix here, updated comments.

rust-lang/libc#887 has landed, but not in a release yet.

@Susurrus
Copy link
Contributor

Please move the CHANGELOG entries into the UNRELEASED section, as they weren't a part of the 0.10.0 release. Then it's LGTM.

@valpackett valpackett force-pushed the freebsd-missing-stuff branch from 589cc50 to 463511b Compare January 29, 2018 10:40
@valpackett
Copy link
Contributor Author

done

@Susurrus
Copy link
Contributor

Looks like it'll need another rebase to deal with changes to the CHANGELOG file. When you do please post back here, as I don't get an email on changes to PRs that aren't comments.

@valpackett valpackett force-pushed the freebsd-missing-stuff branch from 463511b to 647bac7 Compare February 12, 2018 09:53
@valpackett
Copy link
Contributor Author

rebased

@Susurrus
Copy link
Contributor

Thanks for your contribution!

bors r+

bors bot added a commit that referenced this pull request Feb 21, 2018
@bors
Merge #825
2b9c67c 
825: FreeBSD: cfmakesane, EVFILT_* r=Susurrus a=myfreeweb

Depends on: rust-lang/libc#887
@bors
Copy link
Contributor

bors bot commented Feb 21, 2018

Build succeeded

@bors bors bot merged commit 647bac7 into nix-rust:master Feb 21, 2018
@petehayes102 petehayes102 mentioned this pull request Jun 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers left review comments

@Susurrus Susurrus Susurrus requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@valpackett @asomers @Susurrus