[plaso] Fixes for nsrlsvr #1035, #1036 added early abort #1046 and lookup hash argument

[Joachim Metz, 2016-10-15 09:56:51]

Code updated.

[Joachim Metz, 2016-10-15 11:25:12]

Code updated.

[onager, 2016-10-16 09:09:57]

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/interface.py
File plaso/analysis/interface.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/interface....
plaso/analysis/interface.py:431: """Sets the hash to query the nsrlsvr.
Please fix docstring, this isn't nsrlvr

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py
File plaso/analysis/nsrlsvr.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:41: """Connects to the nsrlsvr.
-the or +instance

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:114: def SetHostname(self, host):
-name, this also works for an ip address, which is not a hostname

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:132: 
+ "The nsrlsvr instance will be queried for a specific MD5 hash that should
exist."

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:134: bool: True if the nsrlsvr is reachable.
-the or +instance

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:140: nsrl_socket, u'd41d8cd98f00b204e9800998ecf8427e')
extract to class constant, add docstring making it clear this is md5 of the
empty string.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:178: def SetHostname(self, host):
See above for Host vs Hostname

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/viper.py
File plaso/analysis/viper.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/viper.py#n...
plaso/analysis/viper.py:23: hash_analysis_queue (Queue.queue): that the analyzer
will append
-that

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
File plaso/cli/helpers/nsrlsvr_analysis.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
plaso/cli/helpers/nsrlsvr_analysis.py:36: u'Type of hash to query the NSRL
server, the default is: '
This doesn't query "an NSRL server", it talks to instances of nsrlsvr. Please
fix this docstring, and other instances of this terminology.

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
plaso/cli/helpers/nsrlsvr_analysis.py:62: BadConfigOption: when the nsrlsvr
cannot be connected.
"be connected to" or "when it is not possible to connect to nsrlsvr"

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/viper_a...
File plaso/cli/helpers/viper_analysis.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/viper_a...
plaso/cli/helpers/viper_analysis.py:37: u'Type of hash to query the Viper
server, the default is: '
"to use to query..."

[Joachim Metz, 2016-10-16 09:28:12]

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/interface.py
File plaso/analysis/interface.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/interface....
plaso/analysis/interface.py:431: """Sets the hash to query the nsrlsvr.
On 2016/10/16 09:09:57, onager wrote:
> Please fix docstring, this isn't nsrlvr

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py
File plaso/analysis/nsrlsvr.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:41: """Connects to the nsrlsvr.
On 2016/10/16 09:09:57, onager wrote:
> -the or +instance

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:114: def SetHostname(self, host):
On 2016/10/16 09:09:57, onager wrote:
> -name, this also works for an ip address, which is not a hostname

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:132: 
On 2016/10/16 09:09:57, onager wrote:
> + "The nsrlsvr instance will be queried for a specific MD5 hash that should
> exist."

Nope, the nsrlsvr instance is just queried.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:134: bool: True if the nsrlsvr is reachable.
On 2016/10/16 09:09:57, onager wrote:
> -the or +instance

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:140: nsrl_socket, u'd41d8cd98f00b204e9800998ecf8427e')
why this is just a test MD5?

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/nsrlsvr.py...
plaso/analysis/nsrlsvr.py:178: def SetHostname(self, host):
On 2016/10/16 09:09:57, onager wrote:
> See above for Host vs Hostname

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/viper.py
File plaso/analysis/viper.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/analysis/viper.py#n...
plaso/analysis/viper.py:23: hash_analysis_queue (Queue.queue): that the analyzer
will append
leaving as-is copied from nsrlsvr.py

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
File plaso/cli/helpers/nsrlsvr_analysis.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
plaso/cli/helpers/nsrlsvr_analysis.py:36: u'Type of hash to query the NSRL
server, the default is: '
per previous reviews, please provide an example how you think it should be.

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/nsrlsvr...
plaso/cli/helpers/nsrlsvr_analysis.py:62: BadConfigOption: when the nsrlsvr
cannot be connected.
On 2016/10/16 09:09:57, onager wrote:
> "be connected to" or "when it is not possible to connect to nsrlsvr"

Done.

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/viper_a...
File plaso/cli/helpers/viper_analysis.py (right):

https://codereview.appspot.com/312850043/diff/40001/plaso/cli/helpers/viper_a...
plaso/cli/helpers/viper_analysis.py:37: u'Type of hash to query the Viper
server, the default is: '
On 2016/10/16 09:09:57, onager wrote:
> "to use to query..."

Done.

[Joachim Metz, 2016-10-16 10:47:22]

Code updated.

[Joachim Metz, 2016-10-16 12:14:20]

Code updated.

[Joachim Metz, 2016-10-16 14:08:00]

Code updated.

[Joachim Metz, 2016-10-16 14:42:50]

Code updated.

[Joachim Metz, 2016-10-16 14:43:41]

