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 12:18:07 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.
> 
> daan Hoogland wrote:
>     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)
> 
> Santhosh Edukulla wrote:
>     Ok,  in general this is what this behavior should be. Anyways,below are what we wanted.
>     
>     1. So, move txn commit to end of try block and retain txn close in finally? 
>     2. Regarding rollback, do we need rollback in outer catch? Again, rollback can lead
to exception, so try\catch again? once it is in outer catch, the required resources to rollback
are anyways autoclosed or not available for rolling back anything? so, rollback is not required
in catch?yes\no?

ad 1. agree
ad 2. rollback yes, but rollback should be automatic when no commit is done, if rollback fails
nothing else can be done. Our Transaction class must implement Closable so the close call
is implicit and the opening can go in a try-with-resource clause


- 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