Make signal argument to kill optional #445
Conversation
|
I feel like a better approach might be to add a new enumerated value rather than making the signal an option. A signal of 0 is still a signal but one that is just handled by the kernel without actually making it to the process. I'm not sure this signal has an established name (it doesn't appear to in Linux: http://lxr.free-electrons.com/source/arch/arm/include/asm/signal.h?v=2.6.32#L31). I suggest that we just call it As an added benefit, this won't break things for users using the existing API. |
|
I disagree. 0 is no signal, it represents something different. Therefore, it does not belong in the enumeration. I would not have a problem with giving it it's own name, if I not already objected to it belonging to the operation. Since we break the API left and right at the moment, in order to get things 'nice', I do not have a problem with breaking this. It is easy to adapt code that is affected by the change. |
|
Although 0 is not implemented as a signal to processes in most operating systems, I am still failing to see a strong reason to separate it. Strace, for instance, goes so far as to name the |
|
Thinking about this more, I can be OK with the change (better than not having support for this useful operation). Moving forward, we should probably have a push for doing more documentation on our functions, particularly when there are cases like this where things might not be immediately clear from the signature. At present, I would need to read the source (and probably go deep on the man pages) in order to figure out what passing With some docs added, r=me |
|
I'll add a documentation comment. |
| @@ -388,8 +388,12 @@ pub fn pthread_sigmask(how: SigFlags, | |||
| Errno::result(res).map(drop) | |||
| } | |||
|
|
|||
| pub fn kill(pid: libc::pid_t, signal: Signal) -> Result<()> { | |||
| let res = unsafe { libc::kill(pid, signal as libc::c_int) }; | |||
| pub fn kill(pid: libc::pid_t, signal: Option<Signal>) -> Result<()> { | |||
There was a problem hiding this comment.
maybe we can take advantage of the recently added impl<T> From<T> for Option<T>. On recent enough Rust (1.12+), kill(pid, SIGBLAH) will still work, so it's a bit more ergonomic. On earlier Rust, it is equivalent to this change thanks to impl<T> From<T> for T.
There was a problem hiding this comment.
Interesting, I wasn't aware of that change. Nice!
There was a problem hiding this comment.
What about us supporting Rust (1.7+), though? If someones crate wants to continue to support 1.7+, it has to change its source (which I don't have a problem with in the first place).
So asked differently: Are you saying, that we will passively take advantage of the new impl on new enough Rust versions anyway?
There was a problem hiding this comment.
Yeah, that was my thought. Though perhaps we should wait until we bump minimum version to 1.12, probably in 3-6 months? Not sure how to think about it.
There was a problem hiding this comment.
Trying to think about it differently: the change from Signal to Option<Signal> is breaking. The change from Option<Signal> to T: Into<Option<Signal>> is (I think?) not breaking thanks to impl<T> From<T> for T. (This depends on how you read "breaking" in signatures.) Our use of the Into<Option<T>> impl would be more akin to "progressive enhancement". A downstream crate requiring its use would have a 1.12+ minimum version, but we would not.
There was a problem hiding this comment.
In any case, if making use of the new impl, we should include tests that demonstrate it works in both cases.
There was a problem hiding this comment.
Sounds reasonable. And if I understand you and the matter correctly, then we could already use T: Into<Option<Signal>> right now.
|
@kamalmarhubi better? |
|
Could you add a test to verify that calling it with With that, r=me. |
|
Also I checked and under 1.12 it allows |
|
📌 Commit 45d8bee has been approved by |
Make signal argument to kill optional Fixes #441
|
☀️ Test successful - status |
Fixes #441