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.

Sign up for GitHub

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

Jump to bottom

process: add get/set resuid #40581

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

process: add get/set resuid #40581

wants to merge 5 commits into from

Conversation

WenheLI
Copy link

@WenheLI WenheLI commented Oct 24, 2021

This PR aims to support #40556

Test, lint, and build have been added and passed locally.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 24, 2021
@WenheLI WenheLI force-pushed the impl/serresuid branch 2 times, most recently from b3edf82 to 3690aaa Compare October 24, 2021 01:06
@WenheLI
process: add get/set resuid
87270fd 
This PR adds support for getresuid and setresuid for js side
@WenheLI
Copy link
Author

WenheLI commented Oct 24, 2021

It seems like Apple OSX only has different system API for setresuid
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/setreuid.2.html
We might need to figure how to leverage the gap.

@targos
Copy link
Member

targos commented Oct 25, 2021

@nodejs/process @nodejs/libuv Should this be in libuv?

@targos targos added the process Issues and PRs related to the process subsystem. label Oct 25, 2021
@cjihrig
Copy link
Contributor

cjihrig commented Oct 26, 2021

Should this be in libuv?

I don't think so based on similar functionality in src/node_credentials.cc.

addaleax
addaleax reviewed Oct 28, 2021
View reviewed changes
WenheLI and others added 2 commits November 2, 2021 15:38
@WenheLI
Copy link
Author

WenheLI commented Nov 4, 2021

Hi, I think this PR is read to review.
Any advices?

jasnell
jasnell requested changes Nov 13, 2021
View reviewed changes
lib/internal/bootstrap/node.js
@@ -175,6 +175,9 @@ if (credentials.implementsPosixCredentials) {
process.getgid = credentials.getgid;
process.getegid = credentials.getegid;
process.getgroups = credentials.getgroups;
if (!credentials.isApple) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of apis that may or may not exist depending on what platform you're on. My preference would be for the method to always be defined but throw a reasonable exception on unsupported platforms.

Copy link

@ptrxyz ptrxyz Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can't think of a similar case out of my head, but probably there are some cases like this when it comes to compatibility with Windows? Maybe this could be handled similarily then?
In general, I also like the idea of a consistent API with reasonable exceptions. 👍

WenheLI and others added 2 commits November 17, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptrxyz ptrxyz ptrxyz left review comments

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@WenheLI @targos @cjihrig @ptrxyz @jasnell @addaleax @nodejs-github-bot