Issue #248: Adding JSON lines, also bug fixes and more tests to preg.

Issue #248: Adding JSON lines, also bug fixes and more tests to preg.

[Joachim Metz, 2015-07-03 04:55:54]

https://codereview.appspot.com/255740043/diff/1/plaso/output/__init__.py
File plaso/output/__init__.py (right):

https://codereview.appspot.com/255740043/diff/1/plaso/output/__init__.py#newc...
plaso/output/__init__.py:5: from plaso.output import json_line
alpha order

https://codereview.appspot.com/255740043/diff/1/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode70
tools/preg.py:70: single_file: boolean that indicates whether the tool is run
against a
I opt to use dfvfs source type here instead.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode361
tools/preg.py:361: self, event_objects, file_entry, verbose=False):
verbose what? verbose_output ? why not propagate show_hex since that is what it
is used for ?

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode372
tools/preg.py:372: verbose: optional boolean that determines whether verbose
output should
the boolean does not determine anything, it is just a variable ;)

optional boolean that indicated whether verbose output should be enabled. The
default is False.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode403
tools/preg.py:403: event_object, file_entry=file_entry, show_hex=verbose)
show_hex=show_hex

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode997
tools/preg.py:997: if len(items) < 2:
It is unclear what you are doing here, either move to sub functions to improve
what the code is supposed to be doing.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode1489
tools/preg.py:1489: # No Registry type included, scan for all available types.
There is no scanning happening here:

No Registry type specified use all available types instead. (or equiv)

[kiddi, 2015-07-04 17:27:17]

done

https://codereview.appspot.com/255740043/diff/1/plaso/output/__init__.py
File plaso/output/__init__.py (right):

https://codereview.appspot.com/255740043/diff/1/plaso/output/__init__.py#newc...
plaso/output/__init__.py:5: from plaso.output import json_line
On 2015/07/03 04:55:53, Joachim Metz wrote:
> alpha order

Done.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode70
tools/preg.py:70: single_file: boolean that indicates whether the tool is run
against a
On 2015/07/03 04:55:54, Joachim Metz wrote:
> I opt to use dfvfs source type here instead.

Done.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode361
tools/preg.py:361: self, event_objects, file_entry, verbose=False):
On 2015/07/03 04:55:54, Joachim Metz wrote:
> verbose what? verbose_output ? why not propagate show_hex since that is what
it
> is used for ?

ok

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode372
tools/preg.py:372: verbose: optional boolean that determines whether verbose
output should
On 2015/07/03 04:55:54, Joachim Metz wrote:
> the boolean does not determine anything, it is just a variable ;)
> 
> optional boolean that indicated whether verbose output should be enabled. The
> default is False.

Done.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode403
tools/preg.py:403: event_object, file_entry=file_entry, show_hex=verbose)
On 2015/07/03 04:55:54, Joachim Metz wrote:
> show_hex=show_hex

Done.

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode997
tools/preg.py:997: if len(items) < 2:
On 2015/07/03 04:55:54, Joachim Metz wrote:
> It is unclear what you are doing here, either move to sub functions to improve
> what the code is supposed to be doing.

changed this code, it was quite messy

https://codereview.appspot.com/255740043/diff/1/tools/preg.py#newcode1489
tools/preg.py:1489: # No Registry type included, scan for all available types.
On 2015/07/03 04:55:54, Joachim Metz wrote:
> There is no scanning happening here:
> 
> No Registry type specified use all available types instead. (or equiv)

Acknowledged.

[kiddi, 2015-07-05 00:37:50]

forgot to update the code

[kiddi, 2015-07-05 00:39:19]

Code updated.

[Joachim Metz, 2015-07-05 05:18:29]

https://codereview.appspot.com/255740043/diff/20001/plaso/output/json_line.py
File plaso/output/json_line.py (right):

https://codereview.appspot.com/255740043/diff/20001/plaso/output/json_line.py...
plaso/output/json_line.py:2: """An output module that saves data into a JSON
line format."""
Explain here what "JSON line" is and why it differs from "JSON out".
Maybe also what it is used for.

https://codereview.appspot.com/255740043/diff/20001/tests/frontend/preg.py
File tests/frontend/preg.py (right):

