Re: IpAddress comparison operators lie

On 05/01/2010 01:43 AM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> I appreciate ACL needs, but I think it is not acceptable to ignore
>> ports in these operators because they should compare IpAddresses and not
>> some arbitrary parts of those objects. I see two options:
>>
>> A) Remove all overloaded comparison operators from IpAddress. Add
>> compareWithoutPort() and compareWithPort() methods that return +1/0/-1
>> as expected, following all traditional logic rules. This option will
>> force folks to think which comparison to use. It will add more work to
>> those who want to use C++ operators (e.g., in std::map)
>>
>> B) Fix all overloaded comparison operators in IpAddress so that they
>> apply to the entire object and follow all traditional logic rules. Add
>> compareWithoutPort() method. Change ACLs to use compareWithoutPort().
>> This option may caught some by surprise because they might think that
>> IpAddress does not include port at all and it would take more work to
>> replace all current incorrect uses.
>>
>> Long-term, the right thing to do is to have portless and portfull
>> address classes (one using the other) so that the code that does not
>> care about ports is not forced to ignore them. It is non-trivial to
>> design this right though and the change would be rather big, so let's
>> not go there for now.
>
> Post is not the only field of the sockaddr_in6 being ignored by the
> "Address" boolean operators. There is family etc.

Right, but the general rule still applies: Any comparison operator must
apply to the whole object. If such a comparison operator is not a good
idea because it is too confusing and/or because a lot of code assumes
otherwise, then we should remove such a comparison operator.

> This object in abstract is primarily a raw address. It's Get*()
> operators can retrieve the address-related details which 'happen' to be
> passed with it for testing.

I think the primary problem here is that there is currently no good
definition of (or agreement on) what IpAddress class is. "Raw address"
means different things to different people. IMO, IpAddress cannot be
defined as "an IP address" because it holds more than the IP address.

I think that IpAddress can be defined as "socket address", with the
scope currently limited to IP-based sockets. The scope may change if we
want to support Unix Domain Sockets without adding a few special comm_*
functions for them (UDS uses AF_UNIX and socketaddr_un).

> The shortest path to conformity is to drop the boolean address operators
> and make the code use a.matchIPaddr(b) <> 0 without the convenience of
> (a < b) and memcmp() for matching the whole thing.

I agree that dropping the operators is the best way forward (Option A).

If socket addresses never contain garbage in unused parts, then memcmp()
sounds like a good tool for the new compareWhole() method!

Will you implement these changes? In Squid v3.1?

Thank you,

Alex.

>>>> There may be more inconsistencies; I have not checked all possible
>>>> combinations.
>>>>
>>>> These bugs make it impossible to reliably sort or compare addresses
>>>> using C++ operators. However, I do not know whether some code already
>>>> relies on this broken behavior. Does it? In other words, should we just
>>>> fix the operators or is there more to it?
>>> None that I know of. +1 to fix IpAddress.
>>
>> I disagree that ignoring ports in overloaded comparison operators is OK
>> so our notions of a fix differ. Will any of the above two options work
>> for you?
>>
>
> Well I meant make < consistent with <= and add the unit tests.
Received on Sat May 01 2010 - 15:42:12 MDT