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.

Sign up for GitHub

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

Jump to bottom

Add a note for Ipv4Addr::to_ipv6_compatible #75150

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Add a note for Ipv4Addr::to_ipv6_compatible #75150

merged 1 commit into from
Sep 3, 2020

Conversation

nanpuyue
Copy link
Contributor

@nanpuyue nanpuyue commented Aug 4, 2020 •
edited
Loading

Previous discussion: #75019

I think adding a comment saying "This isn't typically the method you want; these addresses don't typically function on modern systems. Use to_ipv6_mapped instead." would be a good first step, whether this method gets marked as deprecated or not.

Originally posted by @joshtriplett in #75150 (comment)

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
@nanpuyue nanpuyue mentioned this pull request Aug 4, 2020
Merged
@nanpuyue
Copy link
Contributor Author

nanpuyue commented Aug 4, 2020

There is no replacement for Ipv4Addr::to_ipv6_compatible, it just should be deprecated for the following reasons:

  • The current version of IETF RFC 4291 was released on 2006-02-17. It was a long time ago and before the birth of Rust. The "IPv4-Compatible IPv6 address" was deprecated before 2006.

  • The current implementation of Ipv4Addr::to_ipv6_compatible and Ipv6Addr::to_ipv4 (IPv4-Compatible part) does not check whether the IPv4 address is a globally unique unicast address. This is incorrect (for example, conflicts with the unspecified address and the loopback address), and may mislead the caller.

Originally posted by @nanpuyue in #75019 (comment)

@kennytm
Copy link
Member

kennytm commented Aug 5, 2020

r? @LukasKalbertodt

@LukasKalbertodt
Copy link
Member

I am not quite convinced we want to hard-deprecate this method in std. While I absolutely I agree we should mention in the docs that these kinds of addresses were deprecated in the IETF RFC, I don't necessarily think this should translate 1:1 to the Rust standard library. People might have to deal with these IPv6 addresses in their code. I don't really see the advantage of deprecating it.

@rust-lang/libs any opinions? In particular, @BurntSushi, I think you are usually not a fan of hard deprecations?

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2020
@bors
Auto merge of rust-lang#75019 - nanpuyue:to_ipv4_mapped, r=LukasKalbe…
3df25ae 
…rtodt

Add Ipv6Addr::to_ipv4_mapped

* add Ipv6Addr::to_ipv4_mapped
* ~~deprecate Ipv4Addr::to_ipv6_compatible & Ipv6Addr::to_ipv4~~ reference: rust-lang#75150

According to [IETF RFC 4291](https://tools.ietf.org/html/rfc4291#page-10), the "IPv4-Compatible IPv6 address" is deprecated.

> 2.5.5.1.  IPv4-Compatible IPv6 Address
>
>    The "IPv4-Compatible IPv6 address" was defined to assist in the IPv6
>    transition.  The format of the "IPv4-Compatible IPv6 address" is as
>    follows:
>
>    |                80 bits               | 16 |      32 bits        |
>    +--------------------------------------+--------------------------+
>    |0000..............................0000|0000|    IPv4 address     |
>    +--------------------------------------+----+---------------------+
>
>    Note: The IPv4 address used in the "IPv4-Compatible IPv6 address"
>    must be a globally-unique IPv4 unicast address.
>
>    The "IPv4-Compatible IPv6 address" is now deprecated because the
>    current IPv6 transition mechanisms no longer use these addresses.
>    New or updated implementations are not required to support this
>    address type.

And the current implementation of `Ipv4Addr::to_ipv6_compatible`is incorrect: it does not check whether the IPv4 address is a globally-unique IPv4 unicast address.

Please let me know if there are any issues with this pull request.
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2020
@Dylan-DPC-zz
Copy link

Created a zulip thread to discuss this further.

@KodrAus
Copy link
Contributor

KodrAus commented Aug 25, 2020

Related: #75046

As noted the Ipv6Addr::to_ipv4 method is similarly inconsistent with the spec in ways that are difficult to nail down precisely.

I’m also not sure if we need to deprecate these methods or just point out in their docs how their behaviour might differ from what you expect. I have been finding usages on GitHub for to_ipv6_compatible, but haven’t found any that appear to expect more other than I have a Ipv4Addr but I want a Ipv6Addr.

@joshtriplett
Copy link
Member

I think adding a comment saying "This isn't typically the method you want; these addresses don't typically function on modern systems. Use to_ipv6_mapped instead." would be a good first step, whether this method gets marked as deprecated or not.

@spastorino spastorino added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 26, 2020
@bors
Copy link
Contributor

bors commented Aug 31, 2020

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

@CDirkx CDirkx mentioned this pull request Sep 1, 2020
Closed
@LukasKalbertodt
Copy link
Member

@nanpuyue could you change this PR to only add a note in the docs similar to what @joshtriplett wrote? Thus not adding the #[deprecated] attribute for now.

@nanpuyue
Copy link
Contributor Author

nanpuyue commented Sep 2, 2020

@LukasKalbertodt It's done.

@nanpuyue nanpuyue changed the title Deprecate Ipv4Addr::to_ipv6_compatible Add a note for Ipv4Addr::to_ipv6_compatible Sep 2, 2020
@LukasKalbertodt
Copy link
Member

Thanks!

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Sep 2, 2020

📌 Commit 3b29913 has been approved by LukasKalbertodt

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 2, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 3, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2020
@bors
Auto merge of rust-lang#76265 - Dylan-DPC:rollup-j3i509l, r=Dylan-DPC
4f66117 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75150 (Add a note for Ipv4Addr::to_ipv6_compatible)
 - rust-lang#76120 (Add `[T; N]::as_[mut_]slice`)
 - rust-lang#76142 (Make all methods of `std::net::Ipv4Addr` const)
 - rust-lang#76164 (Link to slice pattern in array docs)
 - rust-lang#76167 (Replace MinGW library hack with heuristic controlling link mode)
 - rust-lang#76204 (Rename and expose LoopState as ControlFlow)
 - rust-lang#76238 (Move to intra-doc links for library/core/src/iter/traits/iterator.rs)
 - rust-lang#76242 (Read: adjust a FIXME reference)
 - rust-lang#76243 (Fix typos in vec try_reserve(_exact) docs)
 - rust-lang#76245 (inliner: Avoid query cycles when optimizing generators)
 - rust-lang#76255 (Update books)
 - rust-lang#76261 (Use intra-doc links in `core::marker`)

Failed merges:

r? @ghost
@bors bors merged commit 536b0c0 into rust-lang:master Sep 3, 2020
@CDirkx CDirkx mentioned this pull request Nov 23, 2020
Closed
@CDirkx CDirkx mentioned this pull request May 23, 2021
Open
@cuviper cuviper added this to the 1.48.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@LukasKalbertodt LukasKalbertodt

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
10 participants
@nanpuyue @rust-highfive @kennytm @LukasKalbertodt @Dylan-DPC-zz @KodrAus @joshtriplett @bors @cuviper @spastorino