[plaso] Added experimenal SQLite storage file

[plaso] Added experimenal SQLite storage file

[onager, 2017-03-29 12:52:03]

https://codereview.appspot.com/319360043/diff/1/plaso/multi_processing/task_e...
File plaso/multi_processing/task_engine.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/multi_processing/task_e...
plaso/multi_processing/task_engine.py:438: task_name = u'{0:s} task
queue'.format(process_name)
rename to queue_name =

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py
File plaso/storage/identifiers.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:40: """Class that defines the SQL table attribute
container identifier.
-Class that

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:42: The identifier is used to uniquely identify
attribute containers.
+that are stored in sqlite storage? Or something more to explain why this class
exists.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:54: row_identifier (int): unique identifier fo the
row in the table.
for

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:666: """Class that implements a file-based storage
writer."""
-Class that implements +Defines an interface for

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:700: path (str): path of the task storage file that
should be merged.
-of +to

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:737: analysis_report: an analysis report object
(instance of AnalysisReport).
Please fix docstring

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:887: """Retrieves the events in increasing
chronological order.
Please specify which events are being retrieved here - events that have been
written to the file? Events pending write to the file? Events that have been
written on the file in this session?

"The events" is ambiguous.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1076: abort (bool): True to indicated the stop is
issued on abort.
indicate. We've had problems before in defining what "abort" is supposed to
mean. Please add extra explanation of what the abort is supposed to imply. Maybe
+"If true, task result data will be lost" or something similar.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py
File plaso/storage/sqlite_file.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:29: u'CREATE TABLE {0:s} ('
Please don't do this, use prepared statements instead. From the sqlite3
documentation: "You shouldn’t assemble your query using Python’s string
operations".

We might trust the data for now, but this is a bad practice.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:64: query = u'SELECT _identifier, _data FROM
{0:s}'.format(container_type)
See above re: prepared statements.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:92: """Writes an attribute container.
+note that the table for the container type must exist.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:225: Raises:
This doesn't raise, remove.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:312: path (Optional[str]): path of the storage
file.
-of +to

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:329: return False
Why doesn't this raise? Can connect() really return None?

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:357: pass
+TODO...

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:370: pass
+TODO...

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:473: connection = sqlite3.connect(
You aren't checking for connection is None here, but you are in the regular
reader. One is wrong.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:478: for container_type in self._CONTAINER_TYPES:
What if not all the container types tables are in the storage?

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:493: # pylint: disable=protected-access
Please add a todo to fix this hack.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py
File plaso/storage/zip_file.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py#new...
plaso/storage/zip_file.py:2880: path (str): path of the task storage file that
should be merged.
-of +to

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py#new...
plaso/storage/zip_file.py:2891: path (str): path of the storage file.
-of +to

[Joachim Metz, 2017-04-08 18:11:31]

https://codereview.appspot.com/319360043/diff/1/plaso/multi_processing/task_e...
File plaso/multi_processing/task_engine.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/multi_processing/task_e...
plaso/multi_processing/task_engine.py:438: task_name = u'{0:s} task
queue'.format(process_name)
On 2017/03/29 12:52:02, onager wrote:
> rename to queue_name = 

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py
File plaso/storage/identifiers.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:40: """Class that defines the SQL table attribute
container identifier.
On 2017/03/29 12:52:02, onager wrote:
> -Class that

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:42: The identifier is used to uniquely identify
attribute containers.
On 2017/03/29 12:52:02, onager wrote:
> +that are stored in sqlite storage? Or something more to explain why this
class
> exists.

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/identifiers.py#...
plaso/storage/identifiers.py:54: row_identifier (int): unique identifier fo the
row in the table.
On 2017/03/29 12:52:02, onager wrote:
> for

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:666: """Class that implements a file-based storage
writer."""
On 2017/03/29 12:52:02, onager wrote:
> -Class that implements +Defines an interface for

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:700: path (str): path of the task storage file that
should be merged.
On 2017/03/29 12:52:02, onager wrote:
> -of +to

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:737: analysis_report: an analysis report object
(instance of AnalysisReport).
On 2017/03/29 12:52:02, onager wrote:
> Please fix docstring

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:887: """Retrieves the events in increasing
chronological order.
On 2017/03/29 12:52:02, onager wrote:
> Please specify which events are being retrieved here - events that have been
> written to the file? Events pending write to the file? Events that have been
> written on the file in this session?
> 
> "The events" is ambiguous.

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1076: abort (bool): True to indicated the stop is
issued on abort.
On 2017/03/29 12:52:02, onager wrote:
> indicate. We've had problems before in defining what "abort" is supposed to
> mean. Please add extra explanation of what the abort is supposed to imply.
Maybe
> +"If true, task result data will be lost" or something similar.

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py
File plaso/storage/sqlite_file.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:29: u'CREATE TABLE {0:s} ('
The code fully controls {0:s} and is not based on external input, so injection
of uncontrolled input is unlikely.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:64: query = u'SELECT _identifier, _data FROM
{0:s}'.format(container_type)
The code fully controls {0:s} and is not based on external input, so injection
of uncontrolled input is unlikely.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:92: """Writes an attribute container.
On 2017/03/29 12:52:02, onager wrote:
> +note that the table for the container type must exist.

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:225: Raises:
On 2017/03/29 12:52:03, onager wrote:
> This doesn't raise, remove.

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:312: path (Optional[str]): path of the storage
file.
On 2017/03/29 12:52:02, onager wrote:
> -of +to

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:329: return False
Documenation is not clear about this:
https://docs.python.org/2/library/sqlite3.html#sqlite3.connect