https://codereview.appspot.com/255740043/diff/20001/tests/frontend/preg.py#ne...
tests/frontend/preg.py:48: scan_node = scan_context.GetRootScanNode()
+ comment about getting the most upper node.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode69
tools/preg.py:69: source_type_indicator: dfVFS source type indicator for the
source file.
I opt "source_type" instead of "source_type_indicator"

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode411
tools/preg.py:411: show_hex: optional boolean, if set to True hexadecimal
representation of
What is "value" here? I opt to rewrite similar to:

optional boolean to indicate that the hexadecimal representation of the event
object data is included in the output. The ...

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode539
tools/preg.py:539: show_hex: optional boolean, if set to True hexadecimal
representation of
See previous comment.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1024
tools/preg.py:1024: self.console.LoadRegistryFile(registry_file_index)
Isn't this something that needs to get caught in LoadRegistryFile and have it
return None?

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1347
tools/preg.py:1347: if not registry_helper:
Don't you want to raise ValueError here?

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1450
tools/preg.py:1450: registry_helper_path = self._registry_helpers.keys()[index]
Split this across 2 statements and check the result of
self._registry_helpers.keys()

[kiddi, 2015-07-05 17:19:34]

done

https://codereview.appspot.com/255740043/diff/20001/plaso/output/json_line.py
File plaso/output/json_line.py (right):

https://codereview.appspot.com/255740043/diff/20001/plaso/output/json_line.py...
plaso/output/json_line.py:2: """An output module that saves data into a JSON
line format."""
On 2015/07/05 05:18:28, Joachim Metz wrote:
> Explain here what "JSON line" is and why it differs from "JSON out".
> Maybe also what it is used for.

Done.

https://codereview.appspot.com/255740043/diff/20001/tests/frontend/preg.py
File tests/frontend/preg.py (right):

https://codereview.appspot.com/255740043/diff/20001/tests/frontend/preg.py#ne...
tests/frontend/preg.py:48: scan_node = scan_context.GetRootScanNode()
On 2015/07/05 05:18:28, Joachim Metz wrote:
> + comment about getting the most upper node.

Done.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode69
tools/preg.py:69: source_type_indicator: dfVFS source type indicator for the
source file.
On 2015/07/05 05:18:28, Joachim Metz wrote:
> I opt "source_type" instead of "source_type_indicator"

Acknowledged.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode411
tools/preg.py:411: show_hex: optional boolean, if set to True hexadecimal
representation of
On 2015/07/05 05:18:28, Joachim Metz wrote:
> What is "value" here? I opt to rewrite similar to:
> 
> optional boolean to indicate that the hexadecimal representation of the event
> object data is included in the output. The ...

Done.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode539
tools/preg.py:539: show_hex: optional boolean, if set to True hexadecimal
representation of
On 2015/07/05 05:18:28, Joachim Metz wrote:
> See previous comment.

Done.

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1024
tools/preg.py:1024: self.console.LoadRegistryFile(registry_file_index)
On 2015/07/05 05:18:28, Joachim Metz wrote:
> Isn't this something that needs to get caught in LoadRegistryFile and have it
> return None?

we could do that as well... But LoadRegistryFile doesn't return any value, as in
, it already returns None back.... 

so it would rather be that instead of raising IndexError we raise a custom
one...

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1347
tools/preg.py:1347: if not registry_helper:
On 2015/07/05 05:18:28, Joachim Metz wrote:
> Don't you want to raise ValueError here?

sure

https://codereview.appspot.com/255740043/diff/20001/tools/preg.py#newcode1450
tools/preg.py:1450: registry_helper_path = self._registry_helpers.keys()[index]
On 2015/07/05 05:18:28, Joachim Metz wrote:
> Split this across 2 statements and check the result of
> self._registry_helpers.keys()

Done.

[kiddi, 2015-07-05 17:29:24]

Code updated.

[Joachim Metz, 2015-07-06 19:53:42]

