Re: [Openvpn-devel] [PATCH] Avoid using ~0

[Gert D. <ge...@gr...>]

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.

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...