but I'll remove the check.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:357: pass
On 2017/03/29 12:52:02, onager wrote:
> +TODO...

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:370: pass
On 2017/03/29 12:52:02, onager wrote:
> +TODO...

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:473: connection = sqlite3.connect(
Documenation is not clear about this:
https://docs.python.org/2/library/sqlite3.html#sqlite3.connect

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:478: for container_type in self._CONTAINER_TYPES:
The code at line 335 should ensure the tables exist, but I've added a check if
the table exists to gracefully handle issues.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/sqlite_file.py#...
plaso/storage/sqlite_file.py:493: # pylint: disable=protected-access
On 2017/03/29 12:52:02, onager wrote:
> Please add a todo to fix this hack.

Acknowledged.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py
File plaso/storage/zip_file.py (right):

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py#new...
plaso/storage/zip_file.py:2880: path (str): path of the task storage file that
should be merged.
On 2017/03/29 12:52:03, onager wrote:
> -of +to

Done.

https://codereview.appspot.com/319360043/diff/1/plaso/storage/zip_file.py#new...
plaso/storage/zip_file.py:2891: path (str): path of the storage file.
On 2017/03/29 12:52:03, onager wrote:
> -of +to

Done.

[Joachim Metz, 2017-04-08 18:14:01]

Code updated.

[onager, 2017-05-19 09:00:46]

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/identifiers.py
File plaso/storage/identifiers.py (right):

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/identifiers...
plaso/storage/identifiers.py:42: """The SQL table attribute container
identifier.
-The

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file.py
File plaso/storage/sqlite_file.py (right):

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:408: """Class that implements a SQLite-based
storage file reader for merging."""
-Class that

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:492: # if (maximum_number_of_containers > 0 and
Please fix - this looks pretty easy to implement with a break

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:513: """Class that implements the SQLite-based
storage file writer."""
-Class that

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:519: BaseFileStorage: storage file.
SQLiteStorageFile

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:530: StorageMergeReader: storage merge reader.
SQLiteStorageMergeReader

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:542: StorageWriter: storage writer.
SQLiteStorageFileWriter

[Joachim Metz, 2017-05-19 11:13:43]

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/identifiers.py
File plaso/storage/identifiers.py (right):

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/identifiers...
plaso/storage/identifiers.py:42: """The SQL table attribute container
identifier.
On 2017/05/19 09:00:45, onager wrote:
> -The

Done.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file.py
File plaso/storage/sqlite_file.py (right):

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:408: """Class that implements a SQLite-based
storage file reader for merging."""
On 2017/05/19 09:00:45, onager wrote:
> -Class that

Done.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:492: # if (maximum_number_of_containers > 0 and
I opt to do this in a follow up CL.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:513: """Class that implements the SQLite-based
storage file writer."""
On 2017/05/19 09:00:45, onager wrote:
> -Class that

Done.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:519: BaseFileStorage: storage file.
On 2017/05/19 09:00:46, onager wrote:
> SQLiteStorageFile

Done.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:530: StorageMergeReader: storage merge reader.
On 2017/05/19 09:00:45, onager wrote:
> SQLiteStorageMergeReader

Done.

https://codereview.appspot.com/319360043/diff/20001/plaso/storage/sqlite_file...
plaso/storage/sqlite_file.py:542: StorageWriter: storage writer.
On 2017/05/19 09:00:46, onager wrote:
> SQLiteStorageFileWriter

Done.

[Joachim Metz, 2017-05-20 05:26:03]

Code updated.

[onager, 2017-05-25 18:41:58]

LGTM

[Joachim Metz, 2017-05-26 04:15:55]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: python ./utils/review.py close storage1