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

crypto-common: add RngError type #1382

Closed
wants to merge 2 commits into from
Closed

Conversation

tarcieri
Copy link
Member

Adds a common error type which can be used with both getrandom and rand_core.

This eliminates the need to have getrandom as part of the public API.

Adds a common error type which can be used with both `getrandom` and
`rand_core`.

This eliminates the need to have `getrandom` as part of the public API.
/// The error type returned when a random number generator fails.
#[cfg(any(feature = "getrandom", feature = "rand_core"))]
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub struct RngError;
Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially it could carry along an inner code e.g. the raw_os_error as reported by either getrandom::Error or rand_core::Error, although I don't think it's bad for it to be completely opaque

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

I don't think we should replace the errors in such way. They may carry crucial information for finding why an error has occurred. If you don't want to expose getrandom in public API, you could convert getrandom::Error into rand_core::Error using From/Into, but it would mean that you have to pull rand_core when only the getrandom feature is enabled.

Personally, I think the current code is fine.

@newpavlov
Copy link
Member

BTW it may be worth to add std = ["rand_core?/std", "getrandom?/std"] to crypto-common's Cargo.toml, so the errors from those crates would have the Error trait implemented when std is enabled.

@tarcieri
Copy link
Member Author

tarcieri commented Nov 12, 2023

They may carry crucial information for finding why an error has occurred.

Do you want anything more than the code? That's easy enough to propagate. rand_core::Error is just a thin wrapper for getrandom::Error in that regard: https://github.com/rust-random/rand/blob/ef89cbefaf484270dc3936d5d32fc2a73314173c/rand_core/src/error.rs#L178

The downside of the current approach is there are two different error types to deal with depending on which functions you're calling.

@newpavlov
Copy link
Member

newpavlov commented Nov 12, 2023

That's easy enough to propagate. rand_core::Error is just a thin wrapper for getrandom::Error in that regard

Note the #[cfg(feature = "std")] part. User-defined RNGs (e.g. wrappers around an external HW RNG) may define their own errors, without associated error code.

The downside of the current approach is there are two different error types to deal with depending on which functions you're calling.

I don't think generate_* and generate_*_with_rng will be mixed much in practice. And even if they are, you can easily convert Result<T, getrandom::Error> into Result<T, rand_core::Error> by using .map_err(Into::into). In some cases it even can be done as part of error bubbling by ?.

@tarcieri
Copy link
Member Author

Alright, I guess I'll close this

@tarcieri tarcieri closed this Nov 12, 2023
@tarcieri tarcieri deleted the crypto-common/rng-error-type branch November 12, 2023 03:09
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