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

Log feature: OsRng improvements #250

Merged
merged 4 commits into from
Feb 5, 2018
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Feb 1, 2018

Mostly @pitdicker's code with a few tweaks by myself

I didn't realize using strerror was valid with CloudABI but apparently it is.

@pitdicker
Copy link
Contributor

Looks good to me!

src/os.rs Outdated
if err_count >= log_err {
log_err += TRANSIENT_STEP;
warn!("OsRng: retrying ...");
err_count += RETRY_LIMIT / TRANSIENT_RETRIES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is not a perfect division, it will retry 9 times instead of 8. Do we care about that? To round correctly it would have to be err_count += (RETRY_LIMIT + TRANSIENT_RETRIES - 1) / TRANSIENT_RETRIES;

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It doesn't matter a lot, but maybe we should tweak the constants so that RETRY_LIMIT is a multiple of TRANSIENT_RETRIES.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like division with rounding up (which my line in the comment above does) a little more than being careful with the constant to pick. But as we picked it basically arbitrary, the only somewhat important thing is that it runs the same number of times as we claim in the constant and logging.

@dhardy dhardy merged commit 0994d09 into rust-random:master Feb 5, 2018
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
Log feature: OsRng improvements
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