Updated Branches: refs/heads/master d626e29fa -> 5b85edb96
bug CS-16034 getRandomIp can return -1 unexpectedly also fixes unit test failures Project: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/commit/5b85edb9 Tree: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/tree/5b85edb9 Diff: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/5b85edb9 Branch: refs/heads/master Commit: 5b85edb96186583eae08d96e01c4ef4ee31e6222 Parents: d626e29 Author: Chiradeep Vittal <[email protected]> Authored: Thu Aug 16 11:28:11 2012 -0700 Committer: Chiradeep Vittal <[email protected]> Committed: Thu Aug 16 11:42:25 2012 -0700 ---------------------------------------------------------------------- .../com/cloud/network/guru/GuestNetworkGuru.java | 3 +- utils/src/com/cloud/utils/net/NetUtils.java | 36 +++++++++----- utils/test/com/cloud/utils/net/NetUtilsTest.java | 4 +- 3 files changed, 28 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/5b85edb9/server/src/com/cloud/network/guru/GuestNetworkGuru.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/network/guru/GuestNetworkGuru.java b/server/src/com/cloud/network/guru/GuestNetworkGuru.java index d1dc286..212de2f 100755 --- a/server/src/com/cloud/network/guru/GuestNetworkGuru.java +++ b/server/src/com/cloud/network/guru/GuestNetworkGuru.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.Set; +import java.util.SortedSet; import java.util.TreeSet; import javax.ejb.Local; @@ -239,7 +240,7 @@ public abstract class GuestNetworkGuru extends AdapterBase implements NetworkGur public Ip4Address acquireIp4Address(Network network, Ip4Address requestedIp, String reservationId) { List<String> ips = _nicDao.listIpAddressInNetwork(network.getId()); String[] cidr = network.getCidr().split("/"); - Set<Long> usedIps = new TreeSet<Long>(); + SortedSet<Long> usedIps = new TreeSet<Long>(); if (requestedIp != null && requestedIp.equals(network.getGateway())) { s_logger.warn("Requested ip address " + requestedIp + " is used as a gateway address in network " + network); http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/5b85edb9/utils/src/com/cloud/utils/net/NetUtils.java ---------------------------------------------------------------------- diff --git a/utils/src/com/cloud/utils/net/NetUtils.java b/utils/src/com/cloud/utils/net/NetUtils.java index e429022..1341338 100755 --- a/utils/src/com/cloud/utils/net/NetUtils.java +++ b/utils/src/com/cloud/utils/net/NetUtils.java @@ -32,6 +32,7 @@ import java.util.Formatter; import java.util.List; import java.util.Random; import java.util.Set; +import java.util.SortedSet; import java.util.StringTokenizer; import java.util.TreeSet; import java.util.regex.Matcher; @@ -651,39 +652,48 @@ public class NetUtils { * @param avoid set of ips to avoid * @return ip that is within the cidr range but not in the avoid set. -1 if unable to find one. */ - public static long getRandomIpFromCidr(String startIp, int size, Set<Long> avoid) { + public static long getRandomIpFromCidr(String startIp, int size, SortedSet<Long> avoid) { return getRandomIpFromCidr(ip2Long(startIp), size, avoid); } /** * Given a cidr, this method returns an ip address within the range but - * is not in the avoid list. + * is not in the avoid list. + * Note: the gateway address has to be specified in the avoid list * - * @param startIp ip that the cidr starts with + * @param cidr ip that the cidr starts with * @param size size of the cidr * @param avoid set of ips to avoid * @return ip that is within the cidr range but not in the avoid set. -1 if unable to find one. */ - public static long getRandomIpFromCidr(long cidr, int size, Set<Long> avoid) { + public static long getRandomIpFromCidr(long cidr, int size, SortedSet<Long> avoid) { assert (size < 32) : "You do know this is not for ipv6 right? Keep it smaller than 32 but you have " + size; long startNetMask = ip2Long(getCidrNetmask(size)); - long startIp = (cidr & startNetMask) + 2; - int range = 1 << (32 - size); + long startIp = (cidr & startNetMask) + 1; //exclude the first ip since it isnt valid, e.g., 192.168.10.0 + int range = 1 << (32 - size); //e.g., /24 = 2^8 = 256 + range = range -1; //exclude end of the range since that is the broadcast address, e.g., 192.168.10.255 - if (avoid.size() > range) { + if (avoid.size() >= range) { return -1; } - - for (int i = 0; i < range; i++) { - int next = _rand.nextInt(range); - if (!avoid.contains(startIp + next)) { - return startIp + next; + + //Reduce the range by the size of the avoid set + //e.g., cidr = 192.168.10.0, size = /24, avoid = 192.168.10.1, 192.168.10.20, 192.168.10.254 + // range = 2^8 - 1 - 3 = 252 + range = range - avoid.size(); + int next = _rand.nextInt(range); //note: nextInt excludes last value + long ip = startIp + next; + for (Long avoidable : avoid) { + if (ip >= avoidable) { + ip++; + } else { + break; } } - return -1; + return ip; } public static String getIpRangeStartIpFromCidr(String cidr, long size) { http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/5b85edb9/utils/test/com/cloud/utils/net/NetUtilsTest.java ---------------------------------------------------------------------- diff --git a/utils/test/com/cloud/utils/net/NetUtilsTest.java b/utils/test/com/cloud/utils/net/NetUtilsTest.java index bab1406..1eccba3 100644 --- a/utils/test/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/test/com/cloud/utils/net/NetUtilsTest.java @@ -17,6 +17,7 @@ package com.cloud.utils.net; import java.util.Set; +import java.util.SortedSet; import java.util.TreeSet; import junit.framework.TestCase; @@ -37,7 +38,7 @@ public class NetUtilsTest extends TestCase { ip = NetUtils.getRandomIpFromCidr(cidr, 8, new TreeSet<Long>()); assertEquals("The ip " + NetUtils.long2Ip(ip) + " retrieved must be within the cidr " + cidr + "/8", cidr.substring(0, 4), NetUtils.long2Ip(ip).substring(0, 4)); - Set<Long> avoid = new TreeSet<Long>(); + SortedSet<Long> avoid = new TreeSet<Long>(); ip = NetUtils.getRandomIpFromCidr(cidr, 30, avoid); assertTrue("We should be able to retrieve an ip on the first call.", ip != -1); avoid.add(ip); @@ -49,6 +50,7 @@ public class NetUtilsTest extends TestCase { assertTrue("We should be able to retrieve an ip on the third call.", ip != -1); assertTrue("ip returned is not in the avoid list", !avoid.contains(ip)); avoid.add(ip); + ip = NetUtils.getRandomIpFromCidr(cidr, 30, avoid); assertEquals("This should be -1 because we ran out of ip addresses: " + ip, ip, -1); }
