cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wilderrodrigues <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-8506 - Make ACS compliant with...
Date Sat, 23 May 2015 10:00:36 GMT
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
View raw message