Crate implementation

[newpavlov]

Currently only WASM targets are not migrated. I've introduced some changes to the code, but tested only Linux and SGX, thus crate should be carefully reviewed and tested on all supported platforms before using it for rand_os and rand. Notable changes include:

• Thread local storage is used instead of global statics in several places. (in theory should be more efficient for long-lived threads, but uses one file descriptor per thread)
• Dependence on cloudabi and fuchsia-zircon is removed in favor of direct externs.
• Logging is removed. (can be added later)

A certain amount of work will be needed to translate errno values to Error variants, currently I simply return Error::Unknown. Ans we will need to create CI config as well.

[dhardy]

Thread local storage is used instead of global statics in several places. (in theory should be more efficient for long-lived threads, but uses one file descriptor per thread)

I think the global statics design was chosen for two reasons:

• we assume this API will not be used frequently, thus performance is less important than resource usage (though I don't know how true this is, or how significant the trade-offs are)
• it makes error-handling more predictable: one can fairly safely assume that all reads will succeed after the first successful read if re-using the same handle (I guess the main difference here is the vague possibility of running out of file handles)

Dependence on cloudabi and fuchsia-zircon is removed in favor of direct externs.

I'm not so sure this is a good idea? If the ABI changes, presumably it would be reflected in a new version of those API crates, and thus it'll be more obvious that something needs updating. Also, I suspect most real applications on those platforms would depend on those API libs for something or other anyway, so they'd still be in the build. @erickt @EdSchouten (FYI: this is a re-implementation of OsRng without dependence on Rand traits, with the aim to port OsRng to this in the future.)

BTW did you see this PR? rust-random/rand#709

[newpavlov]

we assume this API will not be used frequently, thus performance is less important than resource usage

I don't like the fact that with Mutex reading entropy blocks the source for all other threads. Imagine that due to a mistake one thread wants to fill several megabytes of data, while another thread wants just to seed ThreadRng. In other words TLS based implementation demonstrates a better "fairness", also it makes code a bit simpler.

I'm not so sure this is a good idea?

In the case of Cloud ABI I believe it's safe to rely on ABI stability. Plus pulling less crates will be a plus for potential future inclusion into std. For example for seeding hash tables Rust uses exactly the same code as in this PR and in the fuchsia-cprng crate.

[erickt]

Hello from fuchsia! Thanks for cc'ing me. The code for fuchsia is incorrect, we have since changed the syscall for our cprng Nowadays we recommend using fuchsia-cprng which we plan to keep up to date while we make some bigger changes to the fuchsia-zircon crate. Or you can just copy what we do in fuchsia-cprng, the code is pretty tiny.

[newpavlov]

The code for fuchsia is incorrect, we have since changed the syscall for our cprng

Can you elaborate? (UPD: did you mean &mut self which I've missed?) This PR uses absolutely the same syscall as fuchsia-cprng.

Also I wonder what happens with our backward compatibility promise in presence of changing syscalls. For example if in future you will rename syscall and will publish fuchsia-cprng v0.2, should getrandom get a minor version update or not?

@dhardy
If you believe it will be better to use fuchsia-cprng and cloudabi crates I will bring then back.

[dhardy]

Probably in this case we should just copy the latest OsRng code. The API crate is tiny.

[dhardy]

Okay, there's a lot here and most of it looks good!

The error type still needs some work. Something minimal like struct Error(u32); with custom debug and std::error::Error implementations might be nice?

The WASM implementations can potentially wait until another PR; possibly also changes to the error type.

[erickt]

@newpavlov yeah, if we make a breaking syscall to zx_cprng_draw, we'll publish a 0.2, but we're not expecting to change that api. We're switching people off of fuchsia-zircon because we expect to make breaking changes to the rest of that library, so we made fuchsia-cprng to make sure projects like this aren't impacted by those changes.

[newpavlov]

Something minimal like struct Error(u32); with custom debug and std::error::Error implementations might be nice?

I am not sure... Storing raw error code will allow nice interoperability with io::Error via from_raw_os_error (we could even implement Into trait) and will be trivially extensible, but on the other hand it feels quite unidiomatic. But even if we'll go with this approach, I think it should be struct Error(NonZeroU32), so Result<(), Error> will use only 4 bytes.

Also there is a question of error code for Unavailable error. Should we define some magic error code which may in theory overlap with some OS error code?

[dhardy]

Looks pretty good, but there are a few reasons the code may have to change yet, so I don't know whether the abstraction is a good fit:

• as you mentioned, we can still avoid repeating some of the checks multiple times via use of AtomicBool — this only affects Linux/Android and NetBSD so might be easier to do outside the abstraction
• error messages need to be handled more carefully
• we currently have no non-blocking API — though maybe this doesn't matter since most users won't bother with that and those who are concerned can use a dedicated thread

[dhardy]

Okay, then I think we're good to merge this (assuming you're not planning on more tweaks)?

[newpavlov]

I think I will add AtomicBool and code-based error.

[newpavlov]

Note that because we use NonZeroU32 for Error MSRV is bumped to 1.28.

[dhardy]

Uh, I don't think there's any need to increment the copyright notice year — the only real purpose of this notice is to make it obvious that there is a copyright notice (I don't think they have any legal function).

[newpavlov]

Ah, yes, you are right.

the only real purpose of this notice is to make it obvious that there is a copyright notice

This is why I simply do not include per-file notices in my crates. :)

[newpavlov]

@dhardy
If you don't have any additional change requests I think this PR can be merged. Adding documentation and CI can be done in separate PRs.