Re: Ip::Address::IsAnyAddr

I am sending a version 4 of the patch which is the same with Amos patch
but is a little smaller. IT is easier to see the changes.

But applying this patch I found some problems in icp* related code. This
problems has as result an assertion failure inside icp code.
I am attaching a patch which is required to allow icp works with
isAnyAddr patch, but maybe fixes bugs.

 From what I can understand reading about the icp protocol, it supports
only the IPv4 protocol: It contains in icp messages header a 32bit
number which represents the ip address of sender. This is mean that only
ipv4 addresses can used.

The icp-isanyaddr.patch:
  1) Use the ip::Address::SetIPv4 method to always convert to IPv4 the
ip addresses used to set the hostid field of the ICP header message.

  2) Inside neighbors_init() function there is the line:
       theIcpPublicHostID.GetInAddr( *((struct
in_addr*)&echo_hdr.shostid) );
    The above line sets the hostid field of the header of the icp echo
message, to theIcpPublciHostID. The problem is that this ipaddress is
not initialized yet. Will be initialized inside
icpIncomingConnectionOpened when this function called.
The icp-isanyadd.patch replaces the above line with a simple:
    echo_hdr.shostid = 0;
which is absolutely equivalent (but probably not the correct).

The icp-isanyaddr.patch is not a finished patch but I am posting it here
to show two (possible) problems:
  - The icp does not work with IPv4 so we may add checks to prevent
using ipv6 addresses
  - The echo_hdr message initialized with an IP address which is not
initialized yet.

On 07/23/2011 05:53 AM, Amos Jeffries wrote:
> 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