Re: Finalizing source IP version

On 09/08/2011 06:37 PM, Amos Jeffries wrote:
> On 09/09/11 02:51, Alex Rousskov wrote:
>> On 09/08/2011 08:11 AM, Amos Jeffries wrote:
>>> ------------------------------------------------------------
>>> revno: 11715
>>> fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3335
>>> committer: Amos Jeffries<squid3_at_treenet.co.nz>
>>> branch nick: trunk
>>> timestamp: Fri 2011-09-09 02:11:05 +1200
>>> message:
>>> Bug 3335: ICAP service is down
>>> modified:
>>> src/adaptation/icap/Xaction.cc
>>
>>> getOutgoingAddress(NULL, connection);
>>> + if (connection->remote.IsIPv4()&& !connection->local.SetIPv4()) {
>>> + // This should never happen. getOutgoing should match by
>>> family or skip.
>>> + Must(connection->local.IsAnyAddr());
>>> + return;
>>> + }
>>
>>
>> Hi Amos,
>>
>> IIRC, you have mentioned that already, but I want to stress that
>> there is something really wrong with our code if the above explicit
>> SetIPv4() call is necessary for a high-level module opening a
>> connection. There should be virtually no IP-version specific code in
>> high-level modules such as ICAP!
>>
>> Judging by the name of it, it should be getOutgoingAddress()
>> responsibility to finalize the local source address. However, if that
>> address is not set for whatever reason, the connection establishing code
>> in Comm should not just fail but should select the right IP version if a
>> specific IP version must be selected at that time.
>
> Yes. I'm following up with a move of it into getOutgoingAddress where I
> agree it should be.
>
> Conceptually the change applied and the change to using
> getOutgoingAddress() are both right. The code doing "new
> Comm::Connection" is responsible for setting the template up correctly.

The correct default for the source address is "any address", to be
finalized by the connecting code and the destination address. This is
not something a regular "new Connection" caller should worry about, IMO.

> And the default :: is no longer a valid ANY_ADDR in IPv4 since the
> ANY_ADDR change a few weeks back prevented :: being symmetrical with
> 0.0.0.0 in its IsIPv4()/IsIPv6() results.

If I am an ICAP code writer, for example, I do not know what the above
paragraph is talking about. I just want to connect to the ICAP service
at the configured address. I know nothing about IP versions, ANY_ADDR,
symmetry, etc.

> As it stands today, we default to 0.0.0.0 for IPv4-only systems (thus
> --disable-ipv6 "works" as people are constantly pointing out). Default
> to :: for IPv6-enabled systems, and leaving the caller code in full
> responsibility of using SetPv4() when needed (like this bug) for
> split-stack.

This seems to be the wrong default, IMHO. The default should be "any
address suitable for connecting to the destination address". It should
not be the caller responsibility to know what is suitable and whether
they are running on a split-stack machine!

I do not know whether implementing the right default is technically
possible with the current IP Address and Connection classes, but that
should be the goal as we polish them, IMO.

HTH,

Alex.
Received on Fri Sep 09 2011 - 15:07:02 MDT