-
Notifications
You must be signed in to change notification settings - Fork 675
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
Added tcgetpgrp and tcsetpgrp #451
Conversation
Thank you! The implementation looks good to me. Can you do two minor things:
|
Sure. I'll get in that. |
☔ The latest upstream changes (presumably #449) made this pull request unmergeable. Please resolve the merge conflicts. |
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 seems fine to me. One small change to tcsetgrp
, and then @posborne's request for a changelog entry and docs. If the example docs are to much, just a short one-line would still be useful.
#[inline] | ||
pub fn tcsetpgrp(fd: c_int, pgrp: pid_t) -> Result<c_int> { | ||
let res = unsafe { libc::tcsetpgrp(fd, pgrp) }; | ||
Errno::result(res) |
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 should have .map(drop)
, as the return only distinguishes failures.
I've made the request changes. |
📌 Commit 013e7c8 has been approved by |
Added tcgetpgrp and tcsetpgrp I added the tcgetpgrp and tcsetpgrp functions. I'm not sure if I did everything right so please tell me if I need to change anything.
💥 Test timed out |
@homu retry |
Added tcgetpgrp and tcsetpgrp I added the tcgetpgrp and tcsetpgrp functions. I'm not sure if I did everything right so please tell me if I need to change anything.
☀️ Test successful - status |
I added the tcgetpgrp and tcsetpgrp functions. I'm not sure if I did everything right so please tell me if I need to change anything.