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

Add killpg #1034

Merged
merged 1 commit into from Mar 16, 2019
Merged

Add killpg #1034

merged 1 commit into from Mar 16, 2019

Conversation

DanSnow
Copy link
Contributor

@DanSnow DanSnow commented Mar 4, 2019

It's seem that #644 is inactively about 1 year. But I really want this API that can land in nix.

Actually I not really understand how to check the availability of an API for other platform except Linux. But I saw that this API in libc is wrapping inside a #[cfg(unix)]. So I don't need to add any #[cfg] on this API, am I right?

Resolved #644

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.

This needs documentation and tests, too.

let res = unsafe { libc::killpg(pid.into(),
match signal.into() {
Some(s) => s as libc::c_int,
None => 0,
Copy link
Member

Choose a reason for hiding this comment

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

Why would you ever want to send 0? Is that even valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's valid. It just perform error checking and won't send any signal.

According to the POSIX spec of killpg:

If pgrp is greater than 1, killpg(pgrp, sig) shall be equivalent to kill(-pgrp, sig).

And in the POSIX spec of kill:

The signal to be sent is specified by sig and is either one from the list given in <signal.h> or 0. If sig is 0 (the null signal), error checking is performed but no signal is actually sent. The null signal can be used to check the validity of pid.

Also in the manpage of killpg:

On Linux, killpg() is implemented as a library function that makes the call kill(-pgrp, sig).

@DanSnow
Copy link
Contributor Author

DanSnow commented Mar 4, 2019

Thanks for reviewing. Actually I just copy the implement of nix::sys::signal::kill and it doesn't have any document either. But I think that I can also copy the test of kill.

@DanSnow
Copy link
Contributor Author

DanSnow commented Mar 5, 2019

@asomers I have added test & document. Can you review again please?

@@ -680,6 +680,8 @@ pub fn kill<T: Into<Option<Signal>>>(pid: ::unistd::Pid, signal: T) -> Result<()
Errno::result(res).map(drop)
}

/// Send a signal to a process group [(see
/// killpg(3))](http://pubs.opengroup.org/onlinepubs/9699919799/functions/killpg.html).
Copy link
Member

Choose a reason for hiding this comment

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

The docs should say what the arguments mean. It's not obvious why signal should be an Option. Also, I see from the man page that killpg does something special when pgrp is 0. Perhaps that should be an option too?

@@ -9,6 +9,11 @@ fn test_kill_none() {
kill(getpid(), None).expect("Should be able to send signal to myself.");
}

#[test]
fn test_killpg_none() {
killpg(getpgrp(), None).expect("Should be able to send signal to my process group.");
Copy link
Member

Choose a reason for hiding this comment

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

80 columns, please.

@DanSnow
Copy link
Contributor Author

DanSnow commented Mar 8, 2019

@asomers I have update the document. Can you check again?

/// Send a signal to a process group [(see
/// killpg(3))](http://pubs.opengroup.org/onlinepubs/9699919799/functions/killpg.html).
///
/// If `pgrp` less then or equal 1, the behavior is undefined.
Copy link
Member

Choose a reason for hiding this comment

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

In compiler-speak, "Undefined behavior" means something specific and very bad. It would be better to say "platform-specific".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asomers OK. I have updated it.

@asomers
Copy link
Member

asomers commented Mar 16, 2019

Ok, looks good. It just needs a squash now.

@DanSnow
Copy link
Contributor Author

DanSnow commented Mar 16, 2019

Thanks again. I just squashed all commits.

@asomers
Copy link
Member

asomers commented Mar 16, 2019

Ahh, it looks like the PR I merged yesterday now conflicts with your CHANGELOG entry. I'm afraid you'll have to rebase.

@DanSnow
Copy link
Contributor Author

DanSnow commented Mar 16, 2019

OK. I think I fixed it.

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.

bors r+

bors bot added a commit that referenced this pull request Mar 16, 2019
1034: Add killpg r=asomers a=DanSnow

It's seem that #644 is inactively about 1 year. But I really want this API that can land in nix.

Actually I not really understand how to check the availability of an API for other platform except Linux. But I saw that this API in `libc` is wrapping inside a `#[cfg(unix)]`. So I don't need to add any `#[cfg]` on this API, am I right?

Resolved #644 

Co-authored-by: DanSnow <dododavid006@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 16, 2019

Build succeeded

@bors bors bot merged commit 8d01884 into nix-rust:master Mar 16, 2019
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

2 participants