API Changes #439
Description
Splitting off from #433 to discuss any breaking API changes we might want to make to the crate. CC @briansmith @newpavlov @dhardy @vks Let me know what you think or if I missed anything.
My proposed new API for this crate would be to introduce a getrandom::Opts structure which would initially be empty, but could have fields added over time to support things like #365. This would allow us to introduce new functionality by just adding new fields to Opts . Specifically, the API would be:
#[non_exhaustive] // Possible now that MSRV >= 1.40
#[derive(Clone, Copy, Debug, Default)]
pub struct Opts {}
impl Opts {
pub const fn new() -> Self;
}
pub fn fill(buf: &mut [u8], opts: Opts) -> Result<(), Error>;
/// Do we even want this? See discussion below
pub fn fill_uninit(buf: &mut [MaybeUninit<u8>], opts: Opts) -> Result<&mut [u8], Error>;
/// Convenience wrapper for `fill(buf, Opts::new())`
/// I don't think we would want to deprecate this.
pub fn getrandom(buf: &mut [u8]) -> Result<(), Error>;Changes to the Error type
I think we should mostly leave the API as-is. I think it's perfectly fine to expose all error constants on all targets. Given that the error codes aren't exhaustive, exposing all the constants doesn't do any harm, and makes some handing code easier to write.
I do think that we should get rid of the From<NonZeroU32> impl, as we really only use that conversion internally. It also might be nice to add an Error::from_raw_os_error method to mirror raw_os_error(), but this isn't a breaking change.
Is it worth keeping the uninit methods?
Given that getrandom_uninit is rarely used, I think it's fine to just replace it with fill_uninit. But this brings up a more important question, does it even make sense to keep the uninit methods at all? They add complexity and unsafe code, but it's unclear if they are worth it. We have benchmarks which compare calling getrandom_uninit vs zeroing a buffer and calling getrandom. Even with very fast (> 100 MB/sec) rng sources, the uninit methods aren't consistently faster (and any difference is within the margin of error).
On Linux (cargo bench --jobs=1):
test aes128::bench_getrandom ... bench: 409 ns/iter (+/- 20) = 39 MB/s
test aes128::bench_getrandom_uninit ... bench: 413 ns/iter (+/- 37) = 38 MB/s
test p256::bench_getrandom ... bench: 421 ns/iter (+/- 19) = 76 MB/s
test p256::bench_getrandom_uninit ... bench: 416 ns/iter (+/- 23) = 76 MB/s
test p384::bench_getrandom ... bench: 527 ns/iter (+/- 126) = 91 MB/s
test p384::bench_getrandom_uninit ... bench: 543 ns/iter (+/- 169) = 88 MB/s
test page::bench_getrandom ... bench: 8,676 ns/iter (+/- 219) = 472 MB/s
test page::bench_getrandom_uninit ... bench: 8,591 ns/iter (+/- 551) = 476 MB/s
I would be fine removing the uninit methods entirely, as it would simplify our implementation in multiple places.
Handling register_custom_getrandom!()
I would propose having the register_custom_getrandom! support both functions with the type of fill and those with the type of fill_uninit (if we keep the uninit methods). We can use traits/generics to have the macro work with either function type. Similar to the current implementation, the macro would generate a #[no_mangle] extern "Rust" function, but it would have a different signature:
#[no_mangle]
unsafe fn __getrandom_fill(dest: *mut u8, len: usize, opts: *const Opts) -> u32;I would also propose having the macro also create an unsafe fn __getrandom_custom(dest: *mut u8, len: usize) function, allowing the custom implementation registered with the next version of getrandom to still work with getrandom 0.2 without folks having to do tricks like those suggested in #433 (comment)