----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6558/#review10181 -----------------------------------------------------------
I think this code change might actually inadvertantly introduce a small bug. Checking the avoids.size() and returning -1 is only correct if the contents of the avoids are items that are in the range your are checking. For example, if one was to put -256L to -1L in the avoids set the check would always return -1 for any /24 even though those longs aren't valid IP's. Obviously nobody would do that, but it got me to thinking. The range you check avoids .0, .1, and .255 for a /24 (correct me if I'm wrong. I just read the code, haven't really ran it so I could have understood it wrong). This implies that nobody should include the 0,1, or 255 in their avoids list. While 0 and 255 will probably never happen, there's a good chance .1 might be passed in, and in fact I think the Guest network guru might. So it seems the up front avoid.size() check might be an unneeded optimization. If you remove that check that will of course break the other change you did towards the end of the method. - Darren Shepherd On Aug. 12, 2012, 4:09 p.m., mice xia wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6558/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2012, 4:09 p.m.) > > > Review request for cloudstack and Alex Huang. > > > Description > ------- > > unittest NetUtilsTest failed sometimes because NetUtils.getRandomIpFromCidr's > return is non-deterministic > > fix NetUtils.getRandomIpFromCidr by rewriting it: > 1) remove network addr, broadcast addr, and startIp from Ip 'range' > 2) solve non-deterministic issue by ensuring return a result if avoid.size() > < range > > > Diffs > ----- > > server/src/com/cloud/network/guru/GuestNetworkGuru.java cd5f551 > utils/src/com/cloud/utils/net/NetUtils.java 886f441 > utils/test/com/cloud/utils/net/NetUtilsTest.java 345f1d9 > > Diff: https://reviews.apache.org/r/6558/diff/ > > > Testing > ------- > > pass unittest > > > Thanks, > > mice xia > >
