Darren, You are right, there are still something needs to be updated in my patch. However as I read the code carefully, I found this method 'getRandomIpFromCidr' is actually deprecated. Besides unittest, the only caller is guetNetworkGuru, but nobody uses that caller method at all.
Alex, Is it Ok for you to remove this unittest and related caller methods? Regards Mice From: Darren Shepherd [mailto:nore...@reviews.apache.org] On Behalf Of Darren Shepherd Sent: Monday, August 13, 2012 3:12 AM To: Alex Huang Cc: cloudstack; Mice Xia; Darren Shepherd Subject: Re: Review Request: Fix bug: unittest NetUtilsTest failed sometimes This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6558/ 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 On August 12th, 2012, 4:09 p.m., mice xia wrote: Review request for cloudstack and Alex Huang. By mice xia. Updated Aug. 12, 2012, 4:09 p.m. 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 Testing pass unittest 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) View Diff
