Skip to content
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

Remove Ipv6Addr::is_unicast_link_local_strict #85819

Merged
merged 1 commit into from
May 31, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented May 29, 2021

Removes the unstable method Ipv6Addr::is_unicast_link_local_strict and keeps the behaviour of Ipv6Addr::is_unicast_link_local, see also #85604 where I have tried to summarize related discussion so far.

My intent is for is_unicast_link_local, is_unicast_site_local and is_unicast_global to have the semantics of checking if an address has Link-Local, Site-Local or Global scope, see also #85696 which changes the behaviour of is_unicast_global and renames these methods to has_unicast_XXX_scope to reflect this.

For checking Link-Local scope we currently have two methods: is_unicast_link_local and is_unicast_link_local_strict. This is because of what appears to be conflicting definitions in IETF RFC 4291.

From IETF RFC 4291 section 2.4: "Link-Local unicast" (FE80::/10)

Address type         Binary prefix        IPv6 notation   Section
------------         -------------        -------------   -------
Unspecified          00...0  (128 bits)   ::/128          2.5.2
Loopback             00...1  (128 bits)   ::1/128         2.5.3
Multicast            11111111             FF00::/8        2.7
Link-Local unicast   1111111010           FE80::/10       2.5.6
Global Unicast       (everything else)

From IETF RFC 4291 section 2.5.6: "Link-Local IPv6 Unicast Addresses" (FE80::/64)

| 10 bits  |         54 bits         |          64 bits           |
+----------+-------------------------+----------------------------+
|1111111010|           0             |       interface ID         |
+----------+-------------------------+----------------------------+

With is_unicast_link_local checking FE80::/10 and is_unicast_link_local_strict checking FE80::/64.

There is also IETF RFC 5156 section 2.4 which defines "Link-Scoped Unicast" as FE80::/10.

It has been pointed out that implementations in other languages and the linux kernel all use FE80::/10 (#76098 (comment), #76098 (comment)).

Given all of this I believe the correct interpretation to be the following: All addresses in FE80::/10 are defined as having Link-Local scope, however currently only the block FE80::/64 has been allocated for "Link-Local IPv6 Unicast Addresses". This might change in the future however; more addresses in FE80::/10 could be allocated and those will have Link-Local scope. I therefore believe the current behaviour of is_unicast_link_local to be correct (if interpreting it to have the semantics of has_unicast_link_local_scope) and is_unicast_link_local_strict to be unnecessary, confusing and even a potential source of future bugs:

Currently there is no real difference in checking FE80::/10 or FE80::/64, since any address in practice will be FE80::/64. However if an application uses is_unicast_link_local_strict to implement link-local (so non-global) behaviour, it will be incorrect in the future if addresses outside of FE80::/64 are allocated.

r? @joshtriplett as reviewer of all the related PRs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2021

📌 Commit c1f0c15 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021
@bors
Copy link
Contributor

bors commented May 31, 2021

⌛ Testing commit c1f0c15 with merge dc08641...

@bors
Copy link
Contributor

bors commented May 31, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing dc08641 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2021
@bors bors merged commit dc08641 into rust-lang:master May 31, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 31, 2021
@CDirkx CDirkx deleted the is_unicast_link_local_strict branch May 31, 2021 07:35
Comment on lines 482 to 483
let unicast_link_local: u16 = 1 << 4;
let unicast_link_local_strict: u16 = 1 << 5;
let unicast_site_local: u16 = 1 << 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing with gaps ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bitset of which properties are true, one of the bits being unused shouldn't really matter. I think that is less invasive than renumbering all of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants