QPID-3676: Correct issues found by findbugs marked as blockers

Applied patch from Andrew MacBean <andymacbean@gmail.com>

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1213636 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 122 insertions, 121 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/database/PlainPasswordFilePrincipalDatabase.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/database/PlainPasswordFilePrincipalDatabase.java
index 76ebea0321..89cce31ea6 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/database/PlainPasswordFilePrincipalDatabase.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/database/PlainPasswordFilePrincipalDatabase.java
@@ -375,16 +375,8 @@ public class PlainPasswordFilePrincipalDatabase implements PrincipalDatabase
 
             BufferedReader reader = null;
             PrintStream writer = null;
-            
-            Random r = new Random();
-            File tmp;
-            do
-            {
-                tmp = new File(_passwordFile.getPath() + r.nextInt() + ".tmp");
-            }
-            while(tmp.exists());
-            
-            tmp.deleteOnExit();
+
+            final File tmp = createTempFileOnSameFilesystem(_passwordFile);
 
             try
             {
@@ -449,52 +441,72 @@ public class PlainPasswordFilePrincipalDatabase implements PrincipalDatabase
             }
             finally
             {
-                if (reader != null)
-                {
-                    reader.close();
-                }
-
                 if (writer != null)
                 {
                     writer.close();
                 }
-            }
-            
-            // Swap temp file to main password file.
-            File old = new File(_passwordFile.getAbsoluteFile() + ".old");
-            if (old.exists())
-            {
-                old.delete();
-            }
-            
-            if(!_passwordFile.renameTo(old))
-            {
-                //unable to rename the existing file to the backup name 
-                _logger.error("Could not backup the existing password file");
-                throw new IOException("Could not backup the existing password file");
-            }
-
-            if(!tmp.renameTo(_passwordFile))
-            {
-                //failed to rename the new file to the required filename
-                
-                if(!old.renameTo(_passwordFile))
+                if (reader != null)
                 {
-                    //unable to return the backup to required filename
-                    _logger.error("Could not rename the new password file into place, and unable to restore original file");
-                    throw new IOException("Could not rename the new password file into place, and unable to restore original file");
+                    reader.close();
                 }
-                
-                _logger.error("Could not rename the new password file into place");
-                throw new IOException("Could not rename the new password file into place");
             }
-            
+
+            swapTempFileToLive(_passwordFile, tmp);
+
         }
         finally
         {
             _userUpdate.unlock();
         }
     }
+
+    private void swapTempFileToLive(final File live, final File temp) throws IOException
+    {
+        // Remove any existing ".old" file
+        final File old = new File(live.getAbsoluteFile() + ".old");
+        if (old.exists())
+        {
+            old.delete();
+        }
+
+        // Create an new ".old" file
+        if(!live.renameTo(old))
+        {
+            //unable to rename the existing file to the backup name
+            _logger.error("Could not backup the existing password file");
+            throw new IOException("Could not backup the existing password file");
+        }
+
+        // Move temp file to be the new "live" file
+        if(!temp.renameTo(live))
+        {
+            //failed to rename the new file to the required filename
+            if(!old.renameTo(live))
+            {
+                //unable to return the backup to required filename
+                _logger.error("Could not rename the new password file into place, and unable to restore original file");
+                throw new IOException("Could not rename the new password file into place, and unable to restore original file");
+            }
+
+            _logger.error("Could not rename the new password file into place");
+            throw new IOException("Could not rename the new password file into place");
+        }
+    }
+
+    private File createTempFileOnSameFilesystem(final File liveFile)
+    {
+        File tmp;
+        final Random r = new Random();
+
+        do
+        {
+            tmp = new File(liveFile.getPath() + r.nextInt() + ".tmp");
+        }
+        while(tmp.exists());
+
+        tmp.deleteOnExit();
+        return tmp;
+    }
     
     public void reload() throws IOException
     {
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/store/DerbyMessageStore.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/store/DerbyMessageStore.java
index 8b099b62ce..d3f46d2e90 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/store/DerbyMessageStore.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/store/DerbyMessageStore.java
@@ -142,6 +142,19 @@ public class DerbyMessageStore implements MessageStore
     private boolean _configured;
 
 
+    private static final class CommitStoreFuture implements StoreFuture
+    {
+        public boolean isComplete()
+        {
+            return true;
+        }
+
+        public void waitForCompletion()
+        {
+
+        }
+    }
+
     private enum State
     {
         INITIAL,
@@ -910,19 +923,19 @@ public class DerbyMessageStore implements MessageStore
             throws AMQStoreException
     {
         Connection conn = null;
+        PreparedStatement stmt = null;
 
         try
         {
             conn = newAutoCommitConnection();
             // exchange_name varchar(255) not null, queue_name varchar(255) not null, binding_key varchar(255), arguments blob
-            PreparedStatement stmt = conn.prepareStatement(DELETE_FROM_BINDINGS);
+            stmt = conn.prepareStatement(DELETE_FROM_BINDINGS);
             stmt.setString(1, exchange.getNameShortString().toString() );
             stmt.setString(2, queue.getNameShortString().toString());
             stmt.setString(3, routingKey == null ? null : routingKey.toString());
-            
+
             int result = stmt.executeUpdate();
-            stmt.close();
-            
+
             if(result != 1)
             {
                  throw new AMQStoreException("Queue binding for queue with name " + queue.getNameShortString() + " to exchange "
@@ -936,21 +949,9 @@ public class DerbyMessageStore implements MessageStore
         }
         finally
         {
-            if(conn != null)
-            {
-               try
-               {
-                   conn.close();
-               }
-               catch (SQLException e)
-               {
-                   _logger.error(e);
-               }
-            }
-
+            closePreparedStatement(stmt);
+            closeConnection(conn);
         }
-
-
     }
 
     public void createQueue(AMQQueue queue) throws AMQStoreException
@@ -1153,15 +1154,14 @@ public class DerbyMessageStore implements MessageStore
         AMQShortString name = queue.getNameShortString();
         _logger.debug("public void removeQueue(AMQShortString name = " + name + "): called");
         Connection conn = null;
-
+        PreparedStatement stmt = null;
         try
         {
             conn = newAutoCommitConnection();
-            PreparedStatement stmt = conn.prepareStatement(DELETE_FROM_QUEUE);
+            stmt = conn.prepareStatement(DELETE_FROM_QUEUE);
             stmt.setString(1, name.toString());
             int results = stmt.executeUpdate();
-            stmt.close();
-            
+
             if (results == 0)
             {
                 throw new AMQStoreException("Queue " + name + " not found");
@@ -1173,18 +1173,8 @@ public class DerbyMessageStore implements MessageStore
         }
         finally
         {
-            if(conn != null)
-            {
-               try
-               {
-                   conn.close();
-               }
-               catch (SQLException e)
-               {
-                   _logger.error(e);
-               }
-            }
-
+            closePreparedStatement(stmt);
+            closeConnection(conn);
         }
 
 
@@ -1317,19 +1307,7 @@ public class DerbyMessageStore implements MessageStore
     public StoreFuture commitTranAsync(ConnectionWrapper connWrapper) throws AMQStoreException
     {
         commitTran(connWrapper);
-        return new StoreFuture()
-                    {
-                        public boolean isComplete()
-                        {
-                            return true;
-                        }
-
-                        public void waitForCompletion()
-                        {
-
-                        }
-                    };
-
+        return new CommitStoreFuture();
     }
 
     public void abortTran(ConnectionWrapper connWrapper) throws AMQStoreException
@@ -1572,6 +1550,7 @@ public class DerbyMessageStore implements MessageStore
         {
             _logger.debug("Adding content chunk offset " + offset + " for message " +messageId);
         }
+        PreparedStatement stmt = null;
 
         try
         {
@@ -1580,7 +1559,7 @@ public class DerbyMessageStore implements MessageStore
             byte[] chunkData = new byte[src.limit()];
             src.duplicate().get(chunkData);
 
-            PreparedStatement stmt = conn.prepareStatement(INSERT_INTO_MESSAGE_CONTENT);
+            stmt = conn.prepareStatement(INSERT_INTO_MESSAGE_CONTENT);
             stmt.setLong(1,messageId);
             stmt.setInt(2, offset);
             stmt.setInt(3, offset+chunkData.length);
@@ -1594,24 +1573,16 @@ public class DerbyMessageStore implements MessageStore
             ByteArrayInputStream bis = new ByteArrayInputStream(chunkData);
             stmt.setBinaryStream(4, bis, chunkData.length);
             stmt.executeUpdate();
-            stmt.close();
         }
         catch (SQLException e)
         {
-            if(conn != null)
-            {
-                try
-                {
-                    conn.close();
-                }
-                catch (SQLException e1)
-                {
-
-                }
-            }
-
+            closeConnection(conn);
             throw new RuntimeException("Error adding content chunk offset " + offset + " for message " + messageId + ": " + e.getMessage(), e);
         }
+        finally
+        {
+            closePreparedStatement(stmt);
+        }
 
     }
 
@@ -1619,13 +1590,13 @@ public class DerbyMessageStore implements MessageStore
     public int getContent(long messageId, int offset, ByteBuffer dst)
     {
         Connection conn = null;
-
+        PreparedStatement stmt = null;
 
         try
         {
             conn = newAutoCommitConnection();
 
-            PreparedStatement stmt = conn.prepareStatement(SELECT_FROM_MESSAGE_CONTENT);
+            stmt = conn.prepareStatement(SELECT_FROM_MESSAGE_CONTENT);
             stmt.setLong(1,messageId);
             stmt.setInt(2, offset);
             stmt.setInt(3, offset+dst.remaining());
@@ -1656,28 +1627,18 @@ public class DerbyMessageStore implements MessageStore
                 }
             }
 
-            stmt.close();
-            conn.close();
             return written;
 
         }
         catch (SQLException e)
         {
-            if(conn != null)
-            {
-                try
-                {
-                    conn.close();
-                }
-                catch (SQLException e1)
-                {
-
-                }
-            }
-
             throw new RuntimeException("Error retrieving content from offset " + offset + " for message " + messageId + ": " + e.getMessage(), e);
         }
-
+        finally
+        {
+            closePreparedStatement(stmt);
+            closeConnection(conn);
+        }
 
 
     }
@@ -1849,5 +1810,33 @@ public class DerbyMessageStore implements MessageStore
         }
     }
 
+    private void closeConnection(final Connection conn)
+    {
+        if(conn != null)
+        {
+           try
+           {
+               conn.close();
+           }
+           catch (SQLException e)
+           {
+               _logger.error("Problem closing connection", e);
+           }
+        }
+    }
 
+    private void closePreparedStatement(final PreparedStatement stmt)
+    {
+        if (stmt != null)
+        {
+            try
+            {
+                stmt.close();
+            }
+            catch(SQLException e)
+            {
+                _logger.error("Problem closing prepared statement", e);
+            }
+        }
+    }
 }