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: git commit: updated refs/heads/master to 6dd30ea
Date Tue, 02 Dec 2014 16:35:42 GMT
On Tue, Dec 2, 2014 at 4:50 PM, Rohit Yadav <rohityadav89@gmail.com> wrote:
> Hi Daan/Wilder,
>
> Quick question about fixing the following coverity issue (in-line);
>
> On Tue, Dec 2, 2014 at 8:45 PM, <dahn@apache.org> 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 <dhoogland@schubergphilis.com>
>> Authored: Tue Dec 2 16:14:34 2014 +0100
>> Committer: Daan Hoogland <daan@onecht.net>
>> 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

Mime
View raw message