[plaso] Replace binplist with biplist #1480

[plaso] Replace binplist with biplist #1480

[Joachim Metz, 2018-03-22 06:06:39]

overall looks good, couple of smaller things, I think there are 3 changes in
here we should break out:
1. https://codereview.appspot.com/334610043/
2. log filter ?
3. biplist changes

[Joachim Metz, 2018-03-22 06:06:46]

https://codereview.appspot.com/338590043/diff/1/.travis.yml
File .travis.yml (right):

https://codereview.appspot.com/338590043/diff/1/.travis.yml#newcode1
.travis.yml:1: language: python
Per chat I opt to stage these changes first.

https://codereview.appspot.com/334610043/

https://codereview.appspot.com/338590043/diff/1/dependencies.ini
File dependencies.ini (right):

https://codereview.appspot.com/338590043/diff/1/dependencies.ini#newcode16
dependencies.ini:16: rpm_name: python-binlist
doens't biplist define a version property or function?

https://codereview.appspot.com/338590043/diff/1/plaso/cli/log2timeline_tool.py
File plaso/cli/log2timeline_tool.py (left):

https://codereview.appspot.com/338590043/diff/1/plaso/cli/log2timeline_tool.p...
plaso/cli/log2timeline_tool.py:21: from plaso.cli import logging_filter
why these changes? are they as part of the biplist changes or could we apply
them seperately?

https://codereview.appspot.com/338590043/diff/1/plaso/lib/plist.py
File plaso/lib/plist.py (right):

https://codereview.appspot.com/338590043/diff/1/plaso/lib/plist.py#newcode69
plaso/lib/plist.py:69: AttributeError, binascii.Error,
biplist.InvalidPlistException,
please double check which of these are raised in biplist and which ones remain
from binplist.

[onager, 2018-03-24 21:37:50]

Code updated.

[onager, 2018-03-25 08:32:31]

https://codereview.appspot.com/338590043/diff/1/.travis.yml
File .travis.yml (right):

https://codereview.appspot.com/338590043/diff/1/.travis.yml#newcode1
.travis.yml:1: language: python
On 2018/03/22 06:06:45, Joachim Metz wrote:
> Per chat I opt to stage these changes first.
> 
> https://codereview.appspot.com/334610043/

Acknowledged.

https://codereview.appspot.com/338590043/diff/1/dependencies.ini
File dependencies.ini (right):

https://codereview.appspot.com/338590043/diff/1/dependencies.ini#newcode16
dependencies.ini:16: rpm_name: python-binlist
On 2018/03/22 06:06:46, Joachim Metz wrote:
> doens't biplist define a version property or function?

No, it doesn't.

https://codereview.appspot.com/338590043/diff/1/plaso/cli/log2timeline_tool.py
File plaso/cli/log2timeline_tool.py (left):

https://codereview.appspot.com/338590043/diff/1/plaso/cli/log2timeline_tool.p...
plaso/cli/log2timeline_tool.py:21: from plaso.cli import logging_filter
On 2018/03/22 06:06:46, Joachim Metz wrote:
> why these changes? are they as part of the biplist changes or could we apply
> them seperately?

They could be done separately, but they only support binplist, so all this code
is dead.

https://codereview.appspot.com/338590043/diff/1/plaso/lib/plist.py
File plaso/lib/plist.py (right):

https://codereview.appspot.com/338590043/diff/1/plaso/lib/plist.py#newcode69
plaso/lib/plist.py:69: AttributeError, binascii.Error,
biplist.InvalidPlistException,
On 2018/03/22 06:06:46, Joachim Metz wrote:
> please double check which of these are raised in biplist and which ones remain
> from binplist.

Done.

[Joachim Metz, 2018-03-29 11:12:24]

now let's add biplist to the l2tdevtools plaso preset, so we can build a version
for l2tbinaries

[Joachim Metz, 2018-03-31 11:42:57]

Please update this CL biplist should now be l2tbinaries and GIFT PPA

[onager, 2018-05-04 11:28:50]

Code updated.

[Joachim Metz, 2018-05-05 07:20:02]

Please add the truncated test binary plist file I've mailed you

https://codereview.appspot.com/338590043/diff/40001/.travis.yml
File .travis.yml (left):

https://codereview.appspot.com/338590043/diff/40001/.travis.yml#oldcode42
.travis.yml:42: - env: [TARGET="linux-python34-tox", TOXENV="py34"]
please keep this as is; changes pending for l2tdevtools

https://codereview.appspot.com/345900043/

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py
File plaso/parsers/plist.py (right):

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:57: if not isinstance(exception, py2to3.BYTES_TYPE):
this block of code can likely be changed to:

