cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "daan Hoogland" <daan.hoogl...@gmail.com>
Subject Re: Review Request 22356: Fixed few coverity issues reported
Date Wed, 11 Jun 2014 11:36:08 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?
> 
> Santhosh Edukulla wrote:
>     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.

I certainly don't agree. A transaction that does an unconditional commit in a finally makes
no sense. If the problem is that our Transaction doesn't implement Closable we should make
it implement that interface. It already implements close() so this is no big change. At the
end of regular execution the commit should be done, not in the finally. The exception handlers
should do a rollback().

I realize that this is not entirely new in your code but it is a big bug and as you do touch
the contents of the finally clause, please fix. (A little fix I think)


- daan


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


On June 11, 2014, 6:43 a.m., Santhosh Edukulla wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22356/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 6: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