Afraid we're not there yet.

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode331
tools/preg.py:331: show_hex: optional boolean to indicate that the hexadecimal
representation
partial sentence

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode411
tools/preg.py:411: show_hex: optional boolean to indicate that the hexadecimal
representation
partial sentence

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode539
tools/preg.py:539: show_hex: optional boolean to indicate that the hexadecimal
representation
partial sentence

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1448
tools/preg.py:1448: 
Why not return a boolean instead of raising and do the logging in this method?

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1454
tools/preg.py:1454: if index < 0 or index >= len(self._registry_helpers):
Shouldn't this be done later what if len(self._registry_helpers) !=
len(helper_keys) ?

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1532
tools/preg.py:1532: if (source_type != definitions.TYPE_INDICATOR_OS and
definitions.TYPE_INDICATOR_OS is not a source type so this check is incorrect.

Is this a bug somewhere else as well?

[kiddi, 2015-07-07 15:34:01]

done

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode331
tools/preg.py:331: show_hex: optional boolean to indicate that the hexadecimal
representation
On 2015/07/06 19:53:42, Joachim Metz wrote:
> partial sentence

Done.

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode411
tools/preg.py:411: show_hex: optional boolean to indicate that the hexadecimal
representation
On 2015/07/06 19:53:42, Joachim Metz wrote:
> partial sentence

Done.

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode539
tools/preg.py:539: show_hex: optional boolean to indicate that the hexadecimal
representation
On 2015/07/06 19:53:42, Joachim Metz wrote:
> partial sentence

Done.

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1448
tools/preg.py:1448: 
On 2015/07/06 19:53:42, Joachim Metz wrote:
> Why not return a boolean instead of raising and do the logging in this method?

you really think that is better? I'm not convinced

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1454
tools/preg.py:1454: if index < 0 or index >= len(self._registry_helpers):
On 2015/07/06 19:53:42, Joachim Metz wrote:
> Shouldn't this be done later what if len(self._registry_helpers) !=
> len(helper_keys) ?

that shouldn't really happen... in what case can that happen?
len(self._registry_helpers) is really the len of keys(), but I switched it a
bit....

https://codereview.appspot.com/255740043/diff/40001/tools/preg.py#newcode1532
tools/preg.py:1532: if (source_type != definitions.TYPE_INDICATOR_OS and
On 2015/07/06 19:53:42, Joachim Metz wrote:
> definitions.TYPE_INDICATOR_OS is not a source type so this check is incorrect.
> 
> Is this a bug somewhere else as well?

this is cause I wasn't using the scanning context source type, rather relying on
the type indicator from the source path spec. Changed it.... don't think this is
an issue elsewhere

[kiddi, 2015-07-07 15:35:34]

Code updated.

[Joachim Metz, 2015-07-07 18:32:30]

Strange code

https://codereview.appspot.com/255740043/diff/60001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/60001/tools/preg.py#newcode1537
tools/preg.py:1537: elif self.self.preg_tool.registry_file:
self.self. => self.

I also don't understand the logic of the 2 checks resulting both in:
registry_types = [self.preg_tool.registry_file]

[kiddi, 2015-07-08 04:00:28]

sorry, done in a haste this morning, without actually checking it again

https://codereview.appspot.com/255740043/diff/60001/tools/preg.py
File tools/preg.py (right):

https://codereview.appspot.com/255740043/diff/60001/tools/preg.py#newcode1537
tools/preg.py:1537: elif self.self.preg_tool.registry_file:
On 2015/07/07 18:32:30, Joachim Metz wrote:
> self.self. => self.
> 
> I also don't understand the logic of the 2 checks resulting both in:
> registry_types = [self.preg_tool.registry_file]

sorry... this was done in a haste, wasn't supposed to be like this

[kiddi, 2015-07-08 04:01:41]

Code updated.

[Joachim Metz, 2015-07-08 05:21:36]

LGTM, one style nit remaining

https://codereview.appspot.com/255740043/diff/80001/plaso/lib/errors.py
File plaso/lib/errors.py (right):

https://codereview.appspot.com/255740043/diff/80001/plaso/lib/errors.py#newco...
plaso/lib/errors.py:50: class UnableToLoadRegistryHelper(Error):
Alpha ordering

[kiddi, 2015-07-09 05:26:16]

done

https://codereview.appspot.com/255740043/diff/80001/plaso/lib/errors.py
File plaso/lib/errors.py (right):

https://codereview.appspot.com/255740043/diff/80001/plaso/lib/errors.py#newco...
plaso/lib/errors.py:50: class UnableToLoadRegistryHelper(Error):
On 2015/07/08 05:21:36, Joachim Metz wrote:
> Alpha ordering

Done.

[kiddi, 2015-07-09 05:28:25]

Code Submitted.