This code is a bug. To split in two conditions range 1 is a /31 or range 1
is not a /31;
precondition: endIp1Long == startIp1Long + 1
1: startIp2Long - startIp1Long == startIp2Long - (endIp1Long -
ip31bitPrefixOffSet) # - startip2Long
2: - startIp1Long == - (endIp1Long - ip31bitPrefixOffSet)
# * -1
3: startIp1Long == endIp1Long - ip31bitPrefixOffSet
# + ip31bitPrefixOffSet
4: startip1Long + 1 == endIp1Long
# precondition
5: true ==>> return false
precondition: endIp1Long != startIp1Long + 1 # no /31 network
1 through 4 the same
5: false ==>> do further checking
the conditional block comes down to
if(endIp1Long == startIp1Long + 1)
{
return false; // no further overlap is tested for!!!
}
This means that if range 1 is a slash thirty one no further overlap
checking is done, irrespective of range 2. I don't know if this is right,
but the code is not clear in this perspect for sure.
Op za 23 mei 2015 om 12:00 schreef wilderrodrigues <[email protected]>:
> Github user wilderrodrigues commented on a diff in the pull request:
>
> https://github.com/apache/cloudstack/pull/292#discussion_r30943691
>
> --- Diff: utils/src/com/cloud/utils/net/NetUtils.java ---
> @@ -329,35 +329,46 @@ public static String getMacAddress(InetAddress
> address) {
> return sb.toString();
> }
>
> - public static long getMacAddressAsLong(InetAddress address) {
> + public static long getMacAddressAsLong(final InetAddress address)
> {
> long macAddressAsLong = 0;
> try {
> - NetworkInterface ni =
> NetworkInterface.getByInetAddress(address);
> - byte[] mac = ni.getHardwareAddress();
> + final NetworkInterface ni =
> NetworkInterface.getByInetAddress(address);
> + final byte[] mac = ni.getHardwareAddress();
>
> for (int i = 0; i < mac.length; i++) {
> - macAddressAsLong |= ((long)(mac[i] & 0xff) <<
> (mac.length - i - 1) * 8);
> + macAddressAsLong |= (long)(mac[i] & 0xff) <<
> (mac.length - i - 1) * 8;
> }
>
> - } catch (SocketException e) {
> + } catch (final SocketException e) {
> s_logger.error("SocketException when trying to retrieve
> MAC address", e);
> }
>
> return macAddressAsLong;
> }
>
> - public static boolean ipRangesOverlap(String startIp1, String
> endIp1, String startIp2, String endIp2) {
> - long startIp1Long = ip2Long(startIp1);
> + public static boolean ipRangesOverlap(final String startIp1,
> final String endIp1, final String startIp2, final String endIp2) {
> + final long startIp1Long = ip2Long(startIp1);
> long endIp1Long = startIp1Long;
> if (endIp1 != null) {
> endIp1Long = ip2Long(endIp1);
> }
> - long startIp2Long = ip2Long(startIp2);
> + final long startIp2Long = ip2Long(startIp2);
> long endIp2Long = startIp2Long;
> if (endIp2 != null) {
> endIp2Long = ip2Long(endIp2);
> }
>
> + // check if the gatewayip is the part of the ip range being
> added.
> + // RFC 3021 - 31-Bit Prefixes on IPv4 Point-to-Point Links
> + // GW Netmask Stat IP End IP
> + // 192.168.24.0 - 255.255.255.254 - 192.168.24.0 -
> 192.168.24.1
> + // https://tools.ietf.org/html/rfc3021
> + // Added by Wilder Rodrigues
> + final int ip31bitPrefixOffSet = 1;
> + if (startIp2Long - startIp1Long == startIp2Long - (endIp1Long
> - ip31bitPrefixOffSet)) {
> --- End diff --
>
> Nope, it's not. You should check the configurationImpl and see how the
> checks are done separately
>
> Or then look at the chat last night. The iPOverlaps method doens't
> check cidr, as I said before. It checks that there are only 2 ips in the
> range
>
> Why that? Because the conditions and all the ConfigurationImpl are
> doing the job in many steps
>
> We can discuss it further on Tuesday.
>
> Sent from my iPhone
>
> On 23 May 2015, at 11:09, Daan Hoogland <[email protected]
> <mailto:[email protected]>> wrote:
>
>
> In utils/src/com/cloud/utils/net/NetUtils.java<
> https://github.com/apache/cloudstack/pull/292#discussion_r30943403>:
>
> > long endIp2Long = startIp2Long;
> > if (endIp2 != null) {
> > endIp2Long = ip2Long(endIp2);
> > }
> >
> > + // check if the gatewayip is the part of the ip range being
> added.
> > + // RFC 3021 - 31-Bit Prefixes on IPv4 Point-to-Point Links
> > + // GW Netmask Stat IP End IP
> > + // 192.168.24.0 - 255.255.255.254 - 192.168.24.0 -
> 192.168.24.1
> > + // https://tools.ietf.org/html/rfc3021
> > + // Added by Wilder Rodrigues
> > + final int ip31bitPrefixOffSet = 1;
> > + if (startIp2Long - startIp1Long == startIp2Long -
> (endIp1Long - ip31bitPrefixOffSet)) {
>
>
> for any endIp1Long == startIp1Long +1 this condition is true, no
> matter what startIp2Long is let alone endIp2Long. So is it the intention to
> check (startIp1Long == endIp1Long - ip31bitPrefixOffSet) and return false
> if it is? This comes down to a check if this is indeed a /31 range and
> makes no sense to me.
>
> —
> Reply to this email directly or view it on GitHub<
> https://github.com/apache/cloudstack/pull/292/files#r30943403>.
>
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at [email protected] or file a JIRA ticket
> with INFRA.
> ---
>
