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

Update signalfd #309

Merged
merged 1 commit into from
Mar 11, 2016
Merged

Update signalfd #309

merged 1 commit into from
Mar 11, 2016

Conversation

alex-gulyas
Copy link
Contributor

Update signalfd to use types and functions from libc.

It is a breaking change, because:

  • renamed pub const CREATE_NEW_FD to SIGNALFD_NEW
  • renamed pub const SIGINFO_SIZE to SIGNALFD_SIGINFO_SIZE
  • removed pub const SIGINFO_PADDING

Fixes #307

flags SfdFlags: c_int {
const SFD_NONBLOCK = 0o00004000, // O_NONBLOCK
const SFD_CLOEXEC = 0o02000000, // O_CLOEXEC
flags SfdFlags: 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.

In most code we bring c_int and other common types into the module namespace rather than accessing through libc -- not sure if this is worth promoting as a convention or if it is an important style element (obviously, it makes no functional difference). CC @kamalmarhubi

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not have a convention on this yet. I would prefer module level imports, because they are shorter in general. This is a purely aesthetic argument, and thus not very strong. But I have no other arguments for or against either option.

This does, however, not have any relevance for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I've just been following the convention in the file. I don't have strong feelings in either direction. For common types, I agree module level is shorter, and for nix we're pretty clear on where they would be coming from.

@posborne
Copy link
Member

LGTM, but I'll let another reviewer in. r? @kamalmarhubi

@fiveop
Copy link
Contributor

fiveop commented Mar 11, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Mar 11, 2016

📌 Commit 19ad8ca has been approved by fiveop

@homu
Copy link
Contributor

homu commented Mar 11, 2016

⌛ Testing commit 19ad8ca with merge cb6a921...

homu added a commit that referenced this pull request Mar 11, 2016
Update signalfd

Update `signalfd` to use types and functions from `libc`.

It is a breaking change, because:
- renamed pub const `CREATE_NEW_FD` to `SIGNALFD_NEW`
- renamed pub const `SIGINFO_SIZE` to `SIGNALFD_SIGINFO_SIZE`
- removed pub const `SIGINFO_PADDING`

Fixes #307
@kamalmarhubi
Copy link
Member

Thanks @alex-gulyas for noticing and fixing this!

@homu
Copy link
Contributor

homu commented Mar 11, 2016

☀️ Test successful - status

@homu homu merged commit 19ad8ca into nix-rust:master Mar 11, 2016
@alex-gulyas alex-gulyas deleted the update-signalfd branch March 11, 2016 14:11
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.

5 participants