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

Implement nix wrapper for libc::signal #817

Merged
merged 1 commit into from
Jan 13, 2019
Merged

Implement nix wrapper for libc::signal #817

merged 1 commit into from
Jan 13, 2019

Conversation

rgardner
Copy link
Contributor

This implements nix::sys::signal::signal and adds corresponding documentation and tests.

Closes #476

asomers
asomers requested changes Dec 11, 2017
View reviewed changes
src/sys/signal.rs Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
rgardner
rgardner commented Dec 12, 2017
View reviewed changes
src/sys/signal.rs Outdated Show resolved Hide resolved
asomers
asomers reviewed Dec 13, 2017
View reviewed changes
src/sys/signal.rs Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
@rgardner
Copy link
Contributor Author

@asomers, could you review this again when you have some time? No rush - and happy holidays!

asomers
asomers reviewed Dec 19, 2017
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Please add something to the doc comment explaining that signal should only be used following another call to signal or if the current handler is the default. The return value of signal is undefined after setting the handler with sigaction.

test/sys/test_signal.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Jan 6, 2018

This looks good; it just needs a squash.

Susurrus
Susurrus suggested changes Jan 7, 2018
View reviewed changes
src/sys/signal.rs Outdated
///
/// Returns [`Error::UnsupportedOperation`] if `handler` is
/// [`SigAction`][SigActionStruct]. Use [`sigaction`][SigActionFn] instead.
/// `signal` also returns any error from `libc::signal`. such as when an attempt
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put a blank line above this one? These should be separate paragraphs since they're distinct comments.

Susurrus
Susurrus suggested changes Feb 8, 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.

Please also rebase this and move the CHANGELOG to the UNRELEASED section.

src/sys/signal.rs Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
@Susurrus
Copy link
Contributor

Susurrus commented Feb 8, 2018

I'm also not the biggest fan of the example since it's not a realistic one, it looks more like a test. Can you turn it into a more realistic example? Like something a user might actually write? If not, let's actually remove it. @asomers any ideas on a better example here?

asomers
asomers reviewed Feb 8, 2018
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

For a more realistic example, you could just remove the assert and remove the second line that sets the signal handler back to the default. Using signal to ignore SIGINT is a pretty realistic use case.

The only other realistic use case is probably to set a simple signal handler that sets a flag variable.

src/sys/signal.rs Show resolved Hide resolved
@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2018

@rgardner You planning to come back to this? We'd like to get this merged if you're willing. Otherwise I'd like to close this PR.

@rgardner
Copy link
Contributor Author

Yes, I too would like to get this merged. I'm unsure of exactly what to write in the safety section, I mostly chose unsafe out of inertia with the existing sigaction function above. Of the four unsafe superpowers called out in the book, "calling an unsafe function or method" applies. The transmuting of the previous signal handler is unsafe as the previous signal handler could be invalid - I'll start with that.

I'm open to suggestions for the safety section. Otherwise, I don't think this should block the PR, someone can add it in a follow up.

@Susurrus
Copy link
Contributor

Susurrus commented May 2, 2018

This needs the commits to be squashed to one commit, a rebase onto the latest master, and a re-run through CI to pass before it can be merged. Otherwise it looks good to me.

@asomers any comments on the Safety section?

asomers
asomers requested changes May 6, 2018
View reviewed changes
src/sys/signal.rs Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
test/sys/test_signal.rs Outdated Show resolved Hide resolved
test/sys/test_signal.rs Show resolved Hide resolved
@rgardner rgardner mentioned this pull request Jul 15, 2018
Closed
@rgardner
Copy link
Contributor Author

@asomers - friendly ping, could you take another look at this PR when you get a chance? Thank you for all the feedback.

@asomers
Copy link
Member

asomers commented Jan 13, 2019

Sorry that this fell off of my radar, @rgardner . It LGTM now. But it will need a rebase before it can be merged. Be careful when rebasing CHANGELOG to get your entry in the right section.

@rgardner
Copy link
Contributor Author

rgardner commented Jan 13, 2019 •
edited
Loading

No worries, fell off my radar too. I've rebased and ensured that the CHANGELOG entry is at the bottom of the Added section under [Unreleased].

@asomers
Copy link
Member

asomers commented Jan 13, 2019

bors r+

bors bot added a commit that referenced this pull request Jan 13, 2019
@bors @rgardner
Merge #817
7bcd9d2 
817: Implement nix wrapper for libc::signal r=asomers a=rgardner

This implements `nix::sys::signal::signal` and adds corresponding documentation and tests.

Closes #476

Co-authored-by: Robert Gardner <bob.hn.gardner@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jan 13, 2019

Build succeeded

@bors bors bot merged commit 6bff421 into nix-rust:master Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Susurrus Susurrus Susurrus approved these changes

@asomers asomers asomers approved these 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
@rgardner @asomers @Susurrus