Skip to content

Rework Ipv6Addr::is_global to check for global reachability rather than global scope - #86634

Closed
CDirkx wants to merge 2 commits into
rust-lang:masterfrom
CDirkx:ip-globally-reachable
Closed

Rework Ipv6Addr::is_global to check for global reachability rather than global scope#86634
CDirkx wants to merge 2 commits into
rust-lang:masterfrom
CDirkx:ip-globally-reachable

Conversation

@CDirkx

@CDirkxCDirkx commented Jun 25, 2021

Copy link
Copy Markdown
Contributor

This PR changes the unstable method Ipv6Addr::is_global to make it consistent with Ipv4Addr::is_global, checking for reachability rather than global scope (see also #85696 fixing this conflation in Ipv6Addr::is_unicast_global).

To summarize, there are two concepts; an address can be globally reachable, and an address can have global scope. These concepts have been conflated in the past, but are not the same thing. There exist addresses with global scope that are not globally reachable (for example unique local addresses), and addresses that are globally reachable without having global scope (multicast addresses with non-global scope). is_unicast_global should be used to check for global scope, and is_global for global reachability according to the IANA IPv6 Special-Purpose Address Registry, like is done in Ipv4Addr::is_global and other language implementations. (To prevent this confusion in the future, #85696 renames is_unicast_global to has_unicast_global_scope)

The documentation of Ipv4Addr::is_global is also updated to make the two methods have similar descriptions and examples.

@rustbotrustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api [DEPRECATED; DO NOT USE] labels Jun 25, 2021
@rust-highfive

Copy link
Copy Markdown
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2021
Comment threadlibrary/std/src/net/ip.rs Outdated
Comment on lines 477 to 496

@CDirkxCDirkxJun 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Reworded to a (in my opinion) less cluttered non-exhaustive list. Instead of trying to list every type of address and exceptions, we list the most notable ones (mostly the ones Rust has methods for) and refer to the Special Address Registry for the complete overview. A similar thing is done in the description of Ipv6Addr::is_global.

Comment threadlibrary/std/src/net/ip.rs Outdated
Comment on lines 550 to 567

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Reordered the checks to match the order in the Special Address Registry.

Comment threadlibrary/std/src/net/ip.rs Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

"globally reachable" instead of "globally routable", as that is the terminology used in the Special Address Registry.

Comment threadlibrary/std/src/net/ip.rs Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If you're not connected to the internet, you still can't reach a "globally reachable" address. I hope this also makes it clear that this method doesn't actually do any network traffic.

Comment threadlibrary/std/src/net/ip.rs Outdated
Comment on lines 1235 to 1239

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Trying to avoid the confusion between "globally reachable" and "global scope", the rename in #85696 from is_unicast_global to has_unicast_global_scope should hopefully help with this. Another possibility would be to rename is_global to is_globally_reachable.

Comment threadlibrary/std/src/net/ip.rs Outdated
Comment threadlibrary/std/src/net/ip/tests.rs Outdated
Comment on lines 308 to 313

@CDirkxCDirkxJun 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

As mentioned in the PR description, multicast addresses with non-global scope are actually still globally reachable.

@CDirkx
CDirkxforce-pushed the ip-globally-reachable branch from 2be1e92 to 9ddd5a5CompareJuly 8, 2021 12:36
@CDirkx

Copy link
Copy Markdown
ContributorAuthor

Addressed an incorrect check pointed out in #86969 (comment)

@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021
@bors

bors commented Aug 2, 2021

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #87535) made this pull request unmergeable. Please resolve the merge conflicts.

@camelidcamelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2021
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…plett
Add `Ipv6Addr::is_benchmarking`
This PR adds the unstable method `Ipv6Addr::is_benchmarking`. This method is added for parity with `Ipv4Addr::is_benchmarking`, and I intend to use it in a future rework of `Ipv6Addr::is_global` (edit: rust-lang#86634) to more accurately follow the [IANA Special Address Registry](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) (like is done in `Ipv4Addr::is_global`).
With `Ipv6Addr::is_benchmarking` and `Ipv4Addr::is_benchmarking` now both existing, `IpAddr::is_benchmarking` is also added.
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…plett
Add `Ipv6Addr::is_benchmarking`
This PR adds the unstable method `Ipv6Addr::is_benchmarking`. This method is added for parity with `Ipv4Addr::is_benchmarking`, and I intend to use it in a future rework of `Ipv6Addr::is_global` (edit: rust-lang#86634) to more accurately follow the [IANA Special Address Registry](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) (like is done in `Ipv4Addr::is_global`).
With `Ipv6Addr::is_benchmarking` and `Ipv4Addr::is_benchmarking` now both existing, `IpAddr::is_benchmarking` is also added.
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2021
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2022
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api [DEPRECATED; DO NOT USE] and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api [DEPRECATED; DO NOT USE] labels Mar 20, 2022
@JohnCSimon

Copy link
Copy Markdown

Ping from triage:
@kennytm what is the status of this review?

@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@Mark-Simulacrum

Copy link
Copy Markdown
Member

Approved #99957, so going to go ahead and close this PR. #99957 contains the commits from here though.

Thanks!

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 22, 2022
…se, r=Mark-Simulacrum
Rework Ipv6Addr::is_global to check for global reachability rather than global scope - rebase
Rebasing of pull request rust-lang#86634 off of master to try and get the feature "ip" stabilized.
I also found a test failure in the rebase that is_global was considering the benchmark space to be globally reachable.
This is related to my other rebasing pull request rust-lang#99947
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ioArea: `std::io`, `std::fs`, `std::net` and `std::path`S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@CDirkx@rust-highfive@bors@JohnCSimon@Mark-Simulacrum@kennytm@camelid@rustbot