cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Darren Shepherd" <dar...@godaddy.com>
Subject Re: Review Request: Fix bug: unittest NetUtilsTest failed sometimes
Date Sun, 12 Aug 2012 19:12:25 GMT

-----------------------------------------------------------
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
> 
>


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