> On July 2, 2014, 12:50 p.m., Hugo Trippaers wrote: > > engine/schema/src/com/cloud/dc/dao/VlanDaoImpl.java, line 310 > > <https://reviews.apache.org/r/23226/diff/1/?file=622487#file622487line310> > > > > Shouldn't this be inside the try-with-resources bit of the try keyword? > > Santhosh Edukulla wrote: > Here we are taking care of closing the resource with > "prepareAutoCloseStatement", compared to normal call of "prepareStatement" i > believe. We used try with resource where prepare through auto close was not > followed. Going ahead, we can change it to try with resource, to make it > uniform, but here other leak issue is fixed i believe.
I think we should avoid using the prepareAutoCloseStatement in favor of the try-with-resources. The prepareAutoCloseStatement doesn't really close the statement unless it is called a second time if i understand the code correctly. > On July 2, 2014, 12:50 p.m., Hugo Trippaers wrote: > > engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java, > > line 71 > > <https://reviews.apache.org/r/23226/diff/1/?file=622500#file622500line71> > > > > To make it more clear for the readers of the code why not write 'if > > (params == null) { throw blabla }' > > > > > > Santhosh Edukulla wrote: > We can, As such possibility of null referencing is fixed, going ahead, i > can add a log and throw accordingly, but on second note, i assume that the > calling function based upon the callable output can log and throw accordingly > there, rather we take care in the called function. Let me know your thoughts. This function is apparently called by plugins directly, so we should be prepared to handle params being null in this function. So your solution is correct, it's just coding style.. Instead of if (x != null) { do_lots_of_stuff(); } throw exception, it's better to write if (x==null) { throw exception} do_lots_of_stuff(); - Hugo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23226/#review47181 ----------------------------------------------------------- On July 2, 2014, 8:44 a.m., Santhosh Edukulla wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23226/ > ----------------------------------------------------------- > > (Updated July 2, 2014, 8:44 a.m.) > > > Review request for cloudstack, Abhinandan Prateek and daan Hoogland. > > > Bugs: coverity > https://issues.apache.org/jira/browse/coverity > > > Repository: cloudstack-git > > > Description > ------- > > Fixed resource leaks, null dererence issues, performance issues and bugs > reported by coverity. Simple bug in generic dao was found during these > changes, fixed that as well. > > > Diffs > ----- > > engine/schema/src/com/cloud/dc/dao/VlanDaoImpl.java a38a96e > engine/schema/src/com/cloud/storage/dao/StoragePoolWorkDaoImpl.java c6e1b74 > engine/schema/src/com/cloud/upgrade/DatabaseCreator.java b04607d > engine/schema/src/com/cloud/usage/dao/UsageIPAddressDaoImpl.java 17d5dc8 > engine/schema/src/com/cloud/usage/dao/UsageLoadBalancerPolicyDaoImpl.java > c868ac0 > engine/schema/src/com/cloud/usage/dao/UsageNetworkOfferingDaoImpl.java > f27fd60 > engine/schema/src/com/cloud/usage/dao/UsagePortForwardingRuleDaoImpl.java > 803288a > engine/schema/src/com/cloud/usage/dao/UsageSecurityGroupDaoImpl.java > 0fc2ce0 > engine/schema/src/com/cloud/usage/dao/UsageStorageDaoImpl.java 77fc56f > engine/schema/src/com/cloud/usage/dao/UsageVPNUserDaoImpl.java 64d3ecd > engine/schema/src/com/cloud/usage/dao/UsageVolumeDaoImpl.java 7bf058c > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java > 92793f1 > > engine/storage/cache/src/org/apache/cloudstack/storage/cache/allocator/StorageCacheRandomAllocator.java > b4ef595 > > engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java > 6b12975 > > framework/cluster/src/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java > 3d0c3f5 > framework/db/src/com/cloud/utils/db/DbUtil.java 792573c > framework/db/src/com/cloud/utils/db/GenericDaoBase.java 2052aad > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java > e684b8d > > plugins/storage/volume/sample/src/org/apache/cloudstack/storage/datastore/lifecycle/SamplePrimaryDataStoreLifeCycleImpl.java > 579cc24 > server/src/com/cloud/storage/VolumeApiServiceImpl.java a557c0e > server/src/com/cloud/tags/TaggedResourceManagerImpl.java b77c55e > server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java e085b8d > > Diff: https://reviews.apache.org/r/23226/diff/ > > > Testing > ------- > > Built the management server, ran the simulator and deployed dc. > > > Thanks, > > Santhosh Edukulla > >