raise errors.UnableToParseFile(
    'Unable to parse plist with error: {0!s}'.format(exception))

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:68: 'Unable to parse XML file, reason:
{0:s}'.format(exception))
is this except block still relevant?

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:70: except OverflowError as exception:
is this except block still relevant?

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist_plugi...
File plaso/parsers/plist_plugins/macuser.py (right):

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist_plugi...
plaso/parsers/plist_plugins/macuser.py:136: top_level = plist_file
I opt to remove top_level in favor of plist_file or do this in a follow up CL.

[onager, 2018-05-16 09:33:21]

Code updated.

[onager, 2018-05-16 09:34:08]

https://codereview.appspot.com/338590043/diff/40001/.travis.yml
File .travis.yml (left):

https://codereview.appspot.com/338590043/diff/40001/.travis.yml#oldcode42
.travis.yml:42: - env: [TARGET="linux-python34-tox", TOXENV="py34"]
On 2018/05/05 07:20:02, Joachim Metz wrote:
> please keep this as is; changes pending for l2tdevtools
> 
> https://codereview.appspot.com/345900043/

Done.

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py
File plaso/parsers/plist.py (right):

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:57: if not isinstance(exception, py2to3.BYTES_TYPE):
On 2018/05/05 07:20:02, Joachim Metz wrote:
> this block of code can likely be changed to:
> 
> raise errors.UnableToParseFile(
>     'Unable to parse plist with error: {0!s}'.format(exception))

Done.

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:68: 'Unable to parse XML file, reason:
{0:s}'.format(exception))
On 2018/05/05 07:20:02, Joachim Metz wrote:
> is this except block still relevant? 

Done.

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist.py#ne...
plaso/parsers/plist.py:70: except OverflowError as exception:
On 2018/05/05 07:20:02, Joachim Metz wrote:
> is this except block still relevant? 

Done.

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist_plugi...
File plaso/parsers/plist_plugins/macuser.py (right):

https://codereview.appspot.com/338590043/diff/40001/plaso/parsers/plist_plugi...
plaso/parsers/plist_plugins/macuser.py:136: top_level = plist_file
On 2018/05/05 07:20:02, Joachim Metz wrote:
> I opt to remove top_level in favor of plist_file or do this in a follow up CL.

Done.

[Joachim Metz, 2018-05-18 05:09:52]

LGTM, with some remaining requests, questions, remarks

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py
File plaso/lib/plist.py (right):

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py#newcode36
plaso/lib/plist.py:36: if isinstance(key, (dict, plistlib._InternalDict)):
Does biplist generate objects of type plistlib._InternalDict? Otherwise we could
likely remove this code?

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py#newcode59
plaso/lib/plist.py:59: file_object: the file-like object.
This appears to be an old docstring style, could you please update as well,
seeing you're touching this file. Thx

https://codereview.appspot.com/338590043/diff/60001/plaso/parsers/plist_plugi...
File plaso/parsers/plist_plugins/macuser.py (right):

https://codereview.appspot.com/338590043/diff/60001/plaso/parsers/plist_plugi...
plaso/parsers/plist_plugins/macuser.py:137: plist_file = dict()
should we generate a processing error instead of falling back to empty dict?

I also opt to use {} instead of dict() which is supposedly slightly faster:
https://stackoverflow.com/questions/5790860/and-vs-list-and-dict-which-is-better

[onager, 2018-05-18 11:29:52]

Code updated.

[onager, 2018-05-18 12:03:52]

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

[onager, 2018-05-18 12:29:57]

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py
File plaso/lib/plist.py (right):

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py#newcode36
plaso/lib/plist.py:36: if isinstance(key, (dict, plistlib._InternalDict)):
On 2018/05/18 05:09:52, Joachim Metz wrote:
> Does biplist generate objects of type plistlib._InternalDict? Otherwise we
could
> likely remove this code?

Done.

https://codereview.appspot.com/338590043/diff/60001/plaso/lib/plist.py#newcode59
plaso/lib/plist.py:59: file_object: the file-like object.
On 2018/05/18 05:09:52, Joachim Metz wrote:
> This appears to be an old docstring style, could you please update as well,
> seeing you're touching this file. Thx

Done.

https://codereview.appspot.com/338590043/diff/60001/plaso/parsers/plist_plugi...
File plaso/parsers/plist_plugins/macuser.py (right):

https://codereview.appspot.com/338590043/diff/60001/plaso/parsers/plist_plugi...
plaso/parsers/plist_plugins/macuser.py:137: plist_file = dict()
On 2018/05/18 05:09:52, Joachim Metz wrote:
> should we generate a processing error instead of falling back to empty dict?
> 
> I also opt to use {} instead of dict() which is supposedly slightly faster:
>
https://stackoverflow.com/questions/5790860/and-vs-list-and-dict-which-is-better

We could, many of the MacOS parsers are quite old, and could do with general
clean and refactoring. Filed https://github.com/log2timeline/plaso/issues/1888
for that.