cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hugo Trippaers" <htrippa...@schubergphilis.com>
Subject Re: Review Request 23226: Fixed resource leaks, null dererence issues, performance issues and bugs reported by coverity
Date Thu, 03 Jul 2014 11:37:26 GMT


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


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