-
Notifications
You must be signed in to change notification settings - Fork 676
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
Add sigprocmask #826
Add sigprocmask #826
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. In addition to addressing my inline comments, make sure you add a CHANGELOG entry.
src/sys/signal.rs
Outdated
/// Examine and change blocked signals. | ||
/// | ||
/// For more informations see the [`sigprocmask` man | ||
/// pages](http://man7.org/linux/man-pages/man2/sigprocmask.2.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a portable function, so you should link to a portable man page.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/sigprocmask.html
} | ||
|
||
#[test] | ||
fn test_sigprocmask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should grab SIGNAL_MTX
here so that this doesn't race with any other tests.
test/sys/test_signal.rs
Outdated
.expect("expect to be able to block signals"); | ||
|
||
// And test it again, to make sure the change was effective. | ||
sigprocmask(SigmaskHow::SIG_BLOCK, None, Some(&mut old_signal_set)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about emptying old_signal_set
before this line?
Addressed all feedback, you might want to squash the commits since the second doesn't have a good commit message. |
@Thomasdezeeuw Please go ahead and squash the commits on your end and force-push back to this branch. |
@Susurrus can't you use the squash and merge button on GitHub? Force pushing to Github never seems to work for me. |
We don't use GitHub to merge PRs, we rely on Bors, an external tool. It doesn't squash commits. So we rely on the PR authors to have their commits in order before we merge it. |
@Susurrus I didn't know that, squashed the commits so this is ready to go. |
It's somewhat mentioned in the CONYRIBUTING.MD document, but not very well. I've been meaning to update that actually as this comes up a lot. |
It seems the build bot is failing, it times out after 5 minutes and then fails to kill it.
And trying to digg in further fails to load (part of) the page. |
It looks like my VM suddenly got about 10 times slower. Problems with the VM host? Meltdown/Spectra countermeasures? I don't know. I've bumped the timeouts and I'm restarting the builds. |
bors r+ |
No description provided.