cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: [GitHub] cloudstack pull request: CLOUDSTACK-8506 - Make ACS compliant with...
Date Mon, 25 May 2015 11:56:35 GMT
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 <git@git.apache.org>:

> 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 <notifications@github.com
> <mailto:notifications@github.com>> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message