-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(node/os): implement getPriority, setPriority & userInfo #19370
Conversation
maybe |
gid = -1; | ||
} | ||
|
||
let _homedir = homedir(); |
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 is incorrect, as per node docs:
The value of homedir returned by os.userInfo() is provided by the operating system. This differs from the result of os.homedir(), which queries environment variables for the home directory before falling back to the operating system response.
I am unsure however how to do this, and think overall this should be good enough
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.
LGTM, but I'd like a second pair of eyes on the Windows bits. Nice work @crowlKats
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.
Looks good to me, but left some nitpicks.
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.
dsherret's comments should be addressed.
name: "APIs not yet implemented", | ||
name: "os.setPriority() & os.getPriority()", | ||
// disabled because os.getPriority() doesn't work without sudo | ||
ignore: true, |
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.
suggestion: Just to be extra cool maybe only disable this if user ID is non-zero? ie. When not being run as sudo.
ext/node/ops/os.rs
Outdated
pub fn get_priority(pid: u32) -> Result<i32, AnyError> { | ||
set_errno(Errno(0)); | ||
match ( | ||
// SAFETY: libc::getpriority is unsafe |
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.
nitpick: Not much of a safety comment :)
@crowlKats can we aim to land this PR for v1.36? |
@bartlomieju yes |
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.
LGTM, nice work Leo
Takes #4202 over
Closes #17850
Co-authored-by: ecyrbe ecyrbe@gmail.com