Skip to content

process: add get/set resuid#40581

Open
WenheLI wants to merge 5 commits intonodejs:mainfrom
WenheLI:impl/serresuid
Open

process: add get/set resuid#40581
WenheLI wants to merge 5 commits intonodejs:mainfrom
WenheLI:impl/serresuid

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
update osx
103d902
@WenheLI @addaleax
Update src/node_credentials.cc
c518ac2 
Co-authored-by: Anna Henningsen <github@addaleax.net>
@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
process.getgid = credentials.getgid;
process.getegid = credentials.getegid;
process.getgroups = credentials.getgroups;
if (!credentials.isApple) {
Copy link
Member

@jasnell jasnell Nov 13, 2021

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
@WenheLI @jasnell
Update src/node_credentials.cc
9ae4adb 
Co-authored-by: James M Snell <jasnell@gmail.com>
@WenheLI
add
adcefd7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell requested changes

+1 more reviewer

@ptrxyz ptrxyz ptrxyz left review comments

Reviewers whose approvals may not affect merge requirements

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