PTAL, fixed more related issues in this CL and improvements to status reporting
for trouble shooting

[Joachim Metz, 2016-10-16 15:08:19]

Code updated.

[onager, 2016-10-16 21:28:39]

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface.py
File plaso/analysis/interface.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface...
plaso/analysis/interface.py:199: return
Please log this happening. If an event is for some reason missing the required
attribute, it's better to tell the user as soon as possible.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface...
plaso/analysis/interface.py:366: # Indicate that this is a daemon thread. The
program will exit if only
Remove this comment, it no longer applies.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.py
File plaso/analysis/nsrlsvr.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:44: socket._socketobject: socket of connection to
nsrlsvr or None if
"socket connected to"

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
File plaso/cli/helpers/nsrlsvr_analysis.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:36: u'Type of hash to use to query the
NSRL server, the default is: '
"Type of hash to use to query nsrlsvr instance"

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:43: u'Hostname of the NSRL server to
query, the default is: '
"Hostname or IP address of the nsrlsvr instance to query"

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:49: u'Port of the NSRL server to query,
the default is: {0:d}.'.format(
"Port number of the nsrlsvr instance to query"

https://codereview.appspot.com/312850043/diff/140001/plaso/multi_processing/a...
File plaso/multi_processing/analysis_process.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/multi_processing/a...
plaso/multi_processing/analysis_process.py:181: logging.debug(u'Analysis plugin:
{0!s} (PID: {1:d}) stopped'.format(
This is misleading message now, as the process will be hanging around for
another _FOREMAN_STATUS_WAIT seconds. Please fix.

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

https://codereview.appspot.com/312850043/diff/140001/plaso/storage/interface....
plaso/storage/interface.py:454: number_of_analysis_repors (int): number of
analysis repors written.
reports

[onager, 2016-10-16 21:33:43]

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.py
File plaso/analysis/nsrlsvr.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:40: def _Connect(self):
_GetSocket() - this returns a socket, doesn't connect the analyzer to something.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:44: socket._socketobject: socket of connection to
nsrlsvr or None if
Why aren't you raising here?

[Joachim Metz, 2016-10-17 05:47:37]

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface.py
File plaso/analysis/interface.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface...
plaso/analysis/interface.py:199: return
I can add a log, but under normal usage the user will not see this.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/interface...
plaso/analysis/interface.py:366: # Indicate that this is a daemon thread. The
program will exit if only
On 2016/10/16 21:28:39, onager wrote:
> Remove this comment, it no longer applies.

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.py
File plaso/analysis/nsrlsvr.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:40: def _Connect(self):
On 2016/10/16 21:33:43, onager wrote:
> _GetSocket() - this returns a socket, doesn't connect the analyzer to
something.

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:44: socket._socketobject: socket of connection to
nsrlsvr or None if
On 2016/10/16 21:33:43, onager wrote:
> Why aren't you raising here?

what's the advantage of having the try catch and log error in 2 places?

https://codereview.appspot.com/312850043/diff/140001/plaso/analysis/nsrlsvr.p...
plaso/analysis/nsrlsvr.py:44: socket._socketobject: socket of connection to
nsrlsvr or None if
On 2016/10/16 21:28:39, onager wrote:
> "socket connected to"

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
File plaso/cli/helpers/nsrlsvr_analysis.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:36: u'Type of hash to use to query the
NSRL server, the default is: '
On 2016/10/16 21:28:39, onager wrote:
> "Type of hash to use to query nsrlsvr instance"

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:43: u'Hostname of the NSRL server to
query, the default is: '
On 2016/10/16 21:28:39, onager wrote:
> "Hostname or IP address of the nsrlsvr instance to query"

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/cli/helpers/nsrlsv...
plaso/cli/helpers/nsrlsvr_analysis.py:49: u'Port of the NSRL server to query,
the default is: {0:d}.'.format(
On 2016/10/16 21:28:39, onager wrote:
> "Port number of the nsrlsvr instance to query"

Done.

https://codereview.appspot.com/312850043/diff/140001/plaso/multi_processing/a...
File plaso/multi_processing/analysis_process.py (right):

https://codereview.appspot.com/312850043/diff/140001/plaso/multi_processing/a...
plaso/multi_processing/analysis_process.py:181: logging.debug(u'Analysis plugin:
{0!s} (PID: {1:d}) stopped'.format(
moved it after the wait.

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

https://codereview.appspot.com/312850043/diff/140001/plaso/storage/interface....
plaso/storage/interface.py:454: number_of_analysis_repors (int): number of
analysis repors written.
On 2016/10/16 21:28:39, onager wrote:
> reports

Done.

[Joachim Metz, 2016-10-17 05:52:09]

Code updated.

[onager, 2016-10-21 12:47:09]

LGTM

[Joachim Metz, 2016-10-22 05:18:37]

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 nsrlsvr