Concept ACK

ACK c10f27f

I've reviewed the code and it looks good. Also ran unit tests and make check completes successfully.

@shobitb Thanks a lot for reviewing! I think you might have gotten the commit id wrong when ACK:ing. The last commit id in the sequence of commits is the one used when ACK:ing a PR assuming one ACK:s the entire PR and not just a subset of it :)

Code review ACK 54548ba
It at first surprised me that there are no tests for IPv6ToString. However there are for the higher-level functions. As this passes the CNetAddr ToString tests, I think this is covered sufficiently.

According to #21982 (comment), no longer returning scoped ids was an intentional part of this PR. Maybe that wasn't obvious to reviewers. After test coverage was removed by #21689, no other tests signalled the change.

It would be good to be more explicit in the PR description regarding why make the change, and straightforward about what support was being dropped.

There seems to be two "functionalities" being discussed together, it took me some time to figure it out:

1. The functionality of IPv6 scope id, introduced in Net: Add IPv6 Link-Local Address Support #7570. That is still working, has not been removed.
2. The printout of such IPv6 addresses - before we used the system getnameinfo() which included %zoneid (on all or most platforms). Now we roll our own which does not include %zoneid.

For reference, here is the FreeBSD implementation of getnameinfo() (which is under... eh... the BSD license):

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/lib/libc/net/getnameinfo.c#L43-L46

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/lib/libc/net/getnameinfo.c#L358-L375

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/sys/netinet6/in6.h#L296-L297

@vasild

Exactly. Thanks for bringing some sense to this.

What this entire discussion boils down to is if the expectation is that CNetAddr::ToStringIP() should print the IPv6 address or the IPv6 socket address:

• IPv6 address (std::net::Ipv6Addr): "IPv6 addresses are defined as 128-bit integers in IETF RFC 4291. They are usually represented as eight 16-bit segments."
• IPv6 socket address (std::net::SocketAddrV6): "IPv6 socket addresses consist of an IPv6 address, a 16-bit port number, as well as fields containing the traffic class, the flow label, and a scope identifier (see IETF RFC 2553, Section 3.3 for more details)."

Indeed. What does not make sense to me is why this PR was not upfront about this, nor the reasons that were given to have the only test coverage removed willy-nilly allowing this to be changed without tests warning about it, nor why we should break user space to follow the rustlang version without discussion. If those questions do not make sense, then I apologize.

The comment I referred to above in #21982 (comment):

If you by "functionality" mean not appending an unwanted %zone_index string to CNetAddr::ToString(), then yes that non-appending of an unwanted string is obviously intentional. The new implementation of IPv6ToString (which is called from CNetAddr::ToString) follows Rust's std::net::Ipv6Addr which does not append %zone_index. FWIW that is how the previous implementation of CNetAddr::ToString worked under macOS.

Yes, as the removed test and as #21690 showed, macOS returns a different getnameinfo format for scoped ids, without the zone id, from all the other platforms that do provide support for it. Why should we align with the macOS exception without discussing it? Does it not make sense to call attention to this change?

It should be easy to append %zoneid like @laanwj suggests?

It should be easy to append %zoneid like @laanwj suggests?

#21985