std::net: Ipv4Addr and Ipv6Addr improvements #56050
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
little-dude
force-pushed
the
ip
branch
2 times, most recently
from
ac694a9
to
ea87fb7
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I have a few questions How can I get stacktraces with all the line numbers?I run: But the stacktrace is just: What's the fastest way to run the tests?It seems that many things get re-compiled with the above command, not just libstd. Someone on discord suggested I also tried How could the tests be refactored?The ipv6 properties test in particular look like this: check("fdff:ffff::", &[0xfd, 0xff, 0xff, 0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
false, false, true, false, false, false, false, false, None);I'm planning to add a few more properties so this is starting to get really hard to read and debug. I first started to write everything explicitely for each ip: let ip: Ipv6Addr = "ff01::".parse().unwrap();
assert!(!ip.is_unspecified());
assert!(!ip.is_loopback());
assert!(!ip.is_unique_local());
assert!(!ip.is_global());
assert!(!ip.is_unicast_link_local());
assert!(!ip.is_unicast_link_local_strict());
assert!(!ip.is_unicast_global());
assert!(!ip.is_documentation());
assert!(ip.is_multicast());
assert_eq!(ip.multicast_scope(), Some(InterfaceLocal))It's nice because when something fails, I directly know which line failed, but it's getting really long. Each new test case adds a dozen of lines. |
- quote the RFC - add a link to the RFC - fix markdown
RFC 4291 is a little unclear about what is a unicast link local address. According to section 2.4, the entire fe80::/10 range is reserved for these addresses, but section 2.5.3 defines a stricter format for such addresses. After a discussion[0] is has been decided to add a different method for each definition, so this commit: - renames is_unicast_link_local() into is_unicast_link_local_strict() - relaxed the check in is_unicast_link_local() [0]: rust-lang#27709 (comment)
As per @therealbstern's comment[0]: The implementation of Ipv4::is_global is not complete, according to the IANA IPv4 Special-Purpose Address Registry. - It compares the address to 0.0.0.0, but anything in 0.0.0.0/8 should not be considered global. - 0/8 is not global and is currently forbidden because some systems used to treat it as the local network. - The implementation of Ipv4::is_unspecified is correct. 0.0.0.0 is the unspecified address. - It does not examine 100.64.0.0/10, which is "Shared Address Space" and not global. - Ditto 192.0.0.0/24 (IETF Protocol Assignments), except for 192.0.0.9/32 and 192.0.0.10/32, which are carved out as globally reachable. - 198.18.0.0/15 is for "Benchmarking" and should not be globally reachable. - 240.0.0.0/4 is reserved and not currently reachable
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Building the doc or running the tests more than 30 minutes locally, it's really hard to test changes, even as simple as these. Am I doing something wrong? What's people workflow when they make changes to libstd? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Question: if these were changed to slice matches, would the compiler generate more optimal code? It seems like the complexity in these methods would probably generate much more code than one would want but I'm curious if that's actually the case. |
|
@clarcharr I have no idea. We don't have benchmarks for these but this is something I could add. To whoever will review this PR, I still have a few questions regarding the tests: how to run them quickly, and how to get full stacktraces. That would be really helpful to fix the existing tests and write new ones. |
I don't know what the correct
I don't know.
Maybe have multiple |
|
ping from triage @TimNN @little-dude what's the update on this? |
|
ping from triage @little-dude Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! |
Dylan-DPC-zz
added
S-inactive
28 tasks
13 tasks
bors
added a commit
that referenced
this pull request
Jun 1, 2019
std::net: Ipv4Addr and Ipv6Addr improvements Picking this up again from my previous PR: #56050 Related to: #27709 Fixes: #57558 - add `add Ipv4Addr::is_reserved()` - [X] implementation - [X] tests - add `Ipv6Addr::is_unicast_link_local_strict()` and update `Ipv6Addr::is_unicast_link_local()` documentation - [X] implementation - [X] test - add `Ipv4Addr::is_benchmarking()` - [X] implementation - [X] test - add `Ipv4Addr::is_ietf_protocol_assignment()` - [X] implementation - [X] test - add `Ipv4Addr::is_shared()` - [X] implementation - [x] test - fix `Ipv4Addr:is_global()` - [X] implementation - [x] test - [X] refactor the tests for IP properties. This makes the tests more verbose, but using macros have two advantages: - it will now be easier to add tests for all the new methods - we get clear error messages when a test is failing. For instance: ``` ---- net::ip::tests::ip_properties stdout ---- thread '<unnamed>' panicked at 'assertion failed: !ip!("fec0::").is_global()', src/libstd/net/ip.rs:2036:9 ``` Whereas previously it was something like ``` thread '<unnamed>' panicked at 'assertion failed: `(left == right)` left: `true`, right: `false`', libstd/net/ip.rs:1948:13 ``` ----------------------- # Ongoing discussions: ## Should `Ipv4Addr::is_global()` return `true` or `false` for reserved addresses? Reserved addresses are addresses that are matched by `Ipv4Addr::is_reserved()`. @the8472 [pointed out](#60145 (comment)) that [RFC 4291](https://tools.ietf.org/html/rfc4291#section-2.4) says IPv6 reserved addresses should be considered global: ``` Future specifications may redefine one or more sub-ranges of the Global Unicast space for other purposes, but unless and until that happens, implementations must treat all addresses that do not start with any of the above-listed prefixes as Global Unicast addresses. ``` We could extrapolate that this should also be the case for IPv4. However, it seems that [IANA considers them non global](https://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml) (see [my comment](#60145 (comment))) ### Final decision There seems to be a consensus that reserved addresses have a different meaning for IPv4 and IPv6 ([comment1](#60145 (comment)) [comment2](#60145 (comment)), so we can consider that RFC4291 does not apply to IPv4, and that reserved IPv4 addresses are _not_ global. ## Should `Ipv6Addr::is_unicast_site_local()` exist? @pusateri [noted](#60145 (comment)) that site-local addresses have been deprecated for a while by [RFC 3879](https://tools.ietf.org/html/rfc3879) and new implementations _must not_ support them. However, since this method is stable, removing does not seem possible. This kind of situation is covered by the RFC which stated that existing implementation _may_ continue supporting site-local addresses. ### Final decision Let's keep this method. It is stable already, and the RFC explicitly states that existing implementation may remain. --------- Note: I'll be AFK from April 27th to May 11th. Anyone should feel free to pick this up if the PR hasn't been merged by then. Sorry for dragging that for so long already.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I picked up my previous un-finished PR: #51832
Two tests are failing right now, but I'm working on it.
Ref: #27709