On Thu, Mar 29, 2012 at 9:50 AM, Gert Doering <g...@greenie.muc.de> wrote:
> Hi,
>
> On Wed, Mar 28, 2012 at 08:44:18PM +0200, Alon Bar-Lev wrote:
>> To avoid casting use two constants:
>> 1. IPV4_NETMASK_HOST
>
> Fine with that one.
>
>> 2. dwMINUS_ONE
>
> I think this is way over board. It should be possible to spell "-1" as
> such - and it doesn't increase readability to introduce a MINUS_ONE
> define here. If at all, introduce a "NO_IF_INDEX_FOUND" #define for
> the "find tap ifindex" logic - but please make it a separate patch.
>
> These are unrelated issues and must be reviewed independently (especially
> "different people understand the respective bits of the code and can give
> their ACK").
>
>> @@ -1530,7 +1530,7 @@ print_in6_addr_netbits_only( struct in6_addr
>> network_copy, int netbits,
>> if ( bits_to_clear >= 8 )
>> { network_copy.s6_addr[byte--] = 0; bits_to_clear -= 8; }
>> else
>> - { network_copy.s6_addr[byte--] &= (~0 << bits_to_clear); bits_to_clear
>> = 0; }
>> + { network_copy.s6_addr[byte--] &= (IPV4_NETMASK_HOST <<
>> bits_to_clear); bits_to_clear = 0; }
>> }
>
> This particular one should NOT get "IPV4_NETMASK_HOST", as it isn't IPv4.
>
> "0xff" would be fine, or just leave it at "~0".
>
>> - unsigned int lowest_metric = ~0;
>> + unsigned int lowest_metric = (unsigned int)-1;
>
> This bit is not looking good. If "-1" is a valid value for "lowest_metric"
> (to signal "not set" etc) we shouldn't cast "-1" to (unsigned int) but
> use a signed int here.
>
> This should NOT be "just patched because we're it" but better understood
> and properly fixed.
OK.
Fair enough, although I want to focus on the major cleanups right now.
If we focus on the minor ones we get progress but much slower.
Alon.
