rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
264 stars 166 forks source link

Rename `Weak` to `LazyPtr` and move it to `lazy.rs` #427

Closed newpavlov closed 1 month ago

newpavlov commented 1 month ago

Currently the Weak type is used only by the NetBSD backend, but in future it may be used by other backends. To simplify code a bit and prepare for potential use on Windows, Weak now accepts a "pointer initialization function" instead of a function name, i.e. it now works similarly to LazyUsize and LazyBool.

briansmith commented 1 month ago

I think we should have a plan for #285 first. It may be that we start using Weak on Linux too?

Also, I think we should sort out what we're doing for Windows, as we're basically implementing Weak using the Windows APIs in #415. Although PR #415 isn't written as an implementation of Weak for Windows, I think we should do that, if we're going with the LoadLibrary(Ex)-based approach, because the sandboxing concerns (at least) are analogous across platforms.

josephlr commented 1 month ago

Agreed with @briansmith here, lets figure out if we need Weak on other targets first.

newpavlov commented 1 month ago

I have changed this PR and now Weak resides in a separate module. The new Weak now uses linking function pointer instead of linked function name. It should address @briansmith's concerns which he tried to resolve in #426 and make its code a bit more future-proof.

newpavlov commented 1 month ago

@josephlr I've tweaked the code a bit and moved linking function to the get method (previously ptr). I think it's a slightly better approach since we do not need redundant store of the function pointer. Theoretically this API could be misused by calling get with different functions, but since it's a private API, it should not cause any issues.

briansmith commented 1 month ago

It would be good for somebody to actually test this and any future refactorings of Weak on a NetBSD machine.

newpavlov commented 1 month ago

One final question, should this now live in lazy.rs as its now quite similar to LazyUsize. If we do decide to move it, we should probably rename to LazyPtr.

Good observation! Although there is a certain difference between LazyUsize and LazyPtr in the orderings used under the hood.

briansmith commented 1 month ago

I do think having the test run on --linux-gnu (where we know dlsym makes sense) is a good idea.

let fptr = GETRANDOM.get(dlsym_getrandom).unwrap();
...

Note that we should actually call the function through fptr. We should have LazyPtr::get() do the pointer cast transmute at the end so that the test doesn't need to repeat it.

briansmith commented 1 month ago

I think we should have a plan for https://github.com/rust-random/getrandom/issues/285 first. It may be that we start using Weak on Linux too?

Also, I think we should sort out what we're doing for Windows,

It seems like for both of those, we're close to deciding now to use LazyPtr. If so, maybe we don't need to make this change, unless we want to make a change to improve the testing.

newpavlov commented 1 month ago

maybe we don't need to make this change, unless we want to make a change to improve the testing.

The result is cleaner and this approach does not need the CStr ersatz implemented in your other PR.

I do think having the test run on --linux-gnu (where we know dlsym makes sense) is a good idea.

I would prefer to have a proper VM-based testing implemented in a separate PR.