-
Notifications
You must be signed in to change notification settings - Fork 165
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
Rename and overhall DNS name types #125
base: main
Are you sure you want to change the base?
Conversation
Is any help needed with this? |
Allow `DnsName` to store an owned value instead of just a referenced value.
9f4ecfc
to
34b1032
Compare
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 73.39% 75.17% +1.78%
==========================================
Files 12 12
Lines 1368 1434 +66
==========================================
+ Hits 1004 1078 +74
+ Misses 364 356 -8
Continue to review full report at Codecov.
|
#[cfg(feature = "std")] | ||
#[derive(Clone, Debug, Eq, PartialEq, Hash)] | ||
pub struct DnsName(String); | ||
pub type DnsNameBox = DnsName<Box<[u8]>>; |
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.
Coming back to this after a long time, I'm not sure why I thought it was important to make it especially convenient to support Box<[u8]>
. I think I'll just remove this.
impl From<DnsNameRef<'_>> for DnsName { | ||
fn from(dns_name: DnsNameRef) -> Self { | ||
dns_name.to_owned() | ||
impl DnsNameInput for String { |
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.
I think I will try to remove DnsNameInput
. It seems like a very heavyweight mechanism for accomplishing something simple.
@@ -167,7 +167,7 @@ impl<'a> EndEntityCert<'a> { | |||
} | |||
|
|||
/// Verifies that the certificate is valid for the given DNS host name. | |||
pub fn verify_is_valid_for_dns_name(&self, dns_name: DnsNameRef) -> Result<(), Error> { | |||
pub fn verify_is_valid_for_dns_name(&self, dns_name: DnsName<&[u8]>) -> Result<(), Error> { |
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.
We should make this generic over the storage type of dns_name
.
|
||
// XXX: We need more test cases. | ||
#[test] | ||
fn test_dns_name_eq_different_len() { |
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.
:sigh: Supporting Hash
and PartialEq
for DnsName
has high cost for the benefit. I'll probably spend more time writing these equality/hash tests than doing all the rest.
No description provided.