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

Replace ReadRng with read_exact in OsRng` #48

Closed
wants to merge 1 commit into from

Conversation

pitdicker
Copy link

I feel the abstraction was not worth it.

@dhardy
Copy link
Owner

dhardy commented Nov 13, 2017

Pertinent question: do we want to share file-handles instead of re-opening?

@nixpulvis
Copy link

My gut reaction is that sharing file descriptors is only a win, since it'll take some load off the OS. I know there are some considerations around having a bunch of descriptors allocated at once, but I'm not an expert here.

@dhardy
Copy link
Owner

dhardy commented Nov 17, 2017

@nixpulvis that was a rhetorical question (we already discussed to some extent). PRs welcome BTW.

@pitdicker
Copy link
Author

I see some comments that file descriptors to the same file get reference counted on the kernel side across multiple threads. Then it may not make any difference if we start to reference count them on our side instead. But this would need some testing to be sure.

Once I fix the merge conflict, is this ok to be merged?

@dhardy
Copy link
Owner

dhardy commented Nov 21, 2017

I don't see any advantage to removing ReadRng (a very slight reduction in complexity maybe, but it's marginal). OTOH if further changes get made to this code in the future the extra layer of abstraction may be useful. So it seems like a step backwards removing it, even though it's not necessary.

@pitdicker pitdicker closed this Dec 7, 2017
@pitdicker pitdicker deleted the no_readrng branch February 2, 2018 20:33
@pitdicker pitdicker restored the no_readrng branch March 26, 2018 10:01
@pitdicker pitdicker deleted the no_readrng branch April 4, 2018 17:50
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.

3 participants