Re: Ip::Address::IsAnyAddr

On 23/07/11 04:18, Tsantilas Christos wrote:
> On 07/22/2011 02:27 PM, Amos Jeffries wrote:
>> On 22/07/11 22:53, Tsantilas Christos wrote:
>>> I did a small research and I am finding many problems with the current
>>> implementation and use of the Ip::Address::IsAnyAddr() method.
>>>
>>> To summarize the problem:
>>> - Currently the ip::Address::IsAnyAddr() return true only for ipv6
>>> anyaddr (0000:0000:0000:0000:0000:0000:0000:0000). It return false when
>>> we have an ipv4 anyaddr (0000:0000:0000:0000:0000:FFFF:0000:0000)
>>> This is not normal.
>>>
>>> - The ip::Address::IsIPv6 and ip::Address::IsIPv4 methods return true in
>>> the case the IsAnyAddr return true, this is mean that the
>>> ip::Address::IsIPv4 return false in the case of ipv4 anyaddr.
>>> This is a bug.
>>>
>>>
>>> Searching in the code I am finding other bugs too. For example:
>>> - inside Ip::Address::SetIPv4(). If we have an ip address which is
>>> (ipv6) anyaddr and we call SetIPv4 for this ip address to convert it to
>>> ipv4 ip address, the conversion will done, but the ip address will not
>>> considred as anyaddr any more, because the IsAnyAddr will return false,
>>> for ipv4 anyaddr.
>>>
>>> - inside cache_cf.cc file inside dump_generic_http_port function there
>>> is the code:
>>> if (s->s.IsAnyAddr() && !s->s.IsIPv6())
>>> storeAppendPrintf(e, " ipv4");
>>> The if condition in the above statement can never be true. But the s->s
>>> can be an ipv4 anyaddr.
>>>
>>> Also in the code I am finding other places where my sense is that the
>>> code will not work as expected in the case we are listening to an ipv4
>>> anyaddr ip address.
>>>
>>> I am attaching a patch here which:
>>> - Ip::Address::IsAnyAddr() return true for both ipv6 or ipv4 anyaddr.
>>> - IsIPv6 return true only in the case of ipv6 anyaddr. If the ip address
>>> is an ipv4 anyaddr return false.
>>> - IsIPv4 return true only in the case of ipv4 anyaddr. If the ip address
>>> is an ipv6 anyaddr return false.
>>>
>>> From what I can see looking the code this is what expected by these
>>> methods.
>>> I did not do extended tests, but looks that this is the best solution...
>>>
>>
>> Hmm. Okay, but not quite far enough. As Alex pointed out IsNoAddr() also
>> need to be kept in sync. Also IsLocalhost().
> The IsLocalhost() is already implemented.
>
> About the IsNoAddr I am suggesting to not change it. Currently someone
> uses the SetNoAddr to set it and use the IsNoAddr method to check if we
> do not have any valid ipaddress. I think does not make sense to have one
> ipv4 non valid address and one ipv6 non valid address.
>
> The localhost and anyaddr are both valid addresses which can be ipv6 or
> ipv4.

The only use would be for logging display and parser testing when
reading 255.255.255.255. Minor but useful. Particularly if IsNoAddr() is
true for both.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.14
   Beta testers wanted for 3.2.0.9