On Tue, Dec 2, 2014 at 4:50 PM, Rohit Yadav <[email protected]> wrote:
> Hi Daan/Wilder,
>
> Quick question about fixing the following coverity issue (in-line);
>
> On Tue, Dec 2, 2014 at 8:45 PM, <[email protected]> wrote:
>
>> Repository: cloudstack
>> Updated Branches:
>> refs/heads/master 818957de0 -> 6dd30eaf1
>>
>>
>> CID-1256273/CID-1256274/CID-1256275 leaky resources plus switch
>> statement warning
>>
>> reviewed by Wilder Rodrigues
>>
>> Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/6dd30eaf
>> Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/6dd30eaf
>> Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/6dd30eaf
>>
>> Branch: refs/heads/master
>> Commit: 6dd30eaf14f323cd84252647c490e84982b0a853
>> Parents: 818957d
>> Author: Daan Hoogland <[email protected]>
>> Authored: Tue Dec 2 16:14:34 2014 +0100
>> Committer: Daan Hoogland <[email protected]>
>> Committed: Tue Dec 2 16:14:34 2014 +0100
>>
>> ----------------------------------------------------------------------
>> .../com/cloud/upgrade/dao/Upgrade442to450.java | 28 ++++++--------------
>> 1 file changed, 8 insertions(+), 20 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/cloudstack/blob/6dd30eaf/engine/schema/src/com/cloud/upgrade/dao/Upgrade442to450.java
>> ----------------------------------------------------------------------
>> diff --git a/engine/schema/src/com/cloud/upgrade/dao/Upgrade442to450.java
>> b/engine/schema/src/com/cloud/upgrade/dao/Upgrade442to450.java
>> index aeb44a1..9fe1319 100644
>> --- a/engine/schema/src/com/cloud/upgrade/dao/Upgrade442to450.java
>> +++ b/engine/schema/src/com/cloud/upgrade/dao/Upgrade442to450.java
>> @@ -115,9 +115,6 @@ public class Upgrade442to450 implements DbUpgrade {
>> }
>>
>> private void upgradeMemoryOfInternalLoadBalancervmOffering(Connection
>> conn) {
>> - PreparedStatement updatePstmt = null;
>> - PreparedStatement selectPstmt = null;
>> - ResultSet selectResultSet = null;
>> int newRamSize = 256; //256MB
>> long serviceOfferingId = 0;
>>
>> @@ -126,10 +123,9 @@ public class Upgrade442to450 implements DbUpgrade {
>> * We should not update/modify any user-defined offering.
>> */
>>
>> - try {
>> - selectPstmt = conn.prepareStatement("SELECT id FROM
>> `cloud`.`service_offering` WHERE vm_type='internalloadbalancervm'");
>> - updatePstmt = conn.prepareStatement("UPDATE
>> `cloud`.`service_offering` SET ram_size=? WHERE id=?");
>> - selectResultSet = selectPstmt.executeQuery();
>> + try (PreparedStatement selectPstmt =
>> conn.prepareStatement("SELECT id FROM `cloud`.`service_offering` WHERE
>> vm_type='internalloadbalancervm'");
>> + PreparedStatement updatePstmt =
>> conn.prepareStatement("UPDATE `cloud`.`service_offering` SET ram_size=?
>> WHERE id=?");
>> + ResultSet selectResultSet = selectPstmt.executeQuery()){
>> if(selectResultSet.next()) {
>> serviceOfferingId = selectResultSet.getLong("id");
>> }
>> @@ -139,19 +135,6 @@ public class Upgrade442to450 implements DbUpgrade {
>> updatePstmt.executeUpdate();
>> } catch (SQLException e) {
>> throw new CloudRuntimeException("Unable to upgrade ram_size
>> of service offering for internal loadbalancer vm. ", e);
>> - } finally {
>> - try {
>> - if (selectPstmt != null) {
>> - selectPstmt.close();
>> - }
>> - if (selectResultSet != null) {
>> - selectResultSet.close();
>> - }
>> - if (updatePstmt != null) {
>> - updatePstmt.close();
>> - }
>> - } catch (SQLException e) {
>> - }
>>
>
> Why are we removing closing statements and resultsets here?
because of the try-with-resource (automatic closing of resources)
>
>
>> }
>> s_logger.debug("Done upgrading RAM for service offering of
>> internal loadbalancer vm to " + newRamSize);
>> }
>> @@ -188,6 +171,8 @@ public class Upgrade442to450 implements DbUpgrade {
>> break;
>> case LXC:
>> hypervisorsListInUse.add(Hypervisor.HypervisorType.LXC);
>> break;
>> + default: // no action on cases Any, BareMetal,
>> None, Ovm, Parralels, Simulator and VirtualBox:
>> + break;
>> }
>> }
>> } catch (SQLException e) {
>> @@ -258,6 +243,8 @@ public class Upgrade442to450 implements DbUpgrade {
>> pstmt.executeUpdate();
>> pstmt.close();
>> } else {
>> + rs.close();
>> + pstmt.close();
>> if
>> (hypervisorsListInUse.contains(hypervisorAndTemplateName.getKey())){
>> throw new CloudRuntimeException("4.5.0 " +
>> hypervisorAndTemplateName.getKey() + " SystemVm template not found. Cannot
>> upgrade system Vms");
>> } else {
>> @@ -286,6 +273,7 @@ public class Upgrade442to450 implements DbUpgrade {
>> pstmt.close();
>>
>
> Why are we keeping statements and resultsets her?
the refactoring of all those usages of the statement would require
major refactorring for this method. I choose not to do this for this
one. could be doen as well.
>
> Regards.
--
Daan
