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

Draft: Options API #353

Closed
wants to merge 3 commits into from
Closed

Draft: Options API #353

wants to merge 3 commits into from

Conversation

josephlr
Copy link
Member

Looking through the wonderful review by @m-ou-se in m-ou-se#1, I realized that at some point we might need a separate API for generating hashmap seeds. This is because when generating a hashmap seed on Linux, we would not want to block waiting on the RNG to initialize.

However, if we decided to add a different source or configuration for the RNG one day, it wouldn't make sense to just have it be a free function in this crate. This is because with the addition of #291 (and potentially #293), we also use free functions to get the random bytes in different ways (fill a &mut [u8], fill a &mut [MaybeUninit<u8>], return an array).

This PR contains the proposed Options API:

Right now we only have a single option: Options::DEFAULT. However, in the future, we could add Options to:

  • use a insecure but non-blocking system source (for seeding hashmaps).
  • guarantee the function doesn't block (would return an error on most targets)
  • not use a fallback mechanism (@briansmith had something like this for ring at one point).
  • never use the custom RNG source
  • always prefere the custom RNG source if it exists

Not all of these are things we should necessarily do, but this gives
us the flexibility to add such options in the future.

Bikeshed: This enum could be called Options/Source/Config, it could also be an opaque struct at this point rather than an enum (as we only have one variant).

Signed-off-by: Joe Richey [email protected]

Fixes #348

we only need to read the contents of the repo to run our tests, no other
permissions are needed, as we currently do not publish via our CI jobs.

Signed-off-by: Joe Richey <[email protected]>
Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr mentioned this pull request Mar 25, 2023
Right now we only have a single option `Option::DEFAULT`.

However, in the future, we could add options to:
  - guarantee the function doesn't block
  - use a insecure but quicker system source (for seeding hashmaps).
  - not use a fallback mechanism
  - never use the custom RNG source
  - always prefere the custom RNG source if it exists

Not all of these are things we should _necessarily_ do, but this gives
us the flexibility to add such options in the future.

Signed-off-by: Joe Richey <[email protected]>
@newpavlov
Copy link
Member

I understand rationale behind exposing less secure, but non-blocking getrandom. It's reasonable to expect that both variants will be used in a same application. But I don't think it applies to the other options you listed. Those are better served by crate's configuration options or by replacing getrandom with something custom.

I suggest we focus on the HashMap use case. I don't quite like your Option approach. It feels backward. I think we should do one of the following:

  1. Introduce separate functions. Increasing number of public functions from 3 to 6 looks fine to me.
  2. Introduce flag-like argument for the functions. It will mirror how getrandom works on Linux with its GRND_RANDOM and GRND_NONBLOCK flags.

@newpavlov
Copy link
Member

Closing it in favor of #365.

@newpavlov newpavlov closed this Jun 6, 2023
@newpavlov newpavlov deleted the options_api branch June 6, 2023 15:16
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