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

log: Remove optional log dependancy #131

Merged
merged 3 commits into from
Jan 9, 2020
Merged

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jan 9, 2020

This feature isn't enabled by rand/rand_core and provides very little
error information that isn't already conveyed through our Error values.

This also simplifies the supported configuration space for getrandom.

See this comment by @newpavlov: #98 (comment)

@josephlr josephlr added this to the 0.2 milestone Jan 9, 2020
This feature isn't enabled by rand/rand_core and provides very little
error information that isn't already conveyed through our Error values.

This also simplifies the supported configuration space for getrandom.

Signed-off-by: Joe Richey <joerichey@google.com>
@newpavlov newpavlov requested a review from dhardy January 9, 2020 14:28
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concern: the Windows case now never reports the raw NTSTATUS code. However, relying on the logging system for this was never an ideal solution anyway, so it's not sufficient reason to avoid this PR.

Ultimately I'm undecided whether the small utility of logging support is worth the small amount of extra code, but admit it has very low utility. I will let you guys decide.

@josephlr
Copy link
Member Author

josephlr commented Jan 9, 2020

Concern: the Windows case now never reports the raw NTSTATUS code. However, relying on the logging system for this was never an ideal solution anyway, so it's not sufficient reason to avoid this PR.

I think this is the only significant case where we actually took advantage of log's full functionality, but I don't think this Windows method will actually return a warning, so we probably weren't getting much out of it regardless.

EDIT: It looks like BCryptGenRandom, BCryptOpenAlgorithmProvider, and all the other BCrypt methods only mention STATUS_SUCCESS or NTSTATUS values in the error range. It seems unlikely then that our info/warn logging would ever really do anything.

Long-term, we could work on getting the correct description for our Error type, but that can be done later.

@josephlr josephlr merged commit d6b75d1 into rust-random:0.2 Jan 9, 2020
@josephlr josephlr deleted the log branch January 9, 2020 21:27
@dhardy
Copy link
Member

dhardy commented Jan 10, 2020

Correct, it seems that MS do not document what the error codes are. OTTH this guy does and it means people can't then search for the error correctly. But for the most part this probably isn't really an issue (RNGs don't usually fail, and if it does, then a Rust app is probably not going to be the only one to notice).

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.

None yet

3 participants