Closed josephlr closed 1 month ago
I think we should close this issue in favor of the existing issues for each of these topics.
I echo @briansmith's point: this is a number of unrelated issues.
get rid of the
From<NonZeroU32>
impl
The only use-case I can think of is if error codes need to be pushed through some other API (FFI) then converted back into an Error
. Possibly it should be kept simply as the inverse of fn Error::code
, though I also don't know how useful that is.
uninit
I've had people ask for this functionality several times in rand
, yet I agree with your sentiment that it's essentially a useless optimisation.
I filed #455 about deprecating/removing The From<NonZeroU32>
implementation and #454 about deprecating/removing getrandom_uninit
.
Remaining items in this issue are:
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 https://github.com/rust-random/getrandom/issues/365. This would allow us to introduce new functionality by just adding new fields toOpts
.
The only motivation given is for a non-blocking API, which as noted in #365 would ideally want a different function signature, hence it makes more sense to use a separate function.
Is there any other motivation? I do not think we should add Opts
"just in case".
It would be better to just discuss non-blocking getrandom in #365 and/or if we need a discussion regarding how to add options to the API, we should do that in its own issue. There are non-obvious considerations in the design of the options API.
Good point @briansmith I've copied relevant sections from this issue into #454 #365 #455
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 toOpts
. Specifically, the API would be:Changes to the
Error
typeI 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 anError::from_raw_os_error
method to mirrorraw_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 withfill_uninit
. But this brings up a more important question, does it even make sense to keep theuninit
methods at all? They add complexity andunsafe
code, but it's unclear if they are worth it. We have benchmarks which compare callinggetrandom_uninit
vs zeroing a buffer and callinggetrandom
. Even with very fast (> 100 MB/sec) rng sources, theuninit
methods aren't consistently faster (and any difference is within the margin of error).On Linux (
cargo bench --jobs=1
):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 offill
and those with the type offill_uninit
(if we keep theuninit
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: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 ofgetrandom
to still work withgetrandom 0.2
without folks having to do tricks like those suggested in https://github.com/rust-random/getrandom/issues/433#issuecomment-2133355788