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
Enable more digest-related functionality for *ring*. #194
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good so far. Let me know when it's ready for merge.
client/src/rr/dnssec/digest_type.rs
Outdated
/// This will always error, enable openssl feature at compile time | ||
#[cfg(not(feature = "openssl"))] | ||
#[cfg(not(any(feature = "openssl", feature = "ring")))] | ||
pub fn hash(&self, _: &[u8]) -> DnsSecResult<Vec<u8>> { | ||
Err(DnsSecErrorKind::Message("openssl feature not enabled").into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change this error to say "openssl or ring"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
client/src/rr/dnssec/digest_type.rs
Outdated
pub fn digest_all(&self, data: &[&[u8]]) -> DnsSecResult<Digest> { | ||
use std::io::Write; | ||
|
||
let digest_type = try!(self.to_openssl_digest()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just an FYI, I'm slowly upgrading the code to use ?
instead of try!
. This doesn't need to be changed, but if you want to use '?' feel free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it using try!
for this set of patches, but I'll remember this in the future and switch to ?
.
} else { | ||
return false; | ||
}) | ||
.map(|hash| hash.as_ref() == self.digest()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, I love it when I forget to just return the result of ==
. This is definitely cleaner.
This looks ready to go, as it passed CI and I addressed your review comments. |
Just needed to update the branch. Assuming tests pass, I'll merge in. |
No description provided.