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

NewSeeded: use JitterRng fallback #54

Merged
merged 5 commits into from
Nov 21, 2017
Merged

NewSeeded: use JitterRng fallback #54

merged 5 commits into from
Nov 21, 2017

Conversation

dhardy
Copy link
Owner

@dhardy dhardy commented Nov 17, 2017

I changed my mind about renaming as mentioned in #17: having "new" in the name is important to clarify that this provides a "new" function.

To prevent confusion, this completely prevents any user implementation of the NewSeeded trait, by making it only applicable to types implementing SeedFromRng anyway (from_rng may migrate to SeedableRng but that's a separate issue).

As @pitdicker put it elsewhere, this is supposed to "make it easy to do the right thing". If users or PRNG authors want a different seeding function, they can use a different trait/function (there's no reason another trait can't also provide a new function).

Since @nagisa @newpavlov @burdges commented on #17 I'll give them a chance to disagree here, but I don't find any of the arguments against this I've seen so far convincing.

(Note: the TODO comments are intended to stay in the source for now; #43 can implement one of them.)

Also restrict to prevent user implementation
Error::new_with_cause(
ErrorKind::Unavailable,
"seeding a new RNG failed: both OS and Jitter entropy sources failed",
e1)

Choose a reason for hiding this comment

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

Why does error handling seem to be to most difficult part again and again ;-)?

Good idea to report e1 from OsRng as the error. That seems to be the most important one.

But not reporting the error cause from JitterRng is not great...

One option is to implement a struct TwoCauses and implement a custom stdError::description and other machinery for that. Maybe overkill though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

If Error::cause returned an iterator instead of an Option it would be easier :(

A custom description implementation doesn't cover it very well either, because (a) it requires formatting two messages into one (long) string somehow and (b) if both causes have sub-causes there are even more problems.

Easier to log/print something directly.

@burdges
Copy link

burdges commented Nov 17, 2017

I think NewSeeded is a better name than AutoSeeded if the function will be called new. I'm not overly worried about the new name since you can always call this as rand::NewSeeded::new::<MyRng>() if you do not want to use NewSeeded;, due to it claiming the name new.

@dhardy dhardy merged commit 8b0d1ad into master Nov 21, 2017
@dhardy dhardy deleted the new_seeded branch December 19, 2017 17:12
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