-
Notifications
You must be signed in to change notification settings - Fork 50
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
IP address support: Address code review comments #21
Conversation
Codecov Report
@@ Coverage Diff @@
## feat-ip-address #21 +/- ##
===================================================
- Coverage 86.40% 86.07% -0.33%
===================================================
Files 15 15
Lines 2375 2349 -26
===================================================
- Hits 2052 2022 -30
- Misses 323 327 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
76b5a77
to
368294b
Compare
src/name/ip_address.rs
Outdated
.map(|octet| format!("{:02x?}{:02x?}", octet[0], octet[1])) | ||
.collect::<Vec<String>>() | ||
.join(":") | ||
format!( |
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 suppose this is a fine solution. I might have gone with pre-allocating a String::with_capacity()
and then using .write_fmt()
to write into it in a loop.
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 have no strong opinion on this one.
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.
What would you do with the Result
when using write_fmt
? ipv6_to_uncompressed_string
is meant to be used on an impl From
, so it cannot fail.
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 unwrap()
it -- after all, format!()
is actually doing the same thing and also doing something like unwrapping.
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.
Addressed in 81837a8bf2862df71a8e35f6f8d91dc4e2f5110f
src/name/verify.rs
Outdated
cert.inner().subject_alt_name, | ||
Err(Error::CertNotValidForName), | ||
&|name| { | ||
#[allow(clippy::single_match)] |
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.
Why not do if let GeneralName::IpAddress(presented_id) = name
instead of the match
with #[allow(clippy::single_match)]
?
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.
Updated.
- Mimic std names `IpAddr::V4`, `IpAddr::V6` for owned addresses, as well as `IpAddrRef::V4`, `IpAddrRef::V6` for referenced ones. Also mimic the exception name, now named `AddrParseError` instead of `InvalidIpAddressError`. - Update copyright header on `name.rs`. - Reduce the number of allocations on `ipv6_to_uncompressed_string` used to print IPv6 addresses in their uncompressed form. Other minor changes.
368294b
to
1ed3634
Compare
- Rename `DnsNameOrIpRef` to `SubjectNameRef` - Rename `InvalidDnsNameOrIpError` to `InvalidSubjectNameError`
f1b6d66
to
fba17e2
Compare
src/name/ip_address.rs
Outdated
if dot_count != 3 { | ||
return Err(AddrParseError); | ||
} | ||
Ok(IpAddrRef::V4(ip_address_, ipv4_octets(ip_address_)?)) |
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.
Sorry, no, I don't think this quite makes sense. Now we still have a routine that's parsing the data separately from the validation. The idea is that we accumulate the output as we validate it, so that the output accumulates from the validated data itself. So for example, in the same place where you currently move to the next octet, write into the output array?
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.
Addressed in 541bc2b5826354db63292fff15bf7bb3b5a2827a.
src/name/name.rs
Outdated
subject_name, | ||
ip_address::ipv6_octets(subject_name).map_err(|_| InvalidSubjectNameError)?, | ||
))); | ||
if let Ok(ip_address) = ip_address::parse_ipv6_address(subject_name) { |
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.
Can do else if
here.
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.
It's a return-on-first-match situation, no need for it, but I will add it nevertheless.
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.
Addressed in 416976f
1b24841
to
81837a8
Compare
Parse, don't validate
Use `write_fmt` in order to reduce repetition of arguments when formatting an IPv6 address.
81837a8
to
40b84b6
Compare
src/name/ip_address.rs
Outdated
octets[octet] = | ||
TryInto::<u8>::try_into(radix10_to_octet(¤t_octet[..current_size])) | ||
.expect("invalid character"); |
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'm adding these TryInto
due to clippy being executed with -D clippy::as-conversions
. If there is a better/more idiomatic way to shape this code I'm happy adapting.
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.
Same comment applies to the rest of TryInto
conversions in this commit.
4f9d583
to
1d30656
Compare
1d30656
to
327e47f
Compare
Mimic std names
IpAddr::V4
,IpAddr::V6
for owned addresses, as well asIpAddrRef::V4
,IpAddrRef::V6
for referenced ones. Also mimic the exception name, now namedAddrParseError
instead ofInvalidIpAddressError
.Update copyright header on
name.rs
.Reduce the number of allocations on
ipv6_to_uncompressed_string
used to print IPv6 addresses in their uncompressed form.Other minor changes.
This addresses most of the feedback provided in #5 (review).