Remove Ip::Address implicit sockaddr_in6 conversion

[yadij]

Migration to sockaddr_storage API.

[rousskov]

I hope this particular code will disappear due to another change request, but the idea expressed here applies to (another area in) adjusted code as well.

---

If possible, please avoid never-nil pointers to local objects and reduce the difference with the current code:

Suggested change

	auto *insock = reinterpret_cast<struct sockaddr_in *>(&ss);
	auto &insock = reinterpret_cast<struct sockaddr_in &>(ss);

Same for the other similar changes in this PR, of course.

[yadij]

Good point. I will do once that other discussion is resolved.

[rousskov]

I support adding a sockaddr_storage constructor, but why remove this constructor? What is wrong with it (other than the fact that it allows implicit conversions -- a problem that should be fixed by adding "explicit" rather than by removing)?

PR description says "Migration to sockaddr_storage API", but that phrase does not answer my question (and raises more red flags). If that phrase was meant to explain why sockaddr_in6 conversion constructor is removed, please rephrase. Otherwise, please remove that PR description phrase completely, so that we do not have to argue about (and adjust it to clarify) its meaning.

[yadij]

This constructor (and matching assignment operator) are no longer used after this PR updates the callers to sockaddr_storage.

[rousskov]

This constructor (and matching assignment operator) are no longer used

... but their code is still there; it is just not wrapped in a Address methods anymore and is now duplicated and hidden. I was going to flag that problem in later review iterations (if still necessary)...

For now, I was hoping that you will agree to restore those methods, preserve Ip::Address API symmetry with respect to IPv4/IPv6, and focus on prohibiting implicit conversions and/or on adding explicit sockaddr_storage conversion support. The last two are good goals that can be accomplished without removing those IPv6-specific methods.

[rousskov]

Can we do the opposite -- continue to use sockaddr_in6 ipv6val definition and then just cast ipv6 to a sockaddr_storage reference at the time of conversion to Ip::Address? Doing so would simplify and clarify this code. Right now, code further below appears to configure one variable (ipv6val) but use another (ss). If we do not have to use sockaddr_storage as ip6val storage while configuring ipv6val, then let's remove that complication.

Suggested change

	struct sockaddr_storage ss;
	auto *ip6val = reinterpret_cast<struct sockaddr_in6*>(&ss);
	struct sockaddr_in6 ip6val;

... and adjust Ip::Address construction accordingly.

Same for other similar test cases, of course.

[yadij]

Not without adding more technical debt for the future.
sockaddr_storage is defined as being large enough to store the largest sockaddr_* type. That includes experimental IPv7+ address types and some edge-cases like interface-scoped IPv6. If we went with sockaddr_in6 we would lock Squid to only supporting "pure" IPv6.

I would like to get the API as minimal as possible without locking out capabilities like that. But not to go as far as a C library with sockaddr * raw-pointers.

[rousskov]

Not without adding more technical debt for the future.

I do not see any technical debt. I suspect that my suggestion was misinterpreted.

I now also realize that the second part of my suggestion was wrong -- one must not cast a sockaddr_in6 variable to sockaddr_storage. Only the opposite cast is safe!

sockaddr_storage is defined as being large enough to store the largest sockaddr_* type. That includes experimental IPv7+ address types and some edge-cases like interface-scoped IPv6. If we went with sockaddr_in6 we would lock Squid to only supporting "pure" IPv6.

I see no relationship between the facts mentioned above and my suggestion (that we should (continue to) use sockaddr_in6 type to create a sockaddr_in6 address for the test case). We obviously want to test with Ip::Address with IPv6 addresses. Let's (continue to) use the corresponding sockaddr_in6 type for that test. There is no reason to outlaw that type; we ought to support it at least until the last sockaddr_in6 expression is gone from Squid primary code.

• In this PR, this code should continue to (naturally) configure its IPv6 input using sockaddr_in6.

• In a future PR, when we add support for experimental IPv7 addresses, the test code will (naturally) configure its IPv7 input using sockaddr_in7.

We can also add sockaddr_storage tests, of course. They can (and probably should) reuse the bulk of existing sockaddr_in6 and sockaddr_in test code, memcopying configured sockaddr_in... addresses to sockaddr_storage variables.

I would like to get the API as minimal as possible without locking out capabilities like that.

No API changes or "locking out capabilities" suggested in this change request.

[rousskov]

If possible:

Suggested change

	Ip::Address bnIPA(ss);
	const Ip::Address bnIPA(reinterpret_cast<struct sockaddr_storage &>(ip6val));

[yadij]

I am reluctant to do this one. While it matches up with the earlier IPv4 constructor test case, the intention of this case is to verify that the sockaddr_storage content is correctly interpreted by Ip::Address when being set via an ipv6val pointer/reference.

[rousskov]

I will rephrase to avoid contaminating this trivial change request with complex changes from another one:

Suggested change

	Ip::Address bnIPA(ss);
	const Ip::Address bnIPA(ss);

[rousskov]

This constructor (and matching assignment operator) are no longer used

... but their code is still there; it is just not wrapped in a Address methods anymore and is now duplicated and hidden. I was going to flag that problem in later review iterations (if still necessary)...

For now, I was hoping that you will agree to restore those methods, preserve Ip::Address API symmetry with respect to IPv4/IPv6, and focus on prohibiting implicit conversions and/or on adding explicit sockaddr_storage conversion support. The last two are good goals that can be accomplished without removing those IPv6-specific methods.

[rousskov]

Not without adding more technical debt for the future.

I do not see any technical debt. I suspect that my suggestion was misinterpreted.

I now also realize that the second part of my suggestion was wrong -- one must not cast a sockaddr_in6 variable to sockaddr_storage. Only the opposite cast is safe!

sockaddr_storage is defined as being large enough to store the largest sockaddr_* type. That includes experimental IPv7+ address types and some edge-cases like interface-scoped IPv6. If we went with sockaddr_in6 we would lock Squid to only supporting "pure" IPv6.

I see no relationship between the facts mentioned above and my suggestion (that we should (continue to) use sockaddr_in6 type to create a sockaddr_in6 address for the test case). We obviously want to test with Ip::Address with IPv6 addresses. Let's (continue to) use the corresponding sockaddr_in6 type for that test. There is no reason to outlaw that type; we ought to support it at least until the last sockaddr_in6 expression is gone from Squid primary code.

• In this PR, this code should continue to (naturally) configure its IPv6 input using sockaddr_in6.

• In a future PR, when we add support for experimental IPv7 addresses, the test code will (naturally) configure its IPv7 input using sockaddr_in7.

We can also add sockaddr_storage tests, of course. They can (and probably should) reuse the bulk of existing sockaddr_in6 and sockaddr_in test code, memcopying configured sockaddr_in... addresses to sockaddr_storage variables.

I would like to get the API as minimal as possible without locking out capabilities like that.

No API changes or "locking out capabilities" suggested in this change request.

[rousskov]

I will rephrase to avoid contaminating this trivial change request with complex changes from another one:

Suggested change

	Ip::Address bnIPA(ss);
	const Ip::Address bnIPA(ss);