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

Allow to specify signal when calling clone. #344

Merged
merged 1 commit into from
Apr 21, 2016
Merged

Allow to specify signal when calling clone. #344

merged 1 commit into from
Apr 21, 2016

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Apr 7, 2016

This is my suggestion on how to fix #343.

@@ -197,15 +198,16 @@ pub fn sched_setaffinity(pid: isize, cpuset: &CpuSet) -> Result<()> {
Errno::result(res).map(drop)
}

pub fn clone(mut cb: CloneCb, stack: &mut [u8], flags: CloneFlags) -> Result<pid_t> {
pub fn clone(mut cb: CloneCb, stack: &mut [u8], flags: CloneFlags, signal: Option<c_int>) -> Result<pid_t> {
Copy link
Member

Choose a reason for hiding this comment

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

Signal should be constrained to an 8-bit unsigned value (u8/c_char) at compile time given the allowable mask for signal: http://lxr.free-electrons.com/source/include/uapi/linux/sched.h#L7. Other values could clobber the other flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to let signal remain of type c_int, the type for signals. In order to address your (valid) concern, we could add a check into our wrapper, that signals an appropriate error.

@posborne
Copy link
Member

posborne commented Apr 8, 2016

I don't have a strong opinion on this one. Allowing for the signal to be included does seem important and I don't see any obvious problems with this approach. @kamalmarhubi

@kamalmarhubi
Copy link
Member

Interesting. I'd have to read the entire man page to figure out if there are any other similar issues so we don't end up changing the signature repeatedly.

@kamalmarhubi
Copy link
Member

I just opened a PR with an alternative approach at #348.

@homu
Copy link
Contributor

homu commented Apr 18, 2016

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

@kamalmarhubi
Copy link
Member

Given discussion on #348 and #343, I think this is the most acceptable approach. If / when we add an enum for signals, we can adjust the type of the argument.

@homu r+ b06432b

@homu
Copy link
Contributor

homu commented Apr 21, 2016

⚡ Test exempted - status

@homu homu merged commit b06432b into nix-rust:master Apr 21, 2016
homu added a commit that referenced this pull request Apr 21, 2016
Allow to specify signal when calling clone.

This is my suggestion on how to fix #343.
@fiveop fiveop deleted the clone_flags branch May 1, 2016 14:02
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.

Expose termination signal option in clone
4 participants