Skip to content

Simplify sudo check on unix #2183

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

Merged
merged 2 commits into from
Jan 5, 2020
Merged

Simplify sudo check on unix #2183

merged 2 commits into from
Jan 5, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jan 1, 2020

This PR uses

  • cfg(unix) attribute to limit sudo check on Unix platforms only.
  • Move unsafe code interact with FFI to separate function: home_dir_from_passwd
    • Use libc::sysconf to get default entry size.

@kinnison
Copy link
Contributor

kinnison commented Jan 2, 2020

There's a substantial set of changes in there for "Simplify" but I think I follow them. This kind of rework, particularly when there is unsafe code is much more easily reviewed as a series of commits for future reference.

Could you please detail what testing you've done to ensure this is right, considering that the test code deliberately skips it in almost every case?

Also, how does this interact with work done in #1936 and does it mean that #699 is obsolete?

@tesuji
Copy link
Contributor Author

tesuji commented Jan 2, 2020

Yeah, I'll try to improve that in the future.
About #699, I think e308c56 enabled it again.
This PR doesn't interact with #1936 at all. More info in the rewrote PR description.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Thank you for splitting things up and clarifying the relationships with those other items.

LGTM.

@kinnison kinnison merged commit e6dd3c8 into rust-lang:master Jan 5, 2020
@tesuji tesuji deleted the sudo-check branch January 5, 2020 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants