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

Move kqueue-related definitions from nix to libc #415

Merged
merged 8 commits into from
Oct 21, 2016

Conversation

asomers
Copy link
Member

@asomers asomers commented Sep 5, 2016

See rust-lang/libc#379 for the companion commit to libc. If and when that commit is merged, I'll update nix's Cargo.toml file and add it to this PR.

Change 11aa1f34243d5bbb7d6327a6607bd9d2530f3954 to libc added kqueue-related
definitions.  They are more accurate and more complete than nix's own
definitions.  Use them where possible.

Also, rationalize Nix's definitions so its public API will be as similar as
possible across all OSes.
@fiveop
Copy link
Contributor

fiveop commented Sep 5, 2016

Our master branch requires libc's github master branch. So you don't need to change our Cargo.toml.

The final version of libc's PR nix-rust#379 removed a few definitions, and fixed
OpenBSD's definition of fn kevent.
nevents: size_t,
timeout: *const timespec) -> c_int;
}
// Redefine kevent in terms of programmer-friendly enums and bitfields.
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 it would be preferable here to just have an inner libc::kevent struct. Then we don't have to duplicate the definition, and can get rid of the test that they match. This would require some setters though, for the enum / flags parts. I guess I should look at some kqueue code to see how it's used first. Never done any.

Copy link
Member

Choose a reason for hiding this comment

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

@fiveop any thoughts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that first. But I found it very inconvenient to require setters in my application code. It's much more convenient just to let ke = KEvent{whatever}

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of nix just wraps libc's structs and provides constructor fuctions:

pub struct NixStruct {
   struct: libc::struct
}
impl NixStruct {
   pub fn new(all the fields as parameters) {
     // do the dirty convertion here
   }
}

I like it and would like to keep it consistent.

Copy link
Member

Choose a reason for hiding this comment

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

One issue here is that there are 6 args. I think it works ok for things with 2 maybe 3. With 6 it becomes hard to know what they are from reading.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fiveop what about testing fields? It's easy to do

let k = KEvent{...}
...
match k.filter {
    nix.event.EVFILT_AIO => something
    _ => something else
}

but that becomes harder if the filter field is no longer an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

With an accessor you still have an enum. See for example EpollEvent::events().

Copy link
Member Author

Choose a reason for hiding this comment

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

@fiveop that's only because EpollEvent and EpollEventKind are both defined within Nix. But kevent is defined within libc, and libc doesn't do enums, only constants. That's why I redefined KEvent within nix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry. I forgot that my change to EpollEvent is not merged yet. You can see, how it can look like at https://github.com/nix-rust/nix/pull/410/files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok @fiveop I think I see what you want. I'll give it a shot.

@asomers
Copy link
Member Author

asomers commented Sep 25, 2016

@fiveop is this more like what you had in mind?

fiveop
fiveop previously requested changes Oct 2, 2016
Copy link
Contributor

@fiveop fiveop left a comment

Choose a reason for hiding this comment

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

Yes this looks good. There are some things that do not follow our conventions yet. Since they are not introduced by this PR, I would suggest fixing them in a separate PR.

Could you also update the CHANGELOG.md with respect to the changes made here? I think we only need to mention, that KEvent is now opaque with respect to its internal structure and field access has been replaced by public read only accessors (so far) and a constructor function.

Thank you for all the work so far.

type type_of_event_flag = u16;
#[cfg(any(target_os = "macos",
target_os = "freebsd",
target_os = "dragonfly"))]
bitflags!(
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 you can now use the libc_bitflags macro, that allows to reduce repetition. You can find examples elsewhere in nix.

Don't use it for FilterFlag, because it triggers recursion limit reached error
@homu
Copy link
Contributor

homu commented Oct 12, 2016

☔ The latest upstream changes (presumably bf00bf2) made this pull request unmergeable. Please resolve the merge conflicts.

@asomers
Copy link
Member Author

asomers commented Oct 15, 2016

All three build failures seem unrelated to this PR.

@kamalmarhubi
Copy link
Member

@asomers thanks!

@homu r+

@homu
Copy link
Contributor

homu commented Oct 20, 2016

📌 Commit 3ccb313 has been approved by kamalmarhubi

homu added a commit that referenced this pull request Oct 20, 2016
Move kqueue-related definitions from nix to libc

See rust-lang/libc#379 for the companion commit to libc.  If and when that commit is merged, I'll update nix's Cargo.toml file and add it to this PR.
@homu
Copy link
Contributor

homu commented Oct 20, 2016

⌛ Testing commit 3ccb313 with merge feb3742...

@homu
Copy link
Contributor

homu commented Oct 20, 2016

💔 Test failed - status

@asomers
Copy link
Member Author

asomers commented Oct 20, 2016

The failure is due to the gcc dependency not working with Rust 1.2. It is unrelated to this PR.

@kamalmarhubi kamalmarhubi dismissed fiveop’s stale review October 21, 2016 03:45

I think these were addressed.

@kamalmarhubi kamalmarhubi merged commit 45c8223 into nix-rust:master Oct 21, 2016
This was referenced Oct 21, 2016
@fiveop
Copy link
Contributor

fiveop commented Oct 21, 2016

I would have prefered to have waited until we get the build fully working again. A rebase instead of a merge from master would have also been nicer.

@kamalmarhubi
Copy link
Member

Sorry about that.

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