From c72c1ef55df58359f9175af26e1be9d36f24895d Mon Sep 17 00:00:00 2001
From: Ann Priestman <apriestman@basistech.com>
Date: Mon, 25 Nov 2019 13:59:05 -0500
Subject: [PATCH] Code review changes

---
 .../sleuthkit/datamodel/AbstractContent.java  | 15 +-------
 .../org/sleuthkit/datamodel/FileSystem.java   | 36 +++++++++----------
 .../src/org/sleuthkit/datamodel/Pool.java     |  1 +
 .../sleuthkit/datamodel/SleuthkitCase.java    | 20 ++++-------
 .../org/sleuthkit/datamodel/SleuthkitJNI.java |  3 +-
 tsk/auto/auto_db.cpp                          |  4 +--
 tsk/auto/db_postgresql.cpp                    |  2 +-
 tsk/auto/db_sqlite.cpp                        | 19 ++++++++--
 tsk/auto/tsk_db.h                             |  2 +-
 tsk/auto/tsk_db_postgresql.h                  |  2 +-
 tsk/auto/tsk_db_sqlite.h                      |  2 +-
 11 files changed, 52 insertions(+), 54 deletions(-)

diff --git a/bindings/java/src/org/sleuthkit/datamodel/AbstractContent.java b/bindings/java/src/org/sleuthkit/datamodel/AbstractContent.java
index 63e43f05e..caf177386 100644
--- a/bindings/java/src/org/sleuthkit/datamodel/AbstractContent.java
+++ b/bindings/java/src/org/sleuthkit/datamodel/AbstractContent.java
@@ -202,20 +202,7 @@ public Content getDataSource() throws TskCoreException {
 	 * @throws TskCoreException 
 	 */
 	boolean isPoolContent() throws TskCoreException {
-		Content myParent = getParent();
-		if (myParent == null) {
-			return false;
-		}
-		
-		if (! (myParent instanceof AbstractContent)) {
-			return false;
-		}
-
-		if (myParent instanceof Pool) {
-			return true;
-		}
-		
-		return ((AbstractContent)myParent).isPoolContent();
+		return getPool() != null;
 	}
 	
 	/**
diff --git a/bindings/java/src/org/sleuthkit/datamodel/FileSystem.java b/bindings/java/src/org/sleuthkit/datamodel/FileSystem.java
index f3a82485b..d20f8b7df 100644
--- a/bindings/java/src/org/sleuthkit/datamodel/FileSystem.java
+++ b/bindings/java/src/org/sleuthkit/datamodel/FileSystem.java
@@ -91,26 +91,26 @@ long getFileSystemHandle() throws TskCoreException {
 			synchronized (this) {
 				if (filesystemHandle == 0) {
 					Content dataSource = getDataSource();
-					if ((dataSource != null) && (dataSource instanceof Image)) {
-						Image image = (Image) dataSource;
-						
-						// Check if this file system is in a pool
-						if (isPoolContent()) {
-							Pool pool = getPool();
-							if (pool == null) {
-								throw new TskCoreException("Error finding pool for file system");
-							}
-							
-							Volume poolVolume = getPoolVolume();
-							if (poolVolume == null) {
-								throw new TskCoreException("File system is in a pool but has no volume");
-							}
-							filesystemHandle = SleuthkitJNI.openFsPool(image.getImageHandle(), imgOffset, pool.getPoolHandle(), poolVolume.getStart(), getSleuthkitCase());
-						} else {
-							filesystemHandle = SleuthkitJNI.openFs(image.getImageHandle(), imgOffset, getSleuthkitCase());
+					if ((dataSource == null) || ( !(dataSource instanceof Image))) {
+						throw new TskCoreException("Data Source of File System is not an image");
+					}
+
+					Image image = (Image) dataSource;
+
+					// Check if this file system is in a pool
+					if (isPoolContent()) {
+						Pool pool = getPool();
+						if (pool == null) {
+							throw new TskCoreException("Error finding pool for file system");
 						}
+
+						Volume poolVolume = getPoolVolume();
+						if (poolVolume == null) {
+							throw new TskCoreException("File system is in a pool but has no volume");
+						}
+						filesystemHandle = SleuthkitJNI.openFsPool(image.getImageHandle(), imgOffset, pool.getPoolHandle(), poolVolume.getStart(), getSleuthkitCase());
 					} else {
-						throw new TskCoreException("Data Source of File System is not an image");
+						filesystemHandle = SleuthkitJNI.openFs(image.getImageHandle(), imgOffset, getSleuthkitCase());
 					}
 				}
 			}
diff --git a/bindings/java/src/org/sleuthkit/datamodel/Pool.java b/bindings/java/src/org/sleuthkit/datamodel/Pool.java
index 7271044cf..1e247aedc 100644
--- a/bindings/java/src/org/sleuthkit/datamodel/Pool.java
+++ b/bindings/java/src/org/sleuthkit/datamodel/Pool.java
@@ -88,6 +88,7 @@ public long getOffset() {
 	 *                          occurs
 	 */
 	long getPoolHandle() throws TskCoreException {
+		// Note that once poolHandle is set, it will never be changed or reset to zero
 		if (poolHandle == 0) {
 			synchronized (this) {
 				if (poolHandle == 0) {
diff --git a/bindings/java/src/org/sleuthkit/datamodel/SleuthkitCase.java b/bindings/java/src/org/sleuthkit/datamodel/SleuthkitCase.java
index a07e01c64..b2690820e 100644
--- a/bindings/java/src/org/sleuthkit/datamodel/SleuthkitCase.java
+++ b/bindings/java/src/org/sleuthkit/datamodel/SleuthkitCase.java
@@ -7322,9 +7322,9 @@ Pool getPoolById(long id, long parentId) throws TskCoreException {
 	}
 	
 	/**
-	 * Get file system by id and Content parent
+	 * Get pool by id and Content parent
 	 *
-	 * @param id     of the filesystem to get
+	 * @param id     of the pool to get
 	 * @param parent a direct parent Content object
 	 *
 	 * @return populated FileSystem object
@@ -7334,28 +7334,22 @@ Pool getPoolById(long id, long parentId) throws TskCoreException {
 	 */
 	private Pool getPoolByIdHelper(long id, Content parent) throws TskCoreException {
 
-		CaseDbConnection connection = connections.getConnection();
 		acquireSingleUserCaseReadLock();
-		Statement s = null;
-		ResultSet rs = null;
-		try {
-			s = connection.createStatement();
-			rs = connection.executeQuery(s, "SELECT * FROM tsk_pool_info " //NON-NLS
-					+ "where obj_id = " + id); //NON-NLS
+		try (CaseDbConnection connection = connections.getConnection();
+			 Statement s = connection.createStatement();
+			 ResultSet rs = connection.executeQuery(s, "SELECT * FROM tsk_pool_info " //NON-NLS
+					+ "where obj_id = " + id);) { //NON-NLS
 			if (rs.next()) {
 				Pool pool = new Pool(this, rs.getLong("obj_id"), TskData.TSK_POOL_TYPE_ENUM.valueOf(rs.getLong("pool_type")).getName(), rs.getLong("pool_type"), rs.getLong("img_offset"));
 				pool.setParent(parent);
 				
 				return pool;
 			} else {
-				throw new TskCoreException("No pool found for id:" + id);
+				throw new TskCoreException("No pool found for ID:" + id);
 			}
 		} catch (SQLException ex) {
 			throw new TskCoreException("Error getting Pool by ID", ex);
 		} finally {
-			closeResultSet(rs);
-			closeStatement(s);
-			connection.close();
 			releaseSingleUserCaseReadLock();
 		}
 	}
diff --git a/bindings/java/src/org/sleuthkit/datamodel/SleuthkitJNI.java b/bindings/java/src/org/sleuthkit/datamodel/SleuthkitJNI.java
index 3a45f8274..a38f11a6f 100644
--- a/bindings/java/src/org/sleuthkit/datamodel/SleuthkitJNI.java
+++ b/bindings/java/src/org/sleuthkit/datamodel/SleuthkitJNI.java
@@ -918,7 +918,8 @@ public static long openFs(long imgHandle, long fsOffset, SleuthkitCase skCase) t
 	}
 	
 	/**
-	 * Get file system Handle Opened handle is cached (transparently) so it does
+	 * Get file system handle for a file system contained in a pool.
+	 * Opened handle is cached (transparently) so it does
 	 * not need be reopened next time for the duration of the application
 	 * 
 	 * @param imgHandle pointer to imgHandle in sleuthkit
diff --git a/tsk/auto/auto_db.cpp b/tsk/auto/auto_db.cpp
index bea833035..43b84169e 100755
--- a/tsk/auto/auto_db.cpp
+++ b/tsk/auto/auto_db.cpp
@@ -311,14 +311,14 @@ TskAutoDb::filterPool(const TSK_POOL_INFO * pool_info)
 
     if (m_volFound && m_vsFound) {
         // there's a volume system and volume
-        if (m_db->addPoolInfo(pool_info, m_curVolId, m_curPoolVs)) {
+        if (m_db->addPoolInfoAndVS(pool_info, m_curVolId, m_curPoolVs)) {
             registerError();
             return TSK_FILTER_STOP;
         }
     }
     else {
         // pool doesn't live in a volume, use image as parent
-        if (m_db->addPoolInfo(pool_info, m_curImgId, m_curPoolVs)) {
+        if (m_db->addPoolInfoAndVS(pool_info, m_curImgId, m_curPoolVs)) {
             registerError();
             return TSK_FILTER_STOP;
         }
diff --git a/tsk/auto/db_postgresql.cpp b/tsk/auto/db_postgresql.cpp
index 866feaace..761ce01f6 100755
--- a/tsk/auto/db_postgresql.cpp
+++ b/tsk/auto/db_postgresql.cpp
@@ -1009,7 +1009,7 @@ int TskDbPostgreSQL::addImageName(int64_t objId, char const *imgName, int sequen
 }
 
 int
-TskDbPostgreSQL::addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) {
+TskDbPostgreSQL::addPoolInfoAndVS(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) {
     return TSK_ERR; // TODO TODO
 }
 
diff --git a/tsk/auto/db_sqlite.cpp b/tsk/auto/db_sqlite.cpp
index 7dace4672..54cda8879 100644
--- a/tsk/auto/db_sqlite.cpp
+++ b/tsk/auto/db_sqlite.cpp
@@ -753,13 +753,23 @@ TskDbSqlite::addVsInfo(const TSK_VS_INFO* vs_info, int64_t parObjId,
                         "Error adding data to tsk_vs_info table: %s\n");
 }
 
+/**
+* Creats a new tsk_pool_info database entry and a new tsk_vs_info 
+* entry with the tsk_pool_info as its parent.
+*
+* @ param pool_info The pool to save to the database
+* @ param parObjId The ID of the parent of the pool object
+* @ param objId Will be set to the object ID of the new volume system created as a child of 
+*               the new pool.
+* @returns 1 on error, 0 on success
+*/
 int
-TskDbSqlite::addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) {
+TskDbSqlite::addPoolInfoAndVS(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) {
 
     char
         stmt[1024];
 
-    // Temp - Make pool and then VS
+    // Add pool
     int64_t poolObjId;
     if (addObject(TSK_DB_OBJECT_TYPE_POOL, parObjId, poolObjId))
         return 1;
@@ -774,6 +784,7 @@ TskDbSqlite::addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64
         return retVal;
     }
 
+    // Add volume system
     if (addObject(TSK_DB_OBJECT_TYPE_VS, poolObjId, objId))
         return 1;
 
@@ -786,6 +797,10 @@ TskDbSqlite::addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64
 
 /**
 * Adds the sector addresses of the pool volumes into the db.
+
+* @param pool_vol The pool volume to save to the DB
+* @param parObjId The ID of the parent of the pool volume (should be a volume system)
+* @param objId Will be set to the object ID of the new volume
 * @returns 1 on error, 0 on success
 */
 int
diff --git a/tsk/auto/tsk_db.h b/tsk/auto/tsk_db.h
index a3afc7e5d..3fefed1b9 100755
--- a/tsk/auto/tsk_db.h
+++ b/tsk/auto/tsk_db.h
@@ -177,7 +177,7 @@ class TskDb {
     virtual int addImageName(int64_t objId, char const *imgName, int sequence) = 0;
     virtual int addVsInfo(const TSK_VS_INFO * vs_info, int64_t parObjId, int64_t & objId) = 0;
     virtual int addVolumeInfo(const TSK_VS_PART_INFO * vs_part, int64_t parObjId, int64_t & objId) = 0;
-    virtual int addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) = 0;
+    virtual int addPoolInfoAndVS(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId) = 0;
     virtual int addPoolVolumeInfo(const TSK_POOL_VOLUME_INFO* pool_vol,
         int64_t parObjId, int64_t& objId) = 0;
     virtual int addFsInfo(const TSK_FS_INFO * fs_info, int64_t parObjId, int64_t & objId) = 0;
diff --git a/tsk/auto/tsk_db_postgresql.h b/tsk/auto/tsk_db_postgresql.h
index 90c070ef0..873b142ba 100755
--- a/tsk/auto/tsk_db_postgresql.h
+++ b/tsk/auto/tsk_db_postgresql.h
@@ -58,7 +58,7 @@ class TskDbPostgreSQL : public TskDb {
         int64_t & objId);
     int addFsInfo(const TSK_FS_INFO * fs_info, int64_t parObjId,
         int64_t & objId);
-    int addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId);
+    int addPoolInfoAndVS(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId);
     int addPoolVolumeInfo(const TSK_POOL_VOLUME_INFO* pool_vol,
         int64_t parObjId, int64_t& objId);
     int addFsFile(TSK_FS_FILE * fs_file, const TSK_FS_ATTR * fs_attr,
diff --git a/tsk/auto/tsk_db_sqlite.h b/tsk/auto/tsk_db_sqlite.h
index b07f79fa2..7fa3abae9 100755
--- a/tsk/auto/tsk_db_sqlite.h
+++ b/tsk/auto/tsk_db_sqlite.h
@@ -50,7 +50,7 @@ class TskDbSqlite : public TskDb {
     int addImageName(int64_t objId, char const *imgName, int sequence);
     int addVsInfo(const TSK_VS_INFO * vs_info, int64_t parObjId,
         int64_t & objId);
-    int addPoolInfo(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId);
+    int addPoolInfoAndVS(const TSK_POOL_INFO *pool_info, int64_t parObjId, int64_t& objId);
     int addPoolVolumeInfo(const TSK_POOL_VOLUME_INFO* pool_vol,
         int64_t parObjId, int64_t& objId);
     int addVolumeInfo(const TSK_VS_PART_INFO * vs_part, int64_t parObjId,
-- 
GitLab