> On Feb. 14, 2014, 8:54 a.m., Hugo Trippaers wrote: > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ServiceInstanceModel.java, > > line 81 > > <https://reviews.apache.org/r/18066/diff/1/?file=484221#file484221line81> > > > > Why should this be set to a random UUID at this point?
In the original code, if the projectId is null, it will simply throw NPE on the last line (nr. 90) of the method where the projectId.getUuid() is called. This was added as a way to avoid NPE. Should we perhaps let the NPE happen in that case? > On Feb. 14, 2014, 8:54 a.m., Hugo Trippaers wrote: > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualMachineModel.java, > > line 93 > > <https://reviews.apache.org/r/18066/diff/1/?file=484222#file484222line93> > > > > Why don't you use the UUID.fromString to validate the UUID? UUID.fromString() does not validate an UUID properly. I tried, for example, informing less digits in the UUID, where 12 were expected, and the UUID.fromstring() did not thrown the exception as expected. However, if you try UUID.fromString("aaa") it breaks, but if you try UUID.fromString("3dd4fa6e-2899-4429-b818-d34fe8df5") it doesn't (the last portion should have 12, instead of 9 digits). In other fix I added the validate UUID method to the UuidUtil classes. > On Feb. 14, 2014, 8:54 a.m., Hugo Trippaers wrote: > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualMachineModel.java, > > line 141 > > <https://reviews.apache.org/r/18066/diff/1/?file=484222#file484222line141> > > > > In the original code this would not be executed, why execute it now? Agree. That's dead code. I will comment it out and add comments about the fact that the code has never been executed. > On Feb. 14, 2014, 8:54 a.m., Hugo Trippaers wrote: > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelObject.java, > > line 48 > > <https://reviews.apache.org/r/18066/diff/1/?file=484220#file484220line48> > > > > Why is this made transient? WeakReference class is not serializable by definition. So, we cannot enforce its serialization unless we write the implementation of methods writeObject() and readObject(). Since the code was already not serializing it, it's been marked as transient. - Wilder ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18066/#review34463 ----------------------------------------------------------- On Feb. 13, 2014, 11:32 a.m., Wilder Rodrigues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18066/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2014, 11:32 a.m.) > > > Review request for cloudstack, daan Hoogland and Hugo Trippaers. > > > Repository: cloudstack-git > > > Description > ------- > > Fixing troubling issues on contrail plugin related to dereference nullpoint; > adding unit tests to cover changes on the compare method on the > VirtualNetworkModel > > > Diffs > ----- > > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManagerImpl.java > b25de48 > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelObject.java > 1b048ed > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ServiceInstanceModel.java > 0ce22ad > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualMachineModel.java > 4d0218c > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualNetworkModel.java > b0505b1 > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/model/VirtualMachineModelTest.java > f85beb6 > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/model/VirtualNetworkModelTest.java > b1b5ae1 > > Diff: https://reviews.apache.org/r/18066/diff/ > > > Testing > ------- > > Build successful + 3 unit tests added in order to cover compare methods cases. > > > Thanks, > > Wilder Rodrigues > >
