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

fcntl: Use bindings from libc instead of our own #354

Merged
merged 1 commit into from
Apr 23, 2016
Merged

fcntl: Use bindings from libc instead of our own #354

merged 1 commit into from
Apr 23, 2016

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Apr 15, 2016

Un-finished and looking for help. As #264 was tagged "mentor, good first bug"

Ref #264

I couldn't find F_GET_SEALS, or F_ADD_SEALS in libc.

Is this a case where I should be making a PR over there? I'm not sure where exactly to address it in libc.

@kamalmarhubi
Copy link
Member

Hi @brianp, thanks for the PR! Taking a look now!

@kamalmarhubi
Copy link
Member

Oh re F_GET_SEAL and F_SET_SEALS in libc, this is blocked by needing to figure out rust-lang/libc#235

pub const F_SETLK: c_int = 6;
pub const F_SETLKW: c_int = 7;
pub const F_GETLK: c_int = 5;
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.

no need for braces if just importing one name

@kamalmarhubi
Copy link
Member

kamalmarhubi commented Apr 18, 2016

I think if you make the change with the libc import, the I think the build should pass. I'm still not sure exactly what to do with F_{GET,SET}_SEALS, but I'll ping issue rust-lang/libc#235.

@brianp
Copy link
Contributor Author

brianp commented Apr 19, 2016

Thanks for the reply. If I can help out with rust-lang/libc#235 in any way let me know!

I'll tidy up this pr but will still leave F_{GET,SET}_SEALS until the above is addressed.

#[cfg(target_os = "netbsd")]
pub const F_SETNOSIGPIPE: c_int = 14;
}
pub const F_ADD_SEALS: c_int = 1033;
Copy link
Member

Choose a reason for hiding this comment

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

add a TODO explaining why these are left here, and reference rust-lang/libc#235

@kamalmarhubi
Copy link
Member

one small comment, otherwise this looks good. could you squash your commits down so we don't have the broken one in our history?

@brianp
Copy link
Contributor Author

brianp commented Apr 20, 2016

Happy to! I'll take care of it now.

@kamalmarhubi
Copy link
Member

Thanks @brianp!

@homu r+ 7568340

homu added a commit that referenced this pull request Apr 23, 2016
fcntl: Use bindings from libc instead of our own

**Un-finished** and looking for help. As #264 was tagged "mentor, good first bug"

Ref #264

I couldn't find `F_GET_SEALS`, or `F_ADD_SEALS` in libc.

Is this a case where I should be making a PR over there? I'm not sure where exactly to address it in libc.
@homu
Copy link
Contributor

homu commented Apr 23, 2016

⌛ Testing commit 7568340 with merge d2c1263...

@homu
Copy link
Contributor

homu commented Apr 23, 2016

☀️ Test successful - status

@homu homu merged commit 7568340 into nix-rust:master Apr 23, 2016
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.

None yet

3 participants