cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Santhosh Edukulla" <santhosh.eduku...@citrix.com>
Subject Re: Review Request 22356: Fixed few coverity issues reported
Date Wed, 11 Jun 2014 06:42:43 GMT


> On June 10, 2014, 12:20 p.m., daan Hoogland wrote:
> > engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java, line 136
> > <https://reviews.apache.org/r/22356/diff/2/?file=606364#file606364line136>
> >
> >     a commit in a finally when try-with-resource used seems not what you want. What
do you expect to be committed here?

Mentioned transaction logic mentioned seems to be our own implementation. I have seen changing
these commits\close at places leads to issues. Though connection is getting auto closed, "assuming"
a transaction can have multiple connections and especially shared with scope across process,
transaction commit  seems ok. So, earlier i didn't delved in too much details, transaction
start and commit seems reasonable,  went to keep it like that.  

There are few places where this piece of code was used. With this fix, I touched to see where
leaks are there, and fixed. Went conservative at places, cautious not to introduce regression
issues. If we agree, i will still keep it like this. Changing these at all places, may be
like like additional refactoring, making sure our own implementation of transaction works\breaks
and require thorough testing. Let me know your thoughts.


> On June 10, 2014, 12:20 p.m., daan Hoogland wrote:
> > engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java, line 250
> > <https://reviews.apache.org/r/22356/diff/2/?file=606364#file606364line250>
> >
> >     a commit in a finally means you might partialy commit. That is not what you
want.

As mentioned above.


> On June 10, 2014, 12:20 p.m., daan Hoogland wrote:
> > server/src/com/cloud/test/IPRangeConfig.java, line 377
> > <https://reviews.apache.org/r/22356/diff/2/?file=606370#file606370line377>
> >
> >     Is Connection not Closable?


> On June 10, 2014, 12:20 p.m., daan Hoogland wrote:
> > framework/db/src/com/cloud/utils/db/TransactionLegacy.java, line 794
> > <https://reviews.apache.org/r/22356/diff/2/?file=606369#file606369line794>
> >
> >     maybe you want to put type.equals(item.type) and some extra check to prevent
NPEs?
> >     and use a similar construct for item.ref?

Added this check for item.ref as well.


> On June 10, 2014, 12:20 p.m., daan Hoogland wrote:
> > framework/db/src/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java, line 269
> > <https://reviews.apache.org/r/22356/diff/2/?file=606366#file606366line269>
> >
> >     some better naming for the second statement is a welcome luxury;)

yeah, it seems little misnomer, changed :)


- Santhosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22356/#review45218
-----------------------------------------------------------


On June 10, 2014, 4:43 a.m., Santhosh Edukulla wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22356/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 4:43 a.m.)
> 
> 
> Review request for cloudstack and daan Hoogland.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Fixed few coverity issues reported for resource leak, value comparison, invalid loop
check for result set.
> 
> 
> Diffs
> -----
> 
>   engine/schema/src/com/cloud/upgrade/DatabaseCreator.java 91ef318 
>   engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java c20a418 
>   engine/schema/src/com/cloud/upgrade/DatabaseUpgradeChecker.java 0761c9f 
>   framework/db/src/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java 58584f9 
>   framework/db/src/com/cloud/utils/db/Merovingian2.java 6eeea9f 
>   framework/db/src/com/cloud/utils/db/ScriptRunner.java 6614527 
>   framework/db/src/com/cloud/utils/db/TransactionLegacy.java ac0ea21 
>   server/src/com/cloud/test/IPRangeConfig.java 1d56471 
>   usage/src/com/cloud/usage/UsageSanityChecker.java 5e6123b 
>   utils/src/com/cloud/utils/crypt/EncryptionSecretKeySender.java 086e8a8 
> 
> Diff: https://reviews.apache.org/r/22356/diff/
> 
> 
> Testing
> -------
> 
> 1.Built the code and found no issues.
> 2.Built the simulator and ran a deploy datacenter with the changes.
> 
> 
> Thanks,
> 
> Santhosh Edukulla
> 
>


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