-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 setgroups to std::os::unix::process::CommandExt #72160
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
As this appears to add public API (perhaps insta-stable), I'm gonna tag in... |
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.
Needs some changes; with those made, I'd be happy to re-review.
On May 23, 2020 3:31:23 AM PDT, Jake Goulding ***@***.***> wrote:
@shepmaster commented on this pull request.>
>
>
>
> @@ -81,6 +81,7 @@ pub struct Command {>
gid: Option<gid_t>,>
saw_nul: bool,>
closures: Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>>,>
+ groups: Option<Vec<gid_t>>,>
>
So there’s a difference between calling `.groups(vec![])` and never
calling it to start with?>
Yes. setgroups with null or an empty list clears all supplementary gids. Not calling setgroups at all leaves the current set of supplementary gids.
|
0c426d8
to
9dd239b
Compare
Sorry for the delay. I made the requested changes |
ping from triage: |
@JohnCSimon How do I do that? I think I already marked all the conversations as resolved. |
@joshtriplett Could you take a look at my changes? Maybe it would have been better to put another commit on top of my previous one, instead of squashing them together. |
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
d4c1d74
to
de36aa0
Compare
@bors r+ |
📌 Commit de36aa0ed350f7ff6c1df156dd8023cd7f026e7c has been approved by |
⌛ Testing commit de36aa0ed350f7ff6c1df156dd8023cd7f026e7c with merge 31502a228a70000f735f1cfd9a517037686deba5... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Co-authored-by: Ashley Mannix <kodraus@hey.com>
de36aa0
to
a4db851
Compare
@bors r+ |
📌 Commit a4db851 has been approved by |
☀️ Test successful - checks-actions |
Should fix #38527. I'm not sure groups is the greatest name though.