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.

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

Freebsd: Try getrandom() first #57

Merged
merged 4 commits into from
Jul 9, 2019
Merged

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jul 7, 2019

Depends on #54 fixes #35

Implementation is quite similar to the Solaris/Illumos implementation. @myfreeweb PTAL

Edit: Also adds a fill_exact helper method for making repeated OS calls to fill a buffer. This let's us simplify the Linux/Solaris/FreeBSD code, as well as adding some changes useful for #54 and #58

@valpackett
Copy link

Looks good, works fine.

@newpavlov
Copy link
Member

Maybe it's worth to make this PR independent from #54? I think it will be some time before we will converge our opinions on that one, while changes in this PR look quite straightforward.

@josephlr
Copy link
Member Author

josephlr commented Jul 8, 2019

Maybe it's worth to make this PR independent from #54? I think it will be some time before we will converge our opinions on that one, while changes in this PR look quite straightforward.

The main reason for the change is that this implementation uses the fill_exact helper method introduced in #54. I can put those changes in this PR if you want.

@newpavlov
Copy link
Member

newpavlov commented Jul 9, 2019

I do not insist on it, you can leave it as-is. I was simply thinking it may be easier for you, since no syncing with #54 would be needed.

@josephlr
Copy link
Member Author

josephlr commented Jul 9, 2019

I was simply thinking it may be easier for you, since no syncing with #54 would be needed.

Good point, I added fff1bde to this PR that adds in the helper function changes.

@josephlr josephlr mentioned this pull request Jul 9, 2019
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks good, other than a comment as mentioned.

while !buf.is_empty() {
let res = f(buf);
if res < 0 {
let err = io::Error::last_os_error();
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to me to assume that calling last_os_error is appropriate here given the function name. At least this should be mentioned by function documentation.

Copy link
Member

Choose a reason for hiding this comment

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

During removal of std dependency it will have to be replaced with an explicit errno, so I think it's fine to keep it as-is for the time being.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I changed the function to be named sys_fill_exact and added a comment to explain the expected semantics of the sys_fill function. @newpavlov is right that this will be replaced with an explicit call to an errno function in #54

src/lib.rs Outdated Show resolved Hide resolved
src/solaris_illumos.rs Show resolved Hide resolved
src/util_libc.rs Outdated Show resolved Hide resolved
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.

Use getrandom on FreeBSD
4